feat: add J1939Message for subscriber callbacks with destination address - #84
feat: add J1939Message for subscriber callbacks with destination address#84RaulSMS wants to merge 1 commit into
Conversation
_notify_subscribers() has always received the destination address from
the data link layer but never passed it to subscriber callbacks, which
only get (priority, pgn, sa, timestamp, data). This makes it impossible
for a subscriber to know which destination address a message was
addressed to.
Add j1939.J1939Message, a NamedTuple carrying all five existing fields
plus dest_address. Subscribers can opt in by declaring a single-argument
callback:
def on_message(msg: j1939.J1939Message):
...
The calling convention is detected once, from the callback's signature,
at subscribe() time (ElectronicControlUnit._callback_takes_message),
so existing 5-argument callbacks throughout the codebase, examples, and
downstream users keep working unchanged -- this is intentionally
additive rather than a hard breaking change, since callbacks in this
kind of protocol stack often run on embedded/field systems with slower
upgrade cycles.
Internal library subscribers (DM1, DM14/16, memory_access) are left on
the legacy form since none of them currently use the destination
address; migrating them would be pure churn. Updated the two simplest
examples and the README quick-start to demonstrate the new style.
Fixes #61
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| self._bus = None | ||
|
|
||
| @staticmethod | ||
| def _callback_takes_message(callback): |
There was a problem hiding this comment.
I think for this one it may be worth documenting that it returns a bool, and that it's true if it does take the new single arg format. Since it's not the most straight forward on it's evaluation.
khauersp
left a comment
khauersp
left a comment
There was a problem hiding this comment.
Thanks for getting to this. I have a general style question. Is it better to leave it how it is and just indefinitely support the legacy way of doing it? Or should we implement a deprecation notice and give a heads up we are going to be moving away from the legacy implementation? It seems this new way is more sustainable and extensible than the legacy way so I wasn't sure the benefit of dragging the legacy implementation along indefinitely.
| """ | ||
| print(f"PGN {hex(pgn)} length {len(data)}") | ||
| def on_message(msg: j1939.J1939Message): | ||
| """Receive incoming messages from the bus""" |
There was a problem hiding this comment.
we should probably still document the param here
| """ | ||
| print(f"PGN {pgn} length {len(data)}") | ||
| def on_message(msg: j1939.J1939Message): | ||
| """Receive incoming messages from the bus""" |
There was a problem hiding this comment.
we should probably also document the param here still
| priority: int | ||
| pgn: int | ||
| source_address: int |
There was a problem hiding this comment.
I think it would be better to have the arbitration id here. If we care about the other fields we can get them from the Arbitration Id. This J1939Message would then comprise of:
arbitration_id, timestamp, data, and dest_address
3 participants
Summary
Implements the RFC in #61 (Option C, message dataclass), with the adjustment discussed in that issue's comment thread: additive rather than a hard breaking change.
j1939.J1939Message, aNamedTuplewithpriority, pgn, source_address, timestamp, data, dest_address.ElectronicControlUnit._notify_subscribersnow passesdest_addressto subscribers that opt in, by declaring a single-argument callback:subscribe()time (ElectronicControlUnit._callback_takes_message), so existing 5-argument callbacks keep working completely unchanged — no forced migration for current users. All 527 pre-existing tests pass with zero modifications.NamedTuple(rather than@dataclass) was chosen sodest_addressis appended last — a new-style callback still getspriority, pgn, sa, timestamp, datain the same order as the legacy form if it ever needs to unpack positionally.What's intentionally NOT changed
Per discussion, internal library subscribers (
diagnostic_messages.py'sDm1._receive,memory_access.py's_listen_for_dm14,Dm14Query/Dm14Server's parse chain) are left on the legacy 5-arg form, since none of them use the destination address today — migrating them would be pure churn with no functional benefit, and would ripple into several more file's signatures.Only the two simplest examples (
simple_receive_global.py,simple_receive_peer_to_peer.py) and the README quick-start were updated to demonstrate the new style; the rest ofexamples/and the second README example are left on the legacy form to also demonstrate that both styles are supported side by side.Test plan
test/test_ecu.py:test_subscribe_new_style_receives_j1939_message,test_subscribe_new_style_receives_peer_to_peer_dest_address(verifies passive wildcard observation of PDU1 dest, consistent with Destination-specific (PDU1) messages are dropped for wildcard subscribers (device_address=None) #59),test_subscribe_both_calling_conventions_side_by_side.pytest . --pyargs -q— 530 passed (527 pre-existing + 3 new), zero pre-existing tests modified.ruff check .— cleanpyright— 0 errorsvermin --target=3.10-— compatiblepython -m build && python -m twine check dist/*— passes,j1939/message.pypackaged correctly