Add a que for multimessages

[kellergoech]

The que will buffer messages when these are triggered while sending of another message is still active. The que will be overwritten by new messages when sending is not active.
-> Therefore the application which uses this function does not need to care about the state of the send buffer.

[Copilot]

Pull request overview

This PR adds a send-queue mechanism to the J1939-21 transport-protocol path so multi-packet messages triggered while another transfer is active can be buffered and sent later, reducing the need for application-side flow control.

Changes:

• Introduced a new _snd_que structure to buffer pending multi-packet sends.
• Refactored multi-packet send initialization into a new helper (_put_multi_msg).
• Added queue-draining logic to async_job_thread to start a queued transfer once sending becomes idle.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[khauersp]

Copilot may have raised some good points, it seems the tests are running significantly slower than normal. Also, can we add some tests to verify the changes work as anticipated?

[kellergoech]

When i have time i can add some tests. Atm I have this change active in our production setup because otherwise when i trigger multiple multimessages simultaniously only the first will be send the others will get stuck. Maybe it should be an optional parameter?

[kellergoech]

The interlock was not present in my old version, it should now also work with this interlock.

[kellergoech]

How can we retrigger the tests? They should pass now

[khauersp]

How can we retrigger the tests? They should pass now

I retriggered them a little while ago, it seems they are still hanging. I'm not seeing any recent commits besides when Raul caught the branch up with main, did you make any other changes? The tests should rerun when you commit a change to the branch

[kellergoech]

I made these changes:

8935305

[kellergoech]

Can i somehow find out why the checks do fail?

[khauersp]

Can i somehow find out why the checks do fail?

I'm not sure if there's a great way outside of trying to make a setup to run the tests locally the same way they are ran on the runners. From looking at the traces, if I had to guess I would think we are hitting some sort of deadlock where something is hanging indefinitely.

[RaulSMS]

Can i somehow find out why the checks do fail?

@kellergoech looks like one scenario from pytest is stuck:

Run pytest . --pyargs
============================= test session starts ==============================
platform linux -- Python 3.13.15, pytest-9.1.1, pluggy-1.6.0
rootdir: /home/runner/work/python-can-j1939/python-can-j1939
configfile: pyproject.toml
collected 511 items
test/test_ca.py ...............                                          [  2%]
test/test_dtc_conversion_methods.py .................................... [  9%]
........................................................................ [ 24%]
........................................................................ [ 38%]
........................................................................ [ 52%]
........................................................................ [ 66%]
......     

test -->https://github.com/RaulSMS/python-can-j1939/blob/master/test/test_dtc_conversion_methods.py

CI

you can try to run the test on your local setup

pytest . --pyargs

This guide should also help.

[RaulSMS]

although it is quite strange because you did not touch anything related to conversion methods as far as I could see

[RaulSMS]

For comparison, this is a green run:

[RaulSMS]

although it is quite strange because you did not touch anything related to conversion methods as far as I could see

@kellergoech actually now that I gave a second look, it is probably hanging on https://github.com/RaulSMS/python-can-j1939/blob/master/test/test_ecu.py

[RaulSMS]

@kellergoech @khauersp I can confirm it is hanging on test_ecu.py here the findings from Claude, I have not review them, do you think this make sense:

CLAUDE review

Review: test_ecu.py hang on pr-64

Scope

Diff assessed: master...pr-64 (j1939/j1939_21.py only, +77/-53).

The change adds a send queue (self._snd_que) so that send_pgn no longer returns False
outright when a TP session is already active for a given (src, dest) pair — instead it
queues the message and a new block in async_job_thread is supposed to drain the queue once
the previous send buffer completes.

Root cause

j1939/j1939_21.py:196-199:

# get from que if buffer is empty
if not bool(self._snd_buffer):
    key = next(iter(self._snd_que.items()))
    self._put_multi_msg(**self._snd_que.pop(key))

async_job_thread is invoked from ElectronicControlUnit._protocol_job_thread
(j1939/electronic_control_unit.py:600-608) in a tight loop, with no try/except around
the call:

while not self._job_thread_end.is_set():
    now = time.monotonic()
    next_wakeup = self.j1939_dll.async_job_thread(now)
    ...

Two bugs in the new block combine to kill this thread almost immediately:

1. Unguarded next() on a possibly-empty queue. self._snd_buffer is empty in the
   idle/steady state (the common case — most of the time nothing is being sent). Whenever
   that's true, the code unconditionally does next(iter(self._snd_que.items())). If
   _snd_que is also empty (again, the common case — no message is pending), this raises
   StopIteration. Since nothing catches it, it propagates out of async_job_thread, out of
   _protocol_job_thread's call site, and terminates the entire while loop — the protocol
   job thread silently dies.

2. Wrong key extracted even in the non-empty case. key = next(iter(self._snd_que.items()))
   binds key to a full (dict_key, dict_value) tuple (because .items() yields pairs), not
   just the dict key. The subsequent self._snd_que.pop(key) would then raise KeyError
   (tuple is not a valid key), since the tuple never matches an actual key.

Impact

Once the protocol job thread dies (bug N1 fires on essentially the first idle tick), all
TP/BAM timeout handling and any protocol-level bookkeeping that depends on
async_job_thread running stops permanently for the life of the ElectronicControlUnit.
Tests that queue a multi-packet TP transfer and then wait for the job thread to drive
timeouts/state transitions (as exercised throughout test_ecu.py) block forever waiting
for events that will never be produced — manifesting as the observed hang partway through
pytest . --pyargs.

This is a full regression versus master, where the pre-existing code returned False
immediately from send_pgn when a buffer slot was already occupied, and async_job_thread
had no dependency on a queue at all.

Proposed patch

# get from que if buffer is empty
if not self._snd_buffer and self._snd_que:
    key = next(iter(self._snd_que))
    self._put_multi_msg(**self._snd_que.pop(key))

Changes:

• Guard with and self._snd_que so next()/StopIteration is never reached when the queue
  is empty (the common case).
• Iterate self._snd_que (the dict itself, yielding keys) instead of .items(), so key is
  the actual dict key and self._snd_que.pop(key) works.

Additional consideration (not fixed above, worth raising with the author)

Even with the fix, _protocol_job_thread still has no try/except around
self.j1939_dll.async_job_thread(now), unlike the dispatch thread (which wraps notify(...)
in try/except Exception: logger.exception(...), see
electronic_control_unit.py:576-579). Any future exception in a DLL's async_job_thread
implementation will again silently kill the protocol thread with no log output. Consider
wrapping the call the same way the dispatch thread does, as defense in depth — this is a
pre-existing gap on master, not something introduced by this PR, but this PR shows how
easily a job-thread crash can go unnoticed.

[kellergoech]

Will look into it.