feat: allow disabling the unresponsive controller recovery feature, d…

…on't recover while initializing, move `enableSoftReset` to `features` (#6480)

docs/api/driver.md

@@ -17,11 +17,12 @@ For most scenarios the default configuration should be sufficient. For more cont
 
 Some curated presets are included in the library:
 
-| Preset         | Description                                                                                                                                                                                                                 |
-| -------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `SAFE_MODE`    | Increases several timeouts to be able to deal with controllers and/or nodes that have severe trouble communicating. This should not be enabled permanently, as it can decrease the performance of the network significantly |
-| `BATTERY_SAVE` | Sends battery powered nodes to sleep more quickly in order to save battery.                                                                                                                                                 |
-| `AWAKE_LONGER` | Sends battery powered nodes to sleep less quickly to give applications more time between interactions.                                                                                                                      |
+| Preset                   | Description                                                                                                                                                                                                                                                                                       |
+| ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `SAFE_MODE`              | Increases several timeouts to be able to deal with controllers and/or nodes that have severe trouble communicating. This should not be enabled permanently, as it can decrease the performance of the network significantly                                                                       |
+| `NO_CONTROLLER_RECOVERY` | Disables the unresponsive controller recovery to be able to deal with controllers that frequently become unresponsive for seemingly no reason. This is meant as a last resort for unstable 500 series controllers, but will result in failed commands and nodes that randomly get marked as dead. |
+| `BATTERY_SAVE`           | Sends battery powered nodes to sleep more quickly in order to save battery.                                                                                                                                                                                                                       |
+| `AWAKE_LONGER`           | Sends battery powered nodes to sleep less quickly to give applications more time between interactions.                                                                                                                                                                                            |
 
 These can be used like this:
 
@@ -865,15 +866,32 @@ interface ZWaveOptions extends ZWaveHostOptions {
 	 */
 	emitValueUpdateAfterSetValue?: boolean;
 
-	/**
-	 * Soft Reset is required after some commands like changing the RF region or restoring an NVM backup.
-	 * Because it may be problematic in certain environments, we provide the user with an option to opt out.
-	 * Default: `true,` except when ZWAVEJS_DISABLE_SOFT_RESET env variable is set.
-	 *
-	 * **Note:** This option has no effect on 700+ series controllers. For those, soft reset is always enabled.
-	 */
-	enableSoftReset?: boolean;
+	features: {
+		/**
+		 * Soft Reset is required after some commands like changing the RF region or restoring an NVM backup.
+		 * Because it may be problematic in certain environments, we provide the user with an option to opt out.
+		 * Default: `true,` except when ZWAVEJS_DISABLE_SOFT_RESET env variable is set.
+		 *
+		 * **Note:** This option has no effect on 700+ series controllers. For those, soft reset is always enabled.
+		 */
+		softReset?: boolean;
 
+		/**
+		 * When enabled, the driver attempts to detect when the controller becomes unresponsive (meaning it did not
+		 * respond within the configured timeout) and performs appropriate recovery actions.
+		 *
+		 * This includes the following scenarios:
+		 * * A command was not acknowledged by the controller
+		 * * The callback for a Send Data command was not received, even after aborting a timed out transmission
+		 *
+		 * In certain environments however, this feature can interfere with the normal operation more than intended,
+		 * so it can be disabled. However disabling it means that commands can fail unnecessarily and nodes can be
+		 * incorrectly marked as dead.
+		 *
+		 * Default: `true`, except when the ZWAVEJS_DISABLE_UNRESPONSIVE_CONTROLLER_RECOVERY env variable is set.
+		 */
+		unresponsiveControllerRecovery?: boolean;
+	};
 	preferences: {
 		/**
 		 * The preferred scales to use when querying sensors. The key is either:

packages/zwave-js/src/lib/driver/Driver.ts

@@ -271,8 +271,13 @@ const defaultOptions: ZWaveOptions = {
 		nodeInterview: 5,
 	},
 	disableOptimisticValueUpdate: false,
-	// By default enable soft reset unless the env variable is set
-	enableSoftReset: !process.env.ZWAVEJS_DISABLE_SOFT_RESET,
+	features: {
+		// By default enable soft reset unless the env variable is set
+		softReset: !process.env.ZWAVEJS_DISABLE_SOFT_RESET,
+		// By default enable the unresponsive controller recovery unless the env variable is set
+		unresponsiveControllerRecovery: !process.env
+			.ZWAVEJS_DISABLE_UNRESPONSIVE_CONTROLLER_RECOVERY,
+	},
 	interview: {
 		queryAllUserCodes: false,
 	},
@@ -568,6 +573,18 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 			mergedOptions,
 			cloneDeep(defaultOptions),
 		) as ZWaveOptions;
+
+		// Normalize deprecated options
+		// TODO: Remove test in packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts
+		// when the deprecated option is removed
+		if (
+			// eslint-disable-next-line deprecation/deprecation
+			this._options.enableSoftReset === false
+			&& this._options.features.softReset
+		) {
+			this._options.features.softReset = false;
+		}
+
 		// And make sure they contain valid values
 		checkOptions(this._options);
 		if (this._options.userAgent) {
@@ -1416,12 +1433,12 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 			await this.initNetworkCache(this.controller.homeId!);
 
 			const maySoftReset = this.maySoftReset();
-			if (this._options.enableSoftReset && !maySoftReset) {
+			if (this._options.features.softReset && !maySoftReset) {
 				this.driverLog.print(
 					`Soft reset is enabled through config, but this stick does not support it.`,
 					"warn",
 				);
-				this._options.enableSoftReset = false;
+				this._options.features.softReset = false;
 			}
 
 			if (maySoftReset) {
@@ -2486,7 +2503,7 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 		}
 
 		// No clear indication, make the result depend on the config option
-		return !!this._options.enableSoftReset;
+		return !!this._options.features.softReset;
 	}
 
 	/**
@@ -2618,6 +2635,18 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 		});
 	}
 
+	/**
+	 * Checks whether recovering an unresponsive controller is enabled
+	 * and whether the driver is in a state where it makes sense.
+	 */
+	private mayRecoverUnresponsiveController(): boolean {
+		if (!this._options.features.unresponsiveControllerRecovery) {
+			return false;
+		}
+		// Only recover after we know the controller has been responsive
+		return this._controllerInterviewed;
+	}
+
 	private async ensureSerialAPI(): Promise<boolean> {
 		// Wait 1.5 seconds after reset to ensure that the module is ready for communication again
 		// Z-Wave 700 sticks are relatively fast, so we also wait for the Serial API started command
@@ -3528,7 +3557,9 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 			context: "ACK";
 		},
 	): boolean {
-		if (!this._controller) return false;
+		if (!this._controller || !this.mayRecoverUnresponsiveController()) {
+			return false;
+		}
 
 		const recoverByReopeningSerialport = async () => {
 			if (!this.serial) return;
@@ -3613,7 +3644,9 @@ export class Driver extends TypedEventEmitter<DriverEventCallbacks>
 			context: "callback" | "response";
 		},
 	): boolean {
-		if (!this._controller) return false;
+		if (!this._controller || !this.mayRecoverUnresponsiveController()) {
+			return false;
+		}
 
 		if (
 			// The SendData response can time out on older controllers trying to reach a dead node.

packages/zwave-js/src/lib/driver/Driver.unit.test.ts

@@ -232,7 +232,9 @@ test("merging multiple sets of options should work", (t) => {
 			sendDataCallback: 33000,
 		},
 		// This comes from the defaults:
-		enableSoftReset: true,
+		features: {
+			softReset: true,
+		},
 	});
 });
 

packages/zwave-js/src/lib/driver/ZWaveOptions.ts

@@ -184,7 +184,36 @@ export interface ZWaveOptions extends ZWaveHostOptions {
 	 */
 	emitValueUpdateAfterSetValue?: boolean;
 
+	features: {
+		/**
+		 * Soft Reset is required after some commands like changing the RF region or restoring an NVM backup.
+		 * Because it may be problematic in certain environments, we provide the user with an option to opt out.
+		 * Default: `true,` except when ZWAVEJS_DISABLE_SOFT_RESET env variable is set.
+		 *
+		 * **Note:** This option has no effect on 700+ series controllers. For those, soft reset is always enabled.
+		 */
+		softReset?: boolean;
+
+		/**
+		 * When enabled, the driver attempts to detect when the controller becomes unresponsive (meaning it did not
+		 * respond within the configured timeout) and performs appropriate recovery actions.
+		 *
+		 * This includes the following scenarios:
+		 * * A command was not acknowledged by the controller
+		 * * The callback for a Send Data command was not received, even after aborting a timed out transmission
+		 *
+		 * In certain environments however, this feature can interfere with the normal operation more than intended,
+		 * so it can be disabled. However disabling it means that commands can fail unnecessarily and nodes can be
+		 * incorrectly marked as dead.
+		 *
+		 * Default: `true`, except when the ZWAVEJS_DISABLE_UNRESPONSIVE_CONTROLLER_RECOVERY env variable is set.
+		 */
+		unresponsiveControllerRecovery?: boolean;
+	};
+
 	/**
+	 * @deprecated Use `features.softReset` instead.
+	 *
 	 * Soft Reset is required after some commands like changing the RF region or restoring an NVM backup.
 	 * Because it may be problematic in certain environments, we provide the user with an option to opt out.
 	 * Default: `true,` except when ZWAVEJS_DISABLE_SOFT_RESET env variable is set.
@@ -353,6 +382,16 @@ export const driverPresets = Object.freeze(
 			},
 		},
 
+		/**
+		 * Disables the unresponsive controller recovery to be able to deal with controllers
+		 * that frequently become unresponsive for seemingly no reason.
+		 */
+		NO_CONTROLLER_RECOVERY: {
+			features: {
+				unresponsiveControllerRecovery: false,
+			},
+		},
+
 		/**
 		 * Sends battery powered nodes to sleep more quickly in order to save battery.
 		 */

packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts

@@ -368,8 +368,9 @@ integrationTest(
 	},
 );
 
+// FIXME: Remove this test when the deprecated enableSoftReset option is removed
 integrationTest(
-	"With soft-reset disabled, transmissions do not get stuck after a missing Send Data callback",
+	"With soft-reset disabled, transmissions do not get stuck after a missing Send Data callback (LEGACY DRIVER OPTION)",
 	{
 		// debug: true,
 
@@ -476,3 +477,114 @@ 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: {
+			features: {
+				softReset: 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();
+		},
+	},
+);