fix: avoid blocking the send queue when recovering controller from mi…

…ssed Send Data callback fails (#6473)
zwave-js · Oct 30, 2023 · 525723f · 525723f
1 parent 48ad38f
commit 525723f
Show file tree

Hide file tree

Showing 2 changed files with 147 additions and 22 deletions.
diff --git a/packages/zwave-js/src/lib/driver/Driver.ts b/packages/zwave-js/src/lib/driver/Driver.ts
@@ -3647,34 +3647,50 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 			}
 		} else if (this._controller.status !== ControllerStatus.Unresponsive) {
 			// The controller was responsive before this transaction failed.
-			// Mark it as unresponsive and try to soft-reset it.
-			this.controller.setStatus(
-				ControllerStatus.Unresponsive,
-			);
 
-			this._recoveryPhase = ControllerRecoveryPhase.CallbackTimeout;
+			if (this.maySoftReset()) {
+				// Mark it as unresponsive and try to soft-reset it.
+				this.controller.setStatus(
+					ControllerStatus.Unresponsive,
+				);
 
-			this.driverLog.print(
-				"Controller missed Send Data callback. Attempting to recover...",
-				"warn",
-			);
+				this._recoveryPhase = ControllerRecoveryPhase.CallbackTimeout;
 
-			// Re-queue the transaction.
-			// Its message generator may have finished, so reset that too.
-			transaction.reset();
-			this.queue.add(transaction.clone());
+				this.driverLog.print(
+					"Controller missed Send Data callback. Attempting to recover...",
+					"warn",
+				);
 
-			// Execute the soft-reset asynchronously
-			void this.softReset().then(() => {
-				// The controller responded. It is no longer unresponsive
-				this._controller?.setStatus(ControllerStatus.Ready);
+				// Re-queue the transaction.
+				// Its message generator may have finished, so reset that too.
+				transaction.reset();
+				this.queue.add(transaction.clone());
 
-				this._recoveryPhase =
-					ControllerRecoveryPhase.CallbackTimeoutAfterReset;
-			}).catch(() => {
-				// Soft-reset failed. Just reject the original transaction.
+				// Execute the soft-reset asynchronously
+				void this.softReset().then(() => {
+					// The controller responded. It is no longer unresponsive
+					this._controller?.setStatus(ControllerStatus.Ready);
+
+					this._recoveryPhase =
+						ControllerRecoveryPhase.CallbackTimeoutAfterReset;
+				}).catch(() => {
+					// Soft-reset failed. Just reject the original transaction.
+					this.rejectTransaction(transaction, error);
+
+					this.driverLog.print(
+						"Automatic controller recovery failed. Returning to normal operation and hoping for the best.",
+						"warn",
+					);
+					this._recoveryPhase = ControllerRecoveryPhase.None;
+					this._controller?.setStatus(ControllerStatus.Ready);
+				});
+			} else {
+				this.driverLog.print(
+					"Controller missed Send Data callback. Cannot recover automatically because the soft reset feature is unsupported or disabled.",
+					"warn",
+				);
 				this.rejectTransaction(transaction, error);
-			});
+			}
 
 			return true;
 		} else {

diff --git a/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts b/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts
@@ -367,3 +367,112 @@ integrationTest(
 		},
 	},
 );
+
+integrationTest(
+	"With soft-reset disabled, transmissions do not get stuck after a missing Send Data callback",
+	{
+		// debug: true,
+
+		// provisioningDirectory: path.join(
+		// 	__dirname,
+		// 	"__fixtures/supervision_binary_switch",
+		// ),
+
+		controllerCapabilities: {
+			// Soft-reset cannot be disabled on 700+ series
+			libraryVersion: "Z-Wave 6.84.0",
+		},
+
+		additionalDriverOptions: {
+			enableSoftReset: false,
+			testingHooks: {
+				skipNodeInterview: true,
+			},
+		},
+
+		customSetup: async (driver, mockController, mockNode) => {
+			// This is almost a 1:1 copy of the default behavior, except that the callback never gets sent
+			const handleBrokenSendData: MockControllerBehavior = {
+				async onHostMessage(host, controller, msg) {
+					// If the controller is operating normally, defer to the default behavior
+					if (!shouldTimeOut) return false;
+
+					if (msg instanceof SendDataRequest) {
+						// Check if this command is legal right now
+						const state = controller.state.get(
+							MockControllerStateKeys.CommunicationState,
+						) as MockControllerCommunicationState | undefined;
+						if (
+							state != undefined
+							&& state !== MockControllerCommunicationState.Idle
+						) {
+							throw new Error(
+								"Received SendDataRequest while not idle",
+							);
+						}
+
+						// Put the controller into sending state
+						controller.state.set(
+							MockControllerStateKeys.CommunicationState,
+							MockControllerCommunicationState.Sending,
+						);
+
+						// Notify the host that the message was sent
+						const res = new SendDataResponse(host, {
+							wasSent: true,
+						});
+						await controller.sendToHost(res.serialize());
+
+						return true;
+					} else if (msg instanceof SendDataAbort) {
+						// Put the controller into idle state
+						controller.state.set(
+							MockControllerStateKeys.CommunicationState,
+							MockControllerCommunicationState.Idle,
+						);
+
+						// We only timeout once in this test
+						shouldTimeOut = false;
+
+						return true;
+					}
+				},
+			};
+			mockController.defineBehavior(handleBrokenSendData);
+		},
+		testBody: async (t, driver, node, mockController, mockNode) => {
+			// Circumvent the options validation so the test doesn't take forever
+			driver.options.timeouts.sendDataAbort = 1000;
+			driver.options.timeouts.sendDataCallback = 1500;
+
+			shouldTimeOut = true;
+
+			const firstCommand = node.commandClasses.Basic.set(99).catch((e) =>
+				e.code
+			);
+			const followupCommand = node.commandClasses.Basic.set(0);
+
+			await wait(2500);
+
+			// Transmission should have been aborted
+			mockController.assertReceivedHostMessage(
+				(msg) => msg.functionType === FunctionType.SendDataAbort,
+			);
+			// but the stick should NOT have been soft-reset
+			t.throws(() =>
+				mockController.assertReceivedHostMessage(
+					(msg) => msg.functionType === FunctionType.SoftReset,
+				)
+			);
+			mockController.clearReceivedHostMessages();
+
+			// The first command should be failed
+			t.is(await firstCommand, ZWaveErrorCodes.Controller_Timeout);
+
+			// The followup command should eventually succeed
+			await followupCommand;
+
+			t.pass();
+		},
+	},
+);