Handle NACK from a Shimmer3/3R instead of dropping the connection

[marknolan]

The protocol has had a NACK_COMMAND_PROCESSED (0xFE) reply for a long time, but
the driver never learned about it. Grepping the whole driver for NACK — excluding
the generated instruction set and the unrelated Verisense protocol — returned two
comments and nothing else.

What was happening

Both protocol implementations tested only for ACK, with no else:

• ShimmerBluetooth.java:833
• LiteProtocol.java:569

So a NACK was read, silently discarded, and the transaction stalled with
mWaitForAck still true and the instruction stack still locked. Two seconds later
(ACK_TIMER_DURATION) the ACK timer expired and checkForAckOrRespTask called
connectionLost().

There was no retry to soften it. NUMBER_OF_TX_RETRIES_LIMIT is 0, and reading
the NACK byte itself resets mNumberofTXRetriesCount to 0, so 0 >= 0 always
holds, one of the two connectionLost() branches always won, and the soft-recovery
else was dead code.

A refused command was indistinguishable from a dead link, and cost the
connection.

Where this actually bites

checkForAckOrRespTask branches on whether the device is streaming. The
connectionLost() teardown above is the not-streaming branch; while streaming
the same timeout instead calls clearAllInstructions(), which destroys the queued
instructions but leaves the link up.

Configuring a device mid-stream is not a supported operation — the only commands
sent while sensing are start/stop onboard recording and stop streaming, and
START_LOGGING_ONLY_COMMAND/STOP_LOGGING_ONLY_COMMAND already have their own
special-cased timeout handling in checkForAckOrRespTask that avoids
connectionLost(). So the not-streaming branch is the one that matters, and an
earlier revision of this description had that backwards.

Counting the firmware's refusals in log-and-stream-common/Comms/shimmer_bt_uart.c,
exactly one of ~14 sendNack sites is the sensing gate. The rest are reachable
while idle:

Condition	Line
SD sync enabled — refuses everything except SET_SD_SYNC_COMMAND and ACK	839, via ShimBt_isCmdAllowedWhileSdSyncing
a sync command while sync is disabled (same XOR gate)	839
payload truncated past args[]	852
SET_SAMPLING_RATE_COMMAND with a zero clock divider	1156
calibration RAM write out of range	1195
SET_DAUGHTER_CARD_MEM EEPROM write failure	1397
SET_CHARGE_STATUS_LED_COMMAND — dispatched but never implemented	1408
SET_BT_COMMS_BAUD_RATE — not supported	1417
SET_INFOMEM_COMMAND offset out of range	1502
RESET_BT_ERROR_COUNTS	1586, 1589
blocked while sensing (ShimBt_isCmdBlockedWhileSensing)	845 — the only streaming one

The SD sync case is the sharpest:

if (storedConfigPtr->syncEnable ^ ShimBt_isCmdAllowedWhileSdSyncing(gAction))
    sendNack = 1;

uint8_t ShimBt_isCmdAllowedWhileSdSyncing(uint8_t command)
{
  return (command == SET_SD_SYNC_COMMAND || command == ACK_COMMAND_PROCESSED);
}

With sync enabled the firmware refuses the driver's entire initialisation sequence,
so on master a sync-enabled Shimmer cannot be connected to at all — it drops
about two seconds in, with nothing in the log saying why.

The existing coping strategy was avoidance rather than handling, which is decent
evidence the failure mode is real — readPressureCalibrationCoefficients() skips
the BMP581 calibration read with the comment "its firmware NACKs … so don't send
it." Avoiding each NACK individually does not scale, and it makes every new
firmware NACK a latent connection-drop in every released copy of the driver.

What this changes

A refusal now unwinds the transaction and keeps the connection up.

ShimmerObject	adds NACK_COMMAND_PROCESSED = (byte) 0xFE
processNackFromCommand()	stops the ACK timer, clears mWaitForAck/mWaitForResponse, reports progress, drops the refused instruction so the stack advances, completes the transaction, releases the stack lock
eventCommandRefused()	the application hook, called after the unwind

