Skip to content

DEV-982: handle NACK from a Shimmer3 instead of leaving the caller suspended - #18

Open
marknolan wants to merge 1 commit into
mainfrom
DEV-982_nack_handling
Open

marknolan wants to merge 1 commit into
mainfrom
DEV-982_nack_handling

Conversation

@marknolan

Copy link
Copy Markdown
Member

The protocol has had a NACK_COMMAND_PROCESSED (0xFE) reply for a long time. This
API never learned about it.

What was happening

Every reply path in startProcessing() keys off ackCommand:

if (self.receivedBytes.first==PacketTypeShimmer.ackCommand.rawValue){

A refusal matched none of them, so the pending CheckedContinuation was never resumed
and the caller's await never returned.

Nothing rescues that. timeoutInSeconds is declared and never used — here and in
VerisenseProtocol — so the suspended task stays suspended for the life of the
process, with the device still apparently connected.

That makes this the worst-behaved of the host APIs that share this defect.
Shimmer-Java-Android-API#293
tore the link down after two seconds,
Shimmer-C-API#204 times out
and disconnects after about ten, and this one simply never comes back. The TypeScript
SDK already handles NACK correctly.

Where it bites

Of the ~14 sendNack sites in the firmware's shimmer_bt_uart.c, only one is the
blocked-while-sensing gate, and sending commands mid-stream is not a supported
operation anyway. The rest are reachable while idle — the sharpest being SD sync:

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 everything else, including the whole connect
sequence, so a sync-enabled Shimmer could never finish connecting — it would just hang.
Also reachable while idle: a sync-mode mismatch, a truncated payload, a zero sampling
clock divider, an out-of-range InfoMem or calibration write, and several commands the
dispatcher accepts but that were never implemented.

What this changes

processNackFromCommand() resumes whichever continuation is outstanding with a failure
value and clears it, so the caller gets a result rather than hanging. The connection is
left up, because a refusal means the device declined this command, not that the link is
gone.

Resuming exactly once matters in both directions: a CheckedContinuation resumed twice
traps at runtime, and one that is never resumed suspends the task for good. Both
continuations are cleared after resuming, matching what every existing branch does.

Two deliberate placement choices:

  • The check sits ahead of the streaming/connected split, rather than inside either,
    because neither branch looks for 0xFE.
  • It waits until the CRC bytes have arrived. The firmware writes the refusal as a
    complete response packet and appends CRC to it exactly as it would to a response, so
    the guard is count >= 1 + CRCMode.rawValue and the whole packet is consumed at once.
    Without that, a CRC byte would be left to be read as the next packet header.

Verification

Not compiled locally, and not unit tested. Two honest caveats:

  • There is no Swift toolchain on the machine this was written on, so the CI run on this
    branch is the first time it is built. Please read the CI log rather than trusting the
    green check alone.
  • No test could cover it in any case: Shimmer3Protocol.init takes a concrete
    BleByteRadio, which needs a live CBPeripheral, so no unit test can construct the
    protocol and none does. Making that initialiser accept ByteCommunication would
    unlock testing for this whole class and is worth doing separately.

Only an existing file is edited, so project.pbxproj is untouched.

🤖 Generated with Claude Code

The protocol has had a NACK (0xFE) reply for a long time; this API never
learned about it. Every reply path in startProcessing() keys off
ackCommand, so a refusal matched nothing, the pending CheckedContinuation
was never resumed, and the caller's await never returned.

Nothing rescues that. timeoutInSeconds is declared and never used, here
and in VerisenseProtocol, so the suspended task stays suspended for the
life of the process. That makes this the worst of the three host APIs
that share the defect: Shimmer-Java-Android-API#293 tore the link down
after two seconds, Shimmer-C-API times out and disconnects after about
ten, and this one simply never comes back.

Found by checking the sibling implementations of the wire format after
the Java fix. The web SDK already handles NACK; C# is being fixed
alongside this.

The firmware refuses far more than it used to. Every command except
SET_SD_SYNC_COMMAND and ACK is refused while SD sync is enabled
(ShimBt_isCmdAllowedWhileSdSyncing), which covers the whole connect
sequence, so a sync-enabled Shimmer could never finish connecting. Also
any SET while sensing, a sync-mode mismatch, an out-of-range InfoMem or
calibration write, and several commands the dispatcher accepts but that
were never implemented.

processNackFromCommand() resumes whichever continuation is outstanding
with a failure value and clears it, so the caller gets a result instead
of hanging. Resuming exactly once matters in both directions: a
CheckedContinuation resumed twice traps at runtime, one never resumed
hangs the task. The connection is left up, because a refusal means the
device declined the command, not that the link is gone.

The check sits ahead of the streaming/connected split rather than inside
either, since neither branch looks for 0xFE, and it waits until the CRC
bytes the firmware appends to the refusal have arrived so the whole
packet is consumed at once - otherwise a CRC byte would be read as the
next packet header.

Not compiled locally: there is no Swift toolchain on the machine this was
written on, so the CI run on this branch is the first time it is built.
No unit test either - Shimmer3Protocol.init takes a concrete BleByteRadio
that needs a live CBPeripheral, so no test can construct the protocol,
which is a limitation worth lifting separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant