From b6842e6e918ada248ffb994a1b894c6f7d79d43c Mon Sep 17 00:00:00 2001 From: Mark Nolan Date: Thu, 3 Sep 2026 14:01:22 +0100 Subject: [PATCH 1/2] DEV-982: handle NACK from a Shimmer3/3R instead of dropping the connection The Shimmer3/Shimmer3R protocol has had a NACK_COMMAND_PROCESSED (0xFE) reply for a long time, but the driver never learned about it. There was no NACK constant anywhere on the Shimmer3 side, and both protocol implementations tested only for ACK with no else branch, so a NACK was read, silently discarded, and the transaction stalled with mWaitForAck still true and the instruction stack still locked. Two seconds later the ACK timer expired and checkForAckOrRespTask called connectionLost(): a refused command was indistinguishable from a dead link, and there was no retry to soften it because NUMBER_OF_TX_RETRIES_LIMIT is 0 and reading the NACK byte itself resets mNumberofTXRetriesCount, making the soft-recovery branch dead code. This is already reachable on shipping firmware - any SET command while the device is sensing, a sync-mode mismatch, a failed SET_DAUGHTER_CARD_MEM, and the BMP180/BMP280 calibration reads on a Shimmer3R all NACK. The existing coping strategy was avoidance rather than handling; see the comment on GET_PRESSURE_CALIBRATION_COEFFICIENTS_COMMAND, which is skipped for a BMP581 specifically because the firmware NACKs it. A refusal now unwinds the transaction and keeps the connection: - ShimmerObject: add NACK_COMMAND_PROCESSED = (byte) 0xFE. - ShimmerBluetooth.processNackFromCommand(): stop the ACK timer, clear mWaitForAck/mWaitForResponse, drop the refused instruction so the stack advances (as both ACK paths do for their own command), complete the transaction and release the stack lock. - Handled in three places: waiting for an ACK while not streaming (the main path), waiting for a response while not streaming, and a data packet followed by a NACK while streaming, which is the SET-while-sensing case. - eventCommandRefused() is the application hook. It is deliberately concrete with an empty default rather than abstract, and not a new ProtocolListener callback, so every existing subclass and listener implementation keeps compiling. Override it to surface the refusal. Two supporting changes: - The resync helper accepted only 0x00 and 0xFF as a packet boundary, so a NACK in the buffer could not be skipped past cleanly. It now accepts 0xFE too, and is renamed from findOffsetOfNextZeroOrFF to findOffsetOfNextPacketBoundary to match what it does. - 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. A stray NACK arriving with no command in flight is logged and ignored rather than turned into a refusal, in processBytesAvailableAndInstreamSupported() which runs only when !mWaitForAck && !mWaitForResponse. The TypeScript SDK gates its own NACK framing the same way, so a leaked stream byte cannot fabricate a refusal. LiteProtocol gets the same treatment. 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 rewrites a large generated file committed in two places for one constant. Compile-verified only - there is no test harness for the BT receive path (ShimmerBluetooth is abstract with many abstract I/O methods and no test instantiates it), and no hardware was involved. Co-Authored-By: Claude Opus 5 --- .../bluetooth/ShimmerBluetooth.java | 106 ++++++++++++++++-- .../comms/radioProtocol/LiteProtocol.java | 50 +++++++++ .../shimmerresearch/driver/ShimmerObject.java | 1 + 3 files changed, 149 insertions(+), 8 deletions(-) diff --git a/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java b/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java index 18bfc999c..6f3dd7ebf 100644 --- a/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java +++ b/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java @@ -305,6 +305,7 @@ public enum SHIMMER_FEATURE { aMap.put(DATA_PACKET, new BtCommandDetails(DATA_PACKET, "DATA_PACKET")); aMap.put(ROUTINE_COMMUNICATION, new BtCommandDetails(ROUTINE_COMMUNICATION, "ROUTINE_COMMUNICATION")); aMap.put(ACK_COMMAND_PROCESSED, new BtCommandDetails(ACK_COMMAND_PROCESSED, "ACK_COMMAND_PROCESSED")); + aMap.put(NACK_COMMAND_PROCESSED, new BtCommandDetails(NACK_COMMAND_PROCESSED, "NACK_COMMAND_PROCESSED")); mBtCommandMapOther = Collections.unmodifiableMap(aMap); } @@ -866,6 +867,14 @@ else if(isKnownGetCommand(mCurrentCommand)){ } } + /* The Shimmer refused the command. Without this branch the NACK + * was discarded, mWaitForAck stayed true and the instruction stack + * stayed locked until the ACK timer expired and tore the + * connection down - a refusal was indistinguishable from a dead + * link. */ + else if((byte)byteBuffer[0]==NACK_COMMAND_PROCESSED) { + processNackFromCommand(); + } } } } @@ -877,8 +886,15 @@ private void processNotStreamingWaitForResp() { if(byteBuffer!=null){ setIamAlive(true); + /* A NACK can arrive here rather than in the ACK state if the + * firmware decides it cannot serve a GET only while building the + * response. Checked before isKnownResponse() so it is handled as a + * refusal rather than falling through to the ACK timeout. */ + if((byte)byteBuffer[0]==NACK_COMMAND_PROCESSED){ + processNackFromCommand(); + } //Check to see whether it is a response byte - if(isKnownResponse(byteBuffer[0])){ + else if(isKnownResponse(byteBuffer[0])){ byte responseCommand = byteBuffer[0]; processResponseCommand(responseCommand); @@ -918,7 +934,14 @@ && bytesAvailableToBeRead()) { byteBuffer=readBytes(1); if(byteBuffer!=null){ - if(byteBuffer[0]==ACK_COMMAND_PROCESSED) { + /* Deliberately log-only: this path runs with no command in + * flight (!mWaitForAck && !mWaitForResponse), so there is no + * transaction to fail and a stray 0xFE must not be turned into a + * refusal. The buffer is cleared below either way. */ + if(byteBuffer[0]==NACK_COMMAND_PROCESSED) { + printLogDataForDebugging("NACK received with no command awaiting a reply - ignored"); + } + else if(byteBuffer[0]==ACK_COMMAND_PROCESSED) { printLogDataForDebugging("ACK RECEIVED , Connected State!!"); byteBuffer = readBytes(1, INSTREAM_CMD_RESPONSE); if(byteBuffer!=null && byteBuffer[0]==ACK_COMMAND_PROCESSED){ //an android fix.. not fully investigated (JC) @@ -1014,6 +1037,25 @@ else if(isSupportedInStreamCmds() && bufferTemp[getPacketSizeWithCrc()+2]==INSTR } } + //Data packet followed by a NACK (a command refused while streaming, e.g. + //any SET blocked by the firmware because it is sensing) + else if(bufferTemp[0]==DATA_PACKET + && bufferTemp[getPacketSizeWithCrc()+1]==NACK_COMMAND_PROCESSED){ + + if (mBtCommsCrcModeCurrent != BT_CRC_MODE.OFF && !checkCrc(bufferTemp, getPacketSize() + 1)) { + discardBufferBytesToNextPacket(); + return; + } + + //Handle the data packet first, then fail the command in flight + processDataPacket(bufferTemp); + processNackFromCommand(); + + /* clearBuffers() rather than clearSingleDataPacketFromBuffers(), + * because the NACK is a single byte with no payload behind it and that + * helper would push it back as the next packet header. */ + clearBuffers(); + } //TODO: ACK in bufferTemp[0] not handled //else if else { @@ -1303,13 +1345,14 @@ protected void clearBuffers() { } /** - * Next packet start should begin with DATA_PACKET or ACK_COMMAND_PROCESSED byte so skip to that point + * Next packet start should begin with DATA_PACKET, ACK_COMMAND_PROCESSED or + * NACK_COMMAND_PROCESSED byte so skip to that point */ protected void discardBufferBytesToNextPacket(){ byte[] bTemp = mByteArrayOutputStream.toByteArray(); //Find index of first DATA_PACKET or ACK byte within the buffer - int offset = findOffsetOfNextZeroOrFF(bTemp); + int offset = findOffsetOfNextPacketBoundary(bTemp); //If not found, just skip one byte offset = (offset == -1) ? 1 : offset; @@ -1323,15 +1366,16 @@ protected void discardBufferBytesToNextPacket(){ } /** - * Finds the offset/index of the next DATA_PACKET (0x00) or ACK_COMMAND_PROCESSED (0xFF) byte. + * Finds the offset/index of the next DATA_PACKET (0x00), ACK_COMMAND_PROCESSED + * (0xFF) or NACK_COMMAND_PROCESSED (0xFE) byte. * * @param buffer a byte array to search within - * @return index of the first 0x00 or 0xFF byte found after position 0, or -1 if not found + * @return index of the first 0x00, 0xFF or 0xFE byte found after position 0, or -1 if not found */ - private static int findOffsetOfNextZeroOrFF(byte[] buffer) { + private static int findOffsetOfNextPacketBoundary(byte[] buffer) { for (int i = 1; i < buffer.length; i++) { byte b = buffer[i]; - if (b == 0 || b == (byte) 0xFF) + if (b == 0 || b == ACK_COMMAND_PROCESSED || b == NACK_COMMAND_PROCESSED) return i; } return -1; @@ -1903,6 +1947,52 @@ else if(responseCommand==BT_FW_VERSION_STR_RESPONSE) { } } + /** + * Unwinds the in-flight transaction after the Shimmer answered a command + * with NACK (0xFE) instead of ACK. + * + *

The refused instruction is dropped and the stack advances, exactly as + * it would on an ACK, so one refused command no longer stalls everything + * queued behind it. The connection is left up: a refusal means the device + * declined this command, not that the link is gone. + */ + private void processNackFromCommand() { + stopTimerCheckForAckOrResp(); //cancel the ack timer + printLogDataForDebugging("NACK Received for Command: \t\t" + btCommandToString(mCurrentCommand)); + + byte refusedCommand = mCurrentCommand; + + mWaitForAck=false; + mWaitForResponse=false; + + //Drop the refused instruction, as both ACK paths do for their command + if(getListofInstructions().size()>0){ + removeInstruction(0); + } + removeAllNulls(); + + mTransactionCompleted=true; + setInstructionStackLock(false); + + eventCommandRefused(refusedCommand); + } + + /** + * Called when the Shimmer refused a command with a NACK. The transaction has + * already been unwound and the connection is still up. + * + *

Deliberately concrete rather than abstract so that existing subclasses + * keep compiling; override to surface the refusal to the application. A + * refusal is expected in normal use - most SET commands are rejected while + * the device is sensing, and some GETs are rejected for hardware that does + * not have the feature. + * + * @param command the command byte that was refused + */ + protected void eventCommandRefused(byte command) { + //Default: no application-level notification, the log line above stands + } + // TODO: Consider removing this, replace by SET then GET and let the // RESPONSE update the variables in ShimmerObject - removes duplication of // code and ensure ShimmerObject it up-to-date with exactly what is on diff --git a/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java b/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java index dc1634651..63ac6f93a 100644 --- a/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java +++ b/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java @@ -126,6 +126,12 @@ public class InstructionsGet{ protected String mDirectoryName; protected int numBytesToReadFromExpBoard=0; private static final int MAX_CALIB_DUMP_MAX = 4096; + /* The generated LiteProtocolInstructionSet has no NACK entry, so the value + * is declared here rather than read from InstructionsSet. Adding it to + * grpcprotosrc/src/LiteProtocolInstructionSet.proto and regenerating would + * be tidier, but that rewrites a large generated file committed in two + * places for the sake of one constant. */ + private static final int NACK_COMMAND_PROCESSED_VALUE = 0xFE; /** @@ -606,6 +612,12 @@ else if(isKnownGetCommand(mCurrentCommand)){ } } + /* The Shimmer refused the command. Without this branch the NACK + * was discarded and the transaction stalled until the ACK timer + * expired, which is treated as a lost connection. */ + else if((((int)byteBuffer[0])&0xFF)==NACK_COMMAND_PROCESSED_VALUE) { + processNackFromCommand(); + } } } } @@ -1295,6 +1307,44 @@ private int availableBytes() throws ShimmerException { public void eventLogAndStreamStatusChanged(int currentCommand){ mProtocolListener.eventLogAndStreamStatusChangedCallback(currentCommand); } + + /** + * Unwinds the in-flight transaction after the Shimmer answered a command + * with NACK (0xFE) instead of ACK. The refused instruction is dropped so the + * stack advances, and the connection is left up - a refusal means the device + * declined this command, not that the link is gone. + */ + private void processNackFromCommand() { + stopTimerCheckForAckOrResp(); //cancel the ack timer + printLogDataForDebugging("NACK Received for Command: \t\t" + btCommandToString(mCurrentCommand)); + + int refusedCommand = ((int)mCurrentCommand)&0xFF; + + mWaitForAck=false; + mWaitForResponse=false; + + if(getListofInstructions().size()>0){ + getListofInstructions().remove(0); + } + getListofInstructions().removeAll(Collections.singleton(null)); + + mTransactionCompleted=true; + setInstructionStackLock(false); + + eventCommandRefused(refusedCommand); + } + + /** + * Called when the Shimmer refused a command with a NACK. The transaction has + * already been unwound and the connection is still up. Concrete rather than + * a new ProtocolListener callback so that existing implementers keep + * compiling; override to surface the refusal to the application. + * + * @param command the command value that was refused + */ + protected void eventCommandRefused(int command) { + //Default: no application-level notification, the log line above stands + } private void isNowStreaming() { mProtocolListener.isNowStreaming(); diff --git a/ShimmerDriver/src/main/java/com/shimmerresearch/driver/ShimmerObject.java b/ShimmerDriver/src/main/java/com/shimmerresearch/driver/ShimmerObject.java index 4a6808c69..b2a435524 100644 --- a/ShimmerDriver/src/main/java/com/shimmerresearch/driver/ShimmerObject.java +++ b/ShimmerDriver/src/main/java/com/shimmerresearch/driver/ShimmerObject.java @@ -457,6 +457,7 @@ public class BTStream { public static final byte GET_RWC_COMMAND = (byte) 0x91; public static final byte ROUTINE_COMMUNICATION = (byte) 0xE0; + public static final byte NACK_COMMAND_PROCESSED = (byte) 0xFE; public static final byte ACK_COMMAND_PROCESSED = (byte) 0xFF; public static final byte START_LOGGING_ONLY_COMMAND = (byte) 0x92; From 648897ba72a514a177f763cff11ef8deb94fde49 Mon Sep 17 00:00:00 2001 From: Mark Nolan Date: Thu, 3 Sep 2026 14:47:08 +0100 Subject: [PATCH 2/2] DEV-982: only drop the queued instruction when it is still queued (Copilot review) processNackFromCommand() removed instruction 0 unconditionally, but a GET's instruction is already removed when its ACK arrives. A NACK arriving in the response-wait state therefore dropped whatever was queued behind it and silently skipped that command. It is now dropped only when mWaitForAck was still set, which is exactly the state that still has it queued. Same fix in LiteProtocol, and the javadoc no longer claims the drop is unconditional. Also from the review: - LiteProtocol was missing the response-wait NACK branch that ShimmerBluetooth has, so a NACK there would still have fallen through to the timeout. Added, checked before isKnownResponseByte() to match. - The resync helper's inline comment still said it looked for DATA_PACKET or ACK only. - The helper used a raw 0 rather than the DATA_PACKET constant it now sits alongside (suppressed review comment). Co-Authored-By: Claude Opus 5 --- .../bluetooth/ShimmerBluetooth.java | 24 ++++++++++++------- .../comms/radioProtocol/LiteProtocol.java | 17 +++++++++++-- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java b/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java index 6f3dd7ebf..55132210e 100644 --- a/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java +++ b/ShimmerDriver/src/main/java/com/shimmerresearch/bluetooth/ShimmerBluetooth.java @@ -1351,7 +1351,7 @@ protected void clearBuffers() { protected void discardBufferBytesToNextPacket(){ byte[] bTemp = mByteArrayOutputStream.toByteArray(); - //Find index of first DATA_PACKET or ACK byte within the buffer + //Find index of first DATA_PACKET, ACK or NACK byte within the buffer int offset = findOffsetOfNextPacketBoundary(bTemp); //If not found, just skip one byte offset = (offset == -1) ? 1 : offset; @@ -1375,7 +1375,7 @@ protected void discardBufferBytesToNextPacket(){ private static int findOffsetOfNextPacketBoundary(byte[] buffer) { for (int i = 1; i < buffer.length; i++) { byte b = buffer[i]; - if (b == 0 || b == ACK_COMMAND_PROCESSED || b == NACK_COMMAND_PROCESSED) + if (b == DATA_PACKET || b == ACK_COMMAND_PROCESSED || b == NACK_COMMAND_PROCESSED) return i; } return -1; @@ -1951,10 +1951,12 @@ else if(responseCommand==BT_FW_VERSION_STR_RESPONSE) { * Unwinds the in-flight transaction after the Shimmer answered a command * with NACK (0xFE) instead of ACK. * - *

The refused instruction is dropped and the stack advances, exactly as - * it would on an ACK, so one refused command no longer stalls everything - * queued behind it. The connection is left up: a refusal means the device - * declined this command, not that the link is gone. + *

The refused instruction is dropped and the stack advances, as it would + * on an ACK, so one refused command no longer stalls everything queued + * behind it. It is dropped only when it is still queued - a GET's + * instruction is already gone by the time its response is awaited. The + * connection is left up: a refusal means the device declined this command, + * not that the link is gone. */ private void processNackFromCommand() { stopTimerCheckForAckOrResp(); //cancel the ack timer @@ -1962,11 +1964,17 @@ private void processNackFromCommand() { byte refusedCommand = mCurrentCommand; + /* Only the ACK-wait state still has the refused instruction queued: a + * GET's instruction is removed as soon as its ACK arrives, so removing + * one here as well would drop whatever was queued behind it and silently + * skip that command. */ + boolean instructionStillQueued = mWaitForAck; + mWaitForAck=false; mWaitForResponse=false; - //Drop the refused instruction, as both ACK paths do for their command - if(getListofInstructions().size()>0){ + //Drop the refused instruction, as the ACK path does for its own command + if(instructionStillQueued && getListofInstructions().size()>0){ removeInstruction(0); } removeAllNulls(); diff --git a/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java b/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java index 63ac6f93a..56dddebf3 100644 --- a/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java +++ b/ShimmerDriver/src/main/java/com/shimmerresearch/comms/radioProtocol/LiteProtocol.java @@ -652,8 +652,16 @@ else if(bytesAvailableToBeRead()){ byteBuffer=readBytes(1); mIamAlive = true; + /* As in the ACK-wait path above, and matching ShimmerBluetooth: a + * NACK can land here instead if the firmware only discovers it + * cannot serve a GET while building the response. Checked before + * isKnownResponseByte() so it is handled as a refusal rather than + * falling through to the ACK/response timeout. */ + if((((int)byteBuffer[0])&0xFF)==NACK_COMMAND_PROCESSED_VALUE){ + processNackFromCommand(); + } //Check to see whether it is a response byte - if(isKnownResponseByte(byteBuffer[0])){ + else if(isKnownResponseByte(byteBuffer[0])){ byte responseCommand = byteBuffer[0]; if(mUseShimmerBluetoothApproach){ @@ -1320,10 +1328,15 @@ private void processNackFromCommand() { int refusedCommand = ((int)mCurrentCommand)&0xFF; + /* Only the ACK-wait state still has the refused instruction queued: a + * GET's instruction is removed as soon as its ACK arrives, so removing + * one here as well would drop whatever was queued behind it. */ + boolean instructionStillQueued = mWaitForAck; + mWaitForAck=false; mWaitForResponse=false; - if(getListofInstructions().size()>0){ + if(instructionStillQueued && getListofInstructions().size()>0){ getListofInstructions().remove(0); } getListofInstructions().removeAll(Collections.singleton(null));