feat(j1939tp)!: serve retransmit requests, normalise PDU1 PGNs, refuse an RTS allowing no packet, deliver before the event, and rename Th to BamPacketSpacing - #152
Conversation
…up it names, refuse an RTS allowing no packet, deliver before the event, and name the BAM spacing what it is #58, the four transport findings, and #144. A CTS for a packet already sent is a retransmit request (J1939-21 §5.10.2.4) and is served now, from that packet on, up to J1939TpOptions.MaxRetransmitRequests (default 2) times per session; the next one is answered with table 7's reason 5, the limit that code names. The whole PDU is in hand, so nothing is lost by starting over. A PDU1 PGN in a TP.CM is read with its low byte cleared, so a stack that writes the destination address there still reaches the session keyed on the PGN; on the sending side a PDU1 PGN with the byte set is refused, as J1939Id.ComposePgn refuses it since #55, so the session is keyed on what the peer will name. An RTS allowing no packet per CTS is not "no limit" -- that is 0xFF -- and opens no session; every CTS this side could send would be a hold. And the datagram is in the inbox before DatagramReceived is raised, off the actor, as IsoTpChannel does, so a handler that waits on the channel gets the datagram instead of deadlocking it. The tests used PDU1 PGNs with the low byte as their per-test uniqueness; they use PDU2 PGNs now, where the byte is the group extension. Mutation-checked, one per finding: without the normalisation the peer's CTS is dropped and the send times out; with 0 read as no limit the malformed RTS gets a CTS; with retransmits never served, and with the limit never reached, the retransmit test fails at its respective step; with the event raised first on the actor the handler's wait times out. BREAKING CHANGE: J1939TpOptions.Th is renamed BamPacketSpacing, and the th parameter of J1939TpOptions.With is renamed bamPacketSpacing (#144). The option is the 50..200 ms spacing between BAM packets of §5.10.3; the standard's Th is the holding time between CTS(0) messages, which this stack does not send. Per docs/decisions/0001-versioning-and-api-stability.md this is a minor bump before v1.3.0 and no shim is introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview TP.CM originator now honors CTS retransmit requests (packets already sent), up to new PDU1 PGNs: outbound sends reject a non-zero low byte; inbound
Breaking: Reviewed by Cursor Bugbot for commit b32ca2b. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 713088449e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7130884. Configure here.
…dOfMsgAck after a partial retransmit, and state the event's threading Codex and Bugbot on #152. A 255-packet message wraps the byte NextSn to 0 once every packet is sent, and a retransmit request for its last packet read as a bad sequence number; the expected next packet is TotalPackets + 1 as an int in that state. A retransmit that did not reach the last packet left the originator waiting for a CTS and rejecting the EndOfMsgAck a receiver that has the rest sends instead; every packet having gone out at least once makes that a valid end. And IJ1939TpChannel.DatagramReceived says it is raised on the thread pool after the inbox write, with what that means for order and concurrency. Mutation-checked: with the wrapped byte the 255-packet test aborts; with the EOM check as before, the partial-retransmit test aborts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff02cc1a08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the outstanding packet is confirmed Codex on #152: a CTS for a packet already sent that arrived while a block was still draining was stashed and applied after the block, so the receiver, missing a packet, kept getting later ones out of sequence. It takes effect once the outstanding DT is confirmed. Mutation-checked: applied at block end only, the new test sees packet 2 follow instead of packet 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb331b66fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ile a block drains Codex on #152: mid-block, every sequence number below the grant's end read as a retransmit request, so a CTS for a packet not yet sent skipped the ones between. While a block drains, a packet is "already sent" only up to the outstanding one; a later one of the grant is a sequence error (table 7, code 7). Mutation-checked: with the earlier classification the new test's send does not abort. CodeQL's unread send in the mid-block test is observed now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebb24b3dee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ghest packet ever sent Codex on #152: after a partial retransmit the cursor is below the highest packet sent, so a request for a packet between the two was a sequence error and one for the packet at the cursor passed as the next block, uncounted. The session keeps the highest packet ever confirmed, and a request at or below it -- or below the outstanding packet while a block drains -- is a retransmit, the packet after it the next block. Mutation-checked: classified against the cursor, one new test aborts and the other serves an uncounted retransmit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
What does this change?
The four transport findings of #58 and the rename of #144, in one J1939-TP pull request; the node findings of #58 follow in a J1939 one, which closes it.
MaxRetransmitRequests(new option, default 2) times per session; the next one gets table 7's reason 5, the limit that code names. The stack used to answer the first such CTS with reason 5. The test that pinned that behaviour now sets the option to 0.J1939Id.ComposePgnrefuses it since Addressing: collected minor findings (Decompose validation, ComposePgn truncation, NAME byte order) #55. The tests had used such values for per-test uniqueness; they use PDU2 PGNs now.DatagramReceivedis raised after the inbox write, off the actor, asIsoTpChanneldoes, so a handler that waits on the channel gets the datagram rather than deadlocking it.J1939TpOptions.Th→BamPacketSpacing(J1939-TP: J1939TpOptions.Th is the BAM inter-packet hold-off, not J1939-21's Th #144): the option is the BAM inter-packet spacing of §5.10.3, not the standard's holding time between CTS(0) messages, which this stack does not send. The README's timer paragraph says so.Closes #144.
Type of change
feat— new behaviour (minor release): retransmit requests served,MaxRetransmitRequestsfix/perf— bug or performance fix (patch release): the PGN normalisation, the RTS cap, the delivery order — in the same commitdocs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration) — the rename; a minor bump under ADR-0001 before v1.3.0Checklist
dotnet build CanKit.Pro.sln -c Releasesucceedsdotnet test CanKit.Pro.sln -c Releasepasses (net10.0, all 951)A_Cts_With_The_Destination_In_The_Pgns_Low_Byte_Reaches_The_Session,A_Pdu1_Pgn_With_A_Low_Byte_Is_Refused_Before_Anything_Goes_Out,An_Rts_Allowing_No_Packet_Per_Cts_Opens_No_Session,A_Retransmit_Request_Is_Served_Until_The_Limit,DatagramReceived_Finds_The_Datagram_Already_Receivable,A_Retransmit_Request_For_The_Last_Of_255_Packets_Is_Served,An_End_Of_Message_After_A_Partial_Retransmit_Completes_The_Send,A_Retransmit_Request_Mid_Block_Takes_Effect_After_The_Outstanding_Packet,A_Cts_For_An_Unsent_Packet_Of_The_Block_Is_A_Sequence_Error_Not_A_Retransmit,A_Retransmit_Request_Above_The_Cursor_But_Below_The_Highest_Sent_Is_Served,A_Retransmit_Request_At_The_Cursor_After_A_Partial_Retransmit_Counts; each fails with its change reverted (mutation-checked, the retransmit one at both of its steps)🤖 Generated with Claude Code