docs: S23.22 give the Datagram role an implementor contract, and Plus-TCP its exceptions - #737
Merged
Conversation
The Datagram vtable said only "see the wrappers", so what an implementor must guarantee was nowhere. It says it now, in the split the Stream role already uses: report MaxPayload for the path in use and never guess high, return OVERSIZE where the platform can distinguish it, and SENT only once the record has actually been handed to the network. The caller-side header gains the consequence of the permission it already grants. An implementation that collapses OVERSIZE into FAILED is conforming, but the sender's trim is reactive — it offers the record at full size and only asks MaxPayload once told the record was too large. So on those platforms an over-large record reaches the stack whole and is lost. Tracked as #736. FreeRTOS-Plus-TCP is one of them, and its page says so, with the sizing advice that mitigates it until the fix lands. Its other undocumented behaviour is the 50 ms yield the datagram adapter takes on an ARP cache miss, because the stack drops rather than queues while ARP resolves — paid by whichever task made the call, on a first send and after cache expiry. Part of #708
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR clarifies ChangesDatagram documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Interface/SolidSyslogDatagram.h`:
- Around line 41-47: Update the send-result documentation in SolidSyslogDatagram
so implementations must not classify OVERSIZE as transient FAILED; either remove
that collapse behavior or define FAILED as “not sent; apply the caller’s
retention policy.” Ensure SolidSyslogUdpSender and SolidSyslog_Service semantics
remain consistent so oversized records do not block later records through
repeated retries.
In `@Core/Interface/SolidSyslogDatagramDefinition.h`:
- Around line 25-28: Clarify the MaxPayload documentation to state that it
applies to the destination selected by the current connection. Require callers
to close and reopen the datagram before changing destinations, or require
multi-destination implementations to return a conservative limit safe for every
supported destination.
In `@docs/platforms/plustcp/index.md`:
- Line 34: Update the platform documentation text to refer explicitly to the
FreeRTOS-Plus-TCP API as `FreeRTOS_sendto` instead of the ambiguous `sendto`,
including the existing ARP-resolution statement.
- Around line 45-57: Revise the workaround guidance in the “An over-large
datagram is lost rather than trimmed” section to state that setting
SOLIDSYSLOG_MAX_MESSAGE_SIZE to the PlusTcp UDP path’s supported payload, such
as 1232 bytes, only prevents oversized datagrams and truncates formatted
records. Explicitly note that the global limit also affects TCP and switching
senders, and do not describe it as preserving long records across transports.
- Around line 32-43: Update the “first send to an unresolved peer” section to
identify the adapter’s vTaskDelay(pdMS_TO_TICKS(50)) as the source of the wait,
note that configTICK_RATE_HZ affects its duration, and state that
ipconfigMAX_ARP_AGE controls when the cache-miss path can recur. Replace each
sendto reference with FreeRTOS_sendto and do not attribute the delay to
FreeRTOS-Plus-TCP or FreeRTOSIPConfig.h.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f186256-c7ee-4fc6-8d3e-849af8388aff
📒 Files selected for processing (3)
Core/Interface/SolidSyslogDatagram.hCore/Interface/SolidSyslogDatagramDefinition.hdocs/platforms/plustcp/index.md
The review was right that this is worse than a dropped record. A failed send is treated as transient — the store keeps the record at its cursor and offers the same one every servicing pass — so an over-large datagram the stack rejects blocks everything behind it, rather than being lost on its own. FAILED staying transient is still correct, and the header now says why: its usual causes are an unreachable collector, a wrong address, a stack not yet up, all resolved outside the library, and holding the record until they are is what an audit trail wants. Size is the one cause the record itself carries, which is what makes it the exception. The Plus-TCP workaround was too glib. SOLIDSYSLOG_MAX_MESSAGE_SIZE is library-wide, so lowering it to fit this UDP path truncates records on every transport the instance uses. Also from the review: the 50 ms ARP wait is the adapter's own vTaskDelay rather than anything the stack does, rounded by configTICK_RATE_HZ and recurring as ipconfigMAX_ARP_AGE expires entries; FreeRTOS_sendto spelled in full so it cannot be read as the POSIX call; and MaxPayload's path is the destination currently being sent to. Part of #708
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Interface/SolidSyslogDatagram.h`:
- Line 49: Update the documentation sentence in the SolidSyslogDatagram API
comments to refer to the caller operation as SolidSyslogDatagram_MaxPayload,
reserving MaxPayload only for references to the vtable member.
In `@docs/platforms/plustcp/index.md`:
- Line 55: Update the sentence in the FreeRTOS_sendto documentation by adding a
comma after “it” and replacing “the send” with “the send operation,” preserving
the rest of the wording.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ca030e7-95ee-4d96-ab25-d05fd3b04379
📒 Files selected for processing (3)
Core/Interface/SolidSyslogDatagram.hCore/Interface/SolidSyslogDatagramDefinition.hdocs/platforms/plustcp/index.md
Collaborator
Author
|
@coderabbitai pause |
Contributor
✅ Action performedReviews paused. |
Two nits from the review. The caller-facing header referred to the operation as MaxPayload, which is the vtable member's name; the caller calls SolidSyslogDatagram_MaxPayload, and the header now says so. And a comma after the subordinate clause on the Plus-TCP page. CodeRabbit is paused on this PR, so this push does not spend a review. Part of #708
Contributor
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The FreeRTOS-Plus-TCP triage. Its own removed content was fully accounted for
already — the lede, the manifest table and the
ipconfigUSE_DNSrequirement allsurvived the restructure — so the value here was verifying the page's claims
against the code, and two things came out of that.
Part of #708
Change Description
SolidSyslogDatagramDefinition.hgains an implementor contract. It said only"per-method contract is on the wrappers", so what an implementor must guarantee
was written nowhere. It now uses the split the
Streamrole alreadyuses — the caller-facing header says what a caller may expect, the Definition
header says what an implementor must deliver: report
MaxPayloadfor the path inuse and never guess high, return
OVERSIZEwhere the platform can distinguishit, and return
SENTonly once the record has actually been handed to thenetwork.
SolidSyslogDatagram.hgains the consequence of a permission it alreadygrants. It already says an implementation that cannot detect oversize may
collapse it into
FAILED, so those platforms are conforming. What it did not sayis that the sender's trim is reactive — it offers the record at full size and
consults
MaxPayloadonly once told the record was too large. So on thoseplatforms an over-large record is never trimmed: it reaches the stack whole and
is lost. Raised as #736.
FreeRTOS-Plus-TCP's page gains two exceptions, both verified in the adapter:
to 50 ms. The stack drops rather than queues while ARP resolves, so the adapter
probes and yields. Sound, well commented in the source, and undocumented until
now. It is paid by the application's thread on an inline wiring or the
servicing thread on a buffered one.
sizing advice that mitigates it until fix: an oversize datagram is lost on any platform that cannot detect oversize #736 lands.
What deliberately did not change
docs/rfc-compliance.md's RFC 5426 §3.2 row stays Supported. Posix andWindows detect oversize and trim, so the library with a conforming platform under
it meets the requirement; the platforms that do not are the exception, and the
exception is recorded on their pages. That the eventual fix may live in Core does
not make it any less a property of choosing this platform today.
Plus-TCP's
@filebriefs are 61–109 characters, inside the band the finishedplatforms sit at, so they needed no work.
Test Evidence
Documentation, plus comment-only changes to two Core headers.
mkdocs build --strict, markdownlint,python3 scripts/check_platform_docs.py(10 platforms,12 roles, boundary clean), and
clang-format --dry-run --Werroron both headers— all green. No MISRA suppression references either header, so no renumbering. No
production logic changed.
Areas Affected
Core/Interface/SolidSyslogDatagram.handSolidSyslogDatagramDefinition.h(comments only), and
docs/platforms/plustcp/index.md. No public API change.Summary by CodeRabbit