The refused instruction is dropped only when it is still queued, which
processNackFromCommand() determines from mWaitForAck on entry. A GET's
instruction is removed as soon as its ACK arrives, so an unconditional removal
would drop whatever was queued behind it and silently skip that command —
Copilot caught that in review, and it is fixed in 648897ba. The
ACK-wait and streaming paths both still have the instruction queued; the
response-wait path does not.

Handled in three places:

1. Waiting for an ACK, not streaming — the main configuration path, and the
   one that matters.
2. Waiting for a response, not streaming — a NACK can land here instead if the
   firmware only discovers it cannot serve a GET while building the response.
   Checked before isKnownResponse(). LiteProtocol gained the matching branch
   in 648897ba after review; it only had the ACK-wait one at first.
3. A data packet followed by a NACK while streaming — off the supported path,
   but still strictly better than master destroying the whole queue.

Completing the operation (8bd5b940)

Unwinding the transaction was not enough on its own. Operation completion is driven
off the ACK path: sendProgressReport(...) advances a counter, and
ShimmerDeviceCallbackAdapter only calls finishOperation() once
mProgressCounter == mProgressEndValue. processNackFromCommand() sent no
progress report, so a single refused command inside an operation meant the
operation never completed — mOperationUnderway stayed true and the application
sat in CONFIGURING/CONNECTING indefinitely.

That matters most at connect time, which is exactly where the SD sync refusals
land. Master never completed the operation either, but it killed the connection
first, so the hang was never visible; making the link survive is what exposes it.

The report is sent before the refused instruction is removed, and only while it is
still queued, because BluetoothProgressReportPerDevice.updateProgress() derives
the counter from the remaining stack size — the response-wait path already reported
when its ACK arrived, and reporting twice there would skip a count. The same three
timer-driven commands the ACK path excludes are excluded here.

Known limitation: a refused command still counts as progress, so the operation
completes as SUCCESS. Reporting a partially-refused operation as
FAIL_WITH_WARNING would mean changing the shared OperationState handling in
finishOperation(), which is wider than this fix; eventCommandRefused() remains
the hook for an application that needs to know.

Two deliberate non-obvious choices

eventCommandRefused() is concrete with an empty default, not abstract — and
not a new ProtocolListener callback either. Every other notification hook here
(sendProgressReport, connectionLost, eventLogAndStreamStatusChanged) is
protected abstract, so adding one would break every downstream subclass and
listener implementation at compile time. This way nothing outside the driver has to
change, and anyone who wants the refusal surfaced overrides it.

A stray NACK with no command in flight is logged and ignored, not treated as a
refusal. processBytesAvailableAndInstreamSupported() runs only when
!mWaitForAck && !mWaitForResponse, so there is no transaction to fail there. The
TypeScript SDK gates its own NACK framing the same way (on a command genuinely
awaiting a reply) so that a leaked stream byte cannot fabricate a refusal; same
reasoning applied here.

Two supporting changes

• The resync helper accepted only 0x00 and 0xFF as a packet boundary, so a
  NACK sitting in the buffer could not be skipped past cleanly. It now accepts
  0xFE, and is renamed findOffsetOfNextZeroOrFF → findOffsetOfNextPacketBoundary
  to match what it actually does (private, one call site).
• NACK is registered in mBtCommandMapOther so btCommandToString names it in
  the logs. Deliberately not in mBtResponseMap — isKnownResponse() must stay
  false for it, or processResponseCommand() would try to parse it as a response
  body.

LiteProtocol gets the same treatment (it is live — BasicShimmerBluetoothManagerPc
instantiates it). Its NACK value is declared locally because the generated
LiteProtocolInstructionSet has no NACK entry; adding it to
LiteProtocolInstructionSet.proto and regenerating would be tidier, but that
rewrites a large generated file committed in two places for the sake of one
constant. Happy to do it that way instead if preferred.

