Skip to content

fix(j1939-21): respect sequence number for fragmented frames on reass… - #73

Open
drewr95 wants to merge 2 commits into
masterfrom
fix/69-sequential-packet-reassembly
Open

fix(j1939-21): respect sequence number for fragmented frames on reass…#73
drewr95 wants to merge 2 commits into
masterfrom
fix/69-sequential-packet-reassembly

Conversation

@drewr95

@drewr95 drewr95 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #69

This pull request strengthens the Transport Protocol (TP) implementation in j1939_21.py by enforcing strict sequence checking for incoming TP.DT packets, ensuring that out-of-sequence packets are correctly detected and handled according to the J1939-21 specification. It also adds comprehensive tests to verify these behaviors.

Transport Protocol improvements:

  • Enforces strict sequence checking in _process_tp_dt: Now tracks the next expected packet and raises a ValueError if a duplicate or invalid sequence number is received, aborting the session and cleaning up resources as appropriate.
  • Initializes the next_expected_packet field in the receive buffer during both BAM and CMDT session setup in _process_tp_cm, ensuring consistent state tracking. [1] [2]

Testing enhancements:

  • Adds new tests in test/test_ecu.py to verify that out-of-sequence TP.DT packets are correctly rejected for both BAM and peer-to-peer (CMDT) sessions, and that session state is properly cleaned up after errors.
  • Ensures that no notifications or messages are sent when an out-of-sequence error occurs, and that the receive buffer is cleared.

Test infrastructure:

  • Imports pytest and relevant classes to support the new test cases.

These changes improve protocol compliance, robustness, and test coverage for the J1939 TP implementation.

@drewr95
drewr95 requested review from RaulSMS and khauersp and a lite review from Copilot August 20, 2026 18:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses Issue #69 by enforcing strict TP.DT sequence-number ordering during J1939-21 multi-packet reassembly, aborting/terminating sessions when out-of-sequence packets are received to prevent corrupted payload delivery.

Changes:

  • Track next_expected_packet in the receive buffer for both BAM and CMDT sessions.
  • Validate TP.DT packet sequence numbers in _process_tp_dt, abort/cleanup on violations, and raise a ValueError.
  • Add ECU-level tests covering out-of-sequence scenarios for BAM and CMDT sessions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
j1939/j1939_21.py Adds receive-side sequence tracking and abort/error behavior for out-of-sequence TP.DT packets.
test/test_ecu.py Adds tests ensuring out-of-sequence TP.DT packets terminate sessions and prevent partial delivery.
Suppressed comments (1)

test/test_ecu.py:128

  • This test requests the feeder fixture but never uses it, which adds unnecessary setup/overhead and can hide accidental dependencies. Remove the unused fixture parameter.
def test_peer_to_peer_sequence_gap_after_valid_packet_aborts(feeder):

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread j1939/j1939_21.py Outdated
Comment thread test/test_ecu.py Outdated
@drewr95
drewr95 force-pushed the fix/69-sequential-packet-reassembly branch from f3b9215 to a8f7a11 Compare August 20, 2026 18:54

@khauersp khauersp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to implement this for j1939_22?

@drewr95

drewr95 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Do we need to implement this for j1939_22?

I'm honestly not sure if j1939-22 requires this. It can be a follow up issue and I can research into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

J1939-21 Sequence Number for fragmented frames is not respected on reassembly

3 participants