From dbf10031e44573759e5eb86b7630c010dc93beab Mon Sep 17 00:00:00 2001 From: Dominic Griesel Date: Tue, 31 Oct 2023 11:36:20 +0100 Subject: [PATCH 1/3] feat: allow disabling unresponsive controller recovery --- docs/api/driver.md | 33 ++++-- packages/zwave-js/src/lib/driver/Driver.ts | 47 ++++++-- .../src/lib/driver/Driver.unit.test.ts | 4 +- .../zwave-js/src/lib/driver/ZWaveOptions.ts | 29 +++++ .../sendDataMissingCallbackAbort.test.ts | 112 ++++++++++++++++++ .../lib/test/driver/unresponsiveStick.test.ts | 58 +++++++++ 6 files changed, 267 insertions(+), 16 deletions(-) diff --git a/docs/api/driver.md b/docs/api/driver.md index 142bddb31e94..14bf007bf1e8 100644 --- a/docs/api/driver.md +++ b/docs/api/driver.md @@ -865,15 +865,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: diff --git a/packages/zwave-js/src/lib/driver/Driver.ts b/packages/zwave-js/src/lib/driver/Driver.ts index f2dacdff9704..a53e6be7be5d 100644 --- a/packages/zwave-js/src/lib/driver/Driver.ts +++ b/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 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 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 } // 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 }); } + /** + * 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 { // 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 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 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. diff --git a/packages/zwave-js/src/lib/driver/Driver.unit.test.ts b/packages/zwave-js/src/lib/driver/Driver.unit.test.ts index 19d3494106c5..c75d67da76b6 100644 --- a/packages/zwave-js/src/lib/driver/Driver.unit.test.ts +++ b/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, + }, }); }); diff --git a/packages/zwave-js/src/lib/driver/ZWaveOptions.ts b/packages/zwave-js/src/lib/driver/ZWaveOptions.ts index 5913996fb088..d2817de9215f 100644 --- a/packages/zwave-js/src/lib/driver/ZWaveOptions.ts +++ b/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. diff --git a/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts b/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts index 8c8ba4c2db4a..7b5e8b7e9c89 100644 --- a/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts +++ b/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts @@ -368,6 +368,7 @@ 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", { @@ -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(); + }, + }, +); diff --git a/packages/zwave-js/src/lib/test/driver/unresponsiveStick.test.ts b/packages/zwave-js/src/lib/test/driver/unresponsiveStick.test.ts index 5b7707ca7abd..ad9729777de1 100644 --- a/packages/zwave-js/src/lib/test/driver/unresponsiveStick.test.ts +++ b/packages/zwave-js/src/lib/test/driver/unresponsiveStick.test.ts @@ -1,4 +1,5 @@ import { ZWaveErrorCodes, assertZWaveError } from "@zwave-js/core"; +import { FunctionType } from "@zwave-js/serial"; import { type MockControllerBehavior } from "@zwave-js/testing"; import { wait } from "alcalzone-shared/async"; import Sinon from "sinon"; @@ -126,3 +127,60 @@ integrationTest( }, }, ); + +integrationTest.only( + "The unresponsive controller recovery does not kick in when it was enabled via config", + { + debug: true, + + additionalDriverOptions: { + attempts: { + controller: 1, + }, + features: { + unresponsiveControllerRecovery: false, + }, + }, + + async customSetup(driver, mockController, mockNode) { + const doNotRespond: MockControllerBehavior = { + onHostMessage(host, controller, msg) { + if (!shouldRespond) { + return true; + } + + return false; + }, + }; + mockController.defineBehavior(doNotRespond); + }, + + async testBody(t, driver, node, mockController, mockNode) { + shouldRespond = false; + mockController.autoAckHostMessages = false; + + // The command fails + await assertZWaveError( + t, + () => + driver.sendMessage( + new GetControllerIdRequest(driver), + { supportCheck: false }, + ), + { + errorCode: ZWaveErrorCodes.Controller_Timeout, + context: "ACK", + }, + ); + + await wait(500); + + // And the controller does not get soft-reset + t.throws(() => + mockController.assertReceivedHostMessage((msg) => + msg.functionType === FunctionType.SoftReset + ) + ); + }, + }, +); From 9d505db0d87d2dc8ff4313247f25065cae75dfd4 Mon Sep 17 00:00:00 2001 From: Dominic Griesel Date: Tue, 31 Oct 2023 11:42:33 +0100 Subject: [PATCH 2/3] feat: add driver preset --- docs/api/driver.md | 11 ++++++----- packages/zwave-js/src/lib/driver/ZWaveOptions.ts | 10 ++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/docs/api/driver.md b/docs/api/driver.md index 14bf007bf1e8..0975d8602be1 100644 --- a/docs/api/driver.md +++ b/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: diff --git a/packages/zwave-js/src/lib/driver/ZWaveOptions.ts b/packages/zwave-js/src/lib/driver/ZWaveOptions.ts index d2817de9215f..968123f7cbc3 100644 --- a/packages/zwave-js/src/lib/driver/ZWaveOptions.ts +++ b/packages/zwave-js/src/lib/driver/ZWaveOptions.ts @@ -382,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. */ From abcf9d6ffc936ac054981a415b980b8c69de164c Mon Sep 17 00:00:00 2001 From: Dominic Griesel Date: Tue, 31 Oct 2023 12:06:25 +0100 Subject: [PATCH 3/3] fix: test title --- .../src/lib/test/driver/sendDataMissingCallbackAbort.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts b/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts index 7b5e8b7e9c89..9bf5e20ad576 100644 --- a/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts +++ b/packages/zwave-js/src/lib/test/driver/sendDataMissingCallbackAbort.test.ts @@ -370,7 +370,7 @@ 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,