Verification

Bench-tested for the streaming case, compile-verified otherwise.

Mas confirmed on real hardware that master never recognises a device NACK (it times
out silently) while this branch detects it, unwinds and advances. That run used the
SET-while-sensing case, so it exercised the streaming branch — which is why the
~2 s connection drop did not reproduce in it, and why a command queued behind the
refused one did not survive on master: the streaming timeout calls
clearAllInstructions() by design.

TestNackNotStreaming (586c58db) drives the path that actually matters. With SD
sync enabled on the device, every command in the driver's initialisation sequence is
refused, so:

• master — CONNECTION_LOST about two seconds after connecting, unexplained.
• this branch — no teardown, with NACK Received for Command: in the log.

Nothing is written to the device by the test, so the run is non-destructive; SD sync
is set once in Consensys beforehand and cleared afterwards. If FULLY_INITIALIZED
arrives the test reports INCONCLUSIVE rather than a pass, since that means sync was
not actually enabled.

ShimmerDriver and ShimmerPCBasicExamples both compile clean with no new
warnings under Gradle 8.10, the version CI uses.

Note the checked-in Gradle wrapper (6.1) cannot build this project in its current
state, with or without this change, for two independent reasons: grpc-all:1.71.0
pulls a guava whose module metadata has android/jre variants that Gradle 6.1
cannot disambiguate, and ShimmerDriverPC's Shadow plugin (7.0.0) refuses to apply
below Gradle 7. I confirmed both against an untouched tree before assuming they were
mine. CI is unaffected because gradle.yml uses Gradle 8.10.2, so CI here is a
genuine check. The stale wrapper looks worth a separate ticket.

No automated test. There is no test harness for the BT receive path —
ShimmerBluetooth is abstract with a large number of abstract I/O methods and no
existing test instantiates it, so exercising a NACK end-to-end needs a fake
transport that does not exist yet. I did not want to half-build one inside this
change; it is worth its own piece of work, and it would pay for itself well beyond
this fix.

Sibling implementations

The same wire format is implemented independently in several other places, so this was
checked against all of them rather than assumed to be Java-only. Three carried the same
defect and now have their own PRs:

	State
Shimmer-C-API	same defect	#204 - 0xFE falls to the switch default, reported as "Misaligned ByteStream Detected" while streaming, then disconnects after ~10 s of read timeouts
SwiftAPI	same defect, worse outcome	#18 - the pending CheckedContinuation is never resumed, and timeoutInSeconds is declared but never used, so the caller's await never returns at all
pyshimmer	same defect, worse outcome	seemoo-lab/pyshimmer#45 - the unhandled byte raises out of _run_readloop, which catches only ReadAbort, killing the reader thread and blocking every waiter for good
shimmer-web-sdk	already correct	devices/shimmer3/protocol.ts defines NACK = 0xFE, and Shimmer3Client gates the framing on a command genuinely being in flight
Shimmer-MATLAB-ID, Shimmer-Labview-API, Shimmer-Advance-API	nothing to do	wrappers over the Java or C# drivers, so they inherit the fix

Nothing links these repos and nothing tests them together, so the timing differs wildly
by accident: two seconds here, ten in C#, never in Swift and Python.

Not in scope

The related writeMem defect from the tracker ticket is untouched here: the
over-range branch in writeMem() detects a buffer running past the device's memory
range and does nothing, its body being entirely commented out, and
MAX_CALIB_DUMP_MAX (4096) disagrees with the firmware's SHIMMER_CALIB_RAM_MAX
(1024 on Shimmer3) so the guard would be wrong even if enabled. Deciding what should
happen on an over-range write is a separate question from teaching the driver to
read a NACK, so I have left it for its own change.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

