Skip to content

Handle a NACK reply instead of killing the read loop - #45

Open
marknolan wants to merge 1 commit into
seemoo-lab:mainfrom
marknolan:handle_nack_command_processed
Open

marknolan wants to merge 1 commit into
seemoo-lab:mainfrom
marknolan:handle_nack_command_processed

Conversation

@marknolan

Copy link
Copy Markdown

The Shimmer Bluetooth protocol answers a command it will not carry out with a NACK
(0xFE) instead of an acknowledgment. pyshimmer has no concept of it, and the
consequence is more serious than a missing feature: one refusal kills the reader
thread and every waiting call blocks forever.

What happens today

process_single_input_event() dispatches on the first byte:

if peek == ACK_COMMAND_PROCESSED:
    self._process_ack()
elif peek == DATA_PACKET:
    self._process_data_packet()
elif peek == INSTREAM_CMD_RESPONSE:
    self._process_in_stream_resp()
else:
    self._process_resp_from_queue()

A 0xFE reaches the else, and _process_resp_from_queue() either raises
queue.Empty from get_nowait() when no response is pending, or raises ValueError
because the response code does not match.

Either exception escapes ShimmerBluetooth._run_readloop, which catches only
ReadAbort. The reader thread dies while the device still looks connected. And since
RequestCompletion.wait() and RequestResponse.wait() block on an Event with no
timeout, that thread's death leaves the current caller — and every later caller —
blocked indefinitely.

I confirmed the failure against an unpatched tree before writing the fix; it raises
queue.Empty.

When the device refuses

A refusal is a normal part of the protocol and says nothing about the health of the
link. The firmware refuses:

  • every command except the SD sync command and an acknowledgment, while SD sync is
    enabled
    — this covers the entire connect sequence, so a sync-enabled device is
    unusable rather than merely awkward
  • a sync command while sync is disabled (the same gate, inverted)
  • any set command while the device is sensing
  • a set-sampling-rate command whose clock divider would be zero
  • an out-of-range InfoMem or calibration write
  • a command whose payload was truncated
  • a handful of commands the dispatcher accepts but that were never implemented

What this changes

_process_nack() takes the pending entry off the acknowledgment queue and marks it
refused, rather than moving it to the response queue — a refused command never produces
a response. Waiting callers get a CommandRefused exception instead of blocking, and
the reader thread carries on, so a later command still runs.

CommandRefused is exported from the package so callers can catch it.
has_completed() and has_result() report True after a refusal, because the request
is finished either way; was_refused() distinguishes the two, and wait() raises.

BluetoothSerial.read_nack() mirrors the existing read_ack().

This does change the contract of wait(), which previously could only return. I think
that is the right trade: the behaviour it replaces is an unbounded block after the
reader thread has already died, so any defined outcome is an improvement, and raising
is the natural way to say "the device declined this".

Tests

Two new cases in TestBluetoothRequestHandler, written against the existing
PTY fixtures:

  • test_enqueue_command_refused — a refused command finishes as refused, carries no
    result, and both wait() calls raise.
  • test_enqueue_refused_then_next_command_still_runs — a refusal followed by a command
    that is acknowledged and answered, so the refusal does not stall the queue.

One caveat worth stating plainly: I could not run these locally. The Bluetooth test
module cannot be collected on Windows, because pyshimmer.test_util imports termios.
I verified the same three scenarios — refusal, refusal-then-success, and an ACK path
left unaffected — through an equivalent in-memory fake serial, and checked that the
unpatched tree fails that harness first so it was not vacuously passing. CI here is the
first run of the PTY versions, so please do look at the result.

black reports no changes.

The Shimmer Bluetooth protocol answers a command it will not carry out
with a NACK (0xFE) rather than an acknowledgment. The API had no concept
of it, so process_single_input_event() fell through to
_process_resp_from_queue(), which either raised queue.Empty because no
response was pending or raised ValueError on the unexpected response
code.

Either exception escapes ShimmerBluetooth._run_readloop, which catches
only ReadAbort. The reader thread then dies while the device still looks
connected, and because RequestCompletion.wait() and
RequestResponse.wait() block on an Event with no timeout, that thread's
death leaves the caller - and every later caller - blocked for good.

A refusal is a normal part of the protocol and says nothing about the
health of the link. The firmware refuses every command except the SD sync
command and an acknowledgment while SD sync is enabled, which covers the
whole connect sequence; it also refuses any set command while the device
is sensing, a sync mode mismatch, an out-of-range InfoMem or calibration
write, and a handful of commands it accepts but has never implemented.

_process_nack() takes the pending entry off the acknowledgment queue and
marks it refused rather than moving it to the response queue, since a
refused command never produces a response. Waiting callers get a
CommandRefused exception instead of blocking, and the reader thread
carries on, so a later command still runs.

CommandRefused is exported from the package so callers can catch it.
has_completed() and has_result() report True after a refusal, because the
request is finished either way; was_refused() distinguishes the two, and
wait() raises.

Two tests cover a refused command and a refusal followed by a command
that succeeds. Note that the Bluetooth test module cannot be collected on
Windows - pyshimmer.test_util imports termios - so these were written
against the existing PTY-based fixtures and verified locally through an
equivalent in-memory fake, with the unpatched tree checked first to
confirm it does fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lumagi

lumagi commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Wow, thank you for the code. It looks really nice!

@lumagi lumagi linked an issue Sep 24, 2026 that may be closed by this pull request
@lumagi

lumagi commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

And you're right, the test cases use the Linux PTY pseudoterminal package. I don't know if there exists something that's cross-platform, but on Linux it works quite well to emulate a duplex serial connection.
I took a look at the failing test case, and it looks like I happened to use 0xfe as a magic number in test_incorrect_resp_code_fail, which now collides with the existing NACK command. I don't have the Github CLI on my laptop, so there's no quick way for me to push to this PR, but I think if you simply change the hex code in the following line to something other than 0xfe then it should work:

mock_creator.write_to_master(b"\xff\xfe")

This branch has not been deployed

No deployments
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.

implement NACK handling

2 participants