There is at least one confirmed transaction-stack correctness bug (and an uncovered NACK path in LiteProtocol) that can cause queued commands to be dropped or time out incorrectly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates the Shimmer driver to recognize and handle NACK_COMMAND_PROCESSED (0xFE) so a refused command unwinds the in-flight transaction and the connection is not dropped due to an ACK/response timeout.

Changes:

• Adds a protocol constant for NACK (0xFE) and registers it for logging/command-to-string mapping.
• Handles NACK in ShimmerBluetooth across ACK-wait, response-wait, and streaming (data packet followed by NACK) paths.
• Adds analogous NACK handling and an overridable eventCommandRefused(...) hook in LiteProtocol.

File summaries

File	Description
ShimmerDriver/src/main/java/com/shimmerresearch/driver/ShimmerObject.java	Adds NACK_COMMAND_PROCESSED constant (0xFE).
ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java	Introduces NACK constant and transaction-unwind handling plus eventCommandRefused(...).
ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java	Adds NACK support in multiple receive paths, updates packet-boundary resync, and adds eventCommandRefused(...).

Review details

Suppressed comments (1)

ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java:1379

• findOffsetOfNextPacketBoundary() uses a raw 0 instead of the existing DATA_PACKET constant; now that this method was touched, using the named constant would avoid a magic number and match the surrounding code style (e.g., bufferTemp[0]==DATA_PACKET).

			byte b = buffer[i];
			if (b == 0 || b == ACK_COMMAND_PROCESSED || b == NACK_COMMAND_PROCESSED)
				return i;

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

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

[marknolan]

Confirmed and fixed in 648897b — this was a real bug, thanks. The GET path does removeInstruction(0) as soon as its ACK arrives, so a NACK in the response-wait state was dropping whatever was queued behind it and silently skipping that command.

Rather than pass a flag from each call site, processNackFromCommand() now captures mWaitForAck on entry: that is true only in the state that still has the refused instruction queued (the ACK block clears it before setting mWaitForResponse), and it is also true in the streaming data-packet-then-NACK path. The javadoc no longer claims the drop is unconditional. Same fix applied to LiteProtocol.processNackFromCommand().

[marknolan]

Fixed in 648897b. Added the response-wait NACK branch to LiteProtocol, checked before isKnownResponseByte() so it mirrors ShimmerBluetooth.processNotStreamingWaitForResp() exactly. It shares the same processNackFromCommand(), so it also picks up the conditional-drop fix from the other comment.

[marknolan]

All four review items are addressed in 648897ba — the three inline comments have individual replies, and this covers the suppressed one.

Suppressed comment (findOffsetOfNextPacketBoundary using a raw 0): agreed and fixed — it now reads b == DATA_PACKET || b == ACK_COMMAND_PROCESSED || b == NACK_COMMAND_PROCESSED, which is both self-documenting and consistent with bufferTemp[0]==DATA_PACKET in the surrounding code. Worth noting this was the most useful of the four after the transaction-stack bug, despite being the one held back.

Summary of the round:

Item	Outcome
processNackFromCommand() dropping the wrong instruction	Real bug, fixed. The drop is now conditional on mWaitForAck, which is set only in the states that still have the instruction queued. Fixed in LiteProtocol too, and the javadoc corrected.
LiteProtocol missing the response-wait NACK branch	Fixed — added, checked before isKnownResponseByte() to mirror ShimmerBluetooth.
Stale resync inline comment	Fixed.
Raw 0 instead of DATA_PACKET (suppressed)	Fixed.

The PR description has been updated rather than just corrected here, since the first version described the instruction drop as unconditional and a future reader could otherwise "fix" the code back to that.

Still compile-verified only, and still no automated coverage of the receive path — the transaction-stack bug is a good argument for the fake-transport harness noted in the description, since a test would have caught it and review only just did.

[Copilot]

🔵 Needs a closer look

It changes low-level transport state-machine behavior (including streaming parsing) without an automated/hardware-backed test to validate the end-to-end receive/unwind paths.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite