From bcfb1d07c6b09ba1221a6e06bdc733ddc5dc3dce Mon Sep 17 00:00:00 2001 From: Dominic Griesel Date: Thu, 30 Nov 2023 13:28:16 +0100 Subject: [PATCH 1/2] fix: only expose currentValue on devices that use Basic CC for reporting --- maintenance/schemas/deviceClasses.json | 6 +++ packages/cc/src/cc/BasicCC.ts | 5 +- packages/cc/src/cc/index.ts | 2 + packages/config/config/deviceClasses.json | 18 ++++++-- packages/config/src/DeviceClasses.ts | 32 +++++++++++++ packages/zwave-js/src/lib/node/Endpoint.ts | 25 +++++++++- packages/zwave-js/src/lib/node/Node.ts | 7 +++ .../basicCCSupportWhenForbidden.test.ts | 46 +++++++++++++++++++ 8 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts diff --git a/maintenance/schemas/deviceClasses.json b/maintenance/schemas/deviceClasses.json index 23c1a8dd3b1e..0f7d1820d169 100644 --- a/maintenance/schemas/deviceClasses.json +++ b/maintenance/schemas/deviceClasses.json @@ -28,6 +28,9 @@ "controlledCCs": { "$ref": "#/definitions/CCs" }, + "maySupportBasicCC": { + "const": false + }, "specific": { "type": "object", "patternProperties": { @@ -45,6 +48,9 @@ }, "controlledCCs": { "$ref": "#/definitions/CCs" + }, + "maySupportBasicCC": { + "const": false } }, "required": ["label"], diff --git a/packages/cc/src/cc/BasicCC.ts b/packages/cc/src/cc/BasicCC.ts index f3b01217c89c..484c815aea9d 100644 --- a/packages/cc/src/cc/BasicCC.ts +++ b/packages/cc/src/cc/BasicCC.ts @@ -327,12 +327,15 @@ remaining duration: ${basicResponse.duration?.toString() ?? "undefined"}`; ret.push(...super.getDefinedValueIDs(applHost)); } - // Add the compat event value if it should be exposed if ( !!applHost.getDeviceConfig?.(endpoint.nodeId)?.compat ?.treatBasicSetAsEvent ) { + // Add the compat event value if it should be exposed ret.push(BasicCCValues.compatEvent.endpoint(endpoint.index)); + } else if (endpoint.controlsCC(CommandClasses.Basic)) { + // Otherwise, only expose currentValue on devices that only control Basic CC + ret.push(BasicCCValues.currentValue.endpoint(endpoint.index)); } return ret; diff --git a/packages/cc/src/cc/index.ts b/packages/cc/src/cc/index.ts index 709b3f9fce7c..05d329ac5f5f 100644 --- a/packages/cc/src/cc/index.ts +++ b/packages/cc/src/cc/index.ts @@ -444,6 +444,8 @@ export type { MultilevelSensorCCGetOptions, MultilevelSensorCCGetSupportedScaleOptions, MultilevelSensorCCReportOptions, + MultilevelSensorCCSupportedScaleReportOptions, + MultilevelSensorCCSupportedSensorReportOptions, } from "./MultilevelSensorCC"; export { MultilevelSensorCC, diff --git a/packages/config/config/deviceClasses.json b/packages/config/config/deviceClasses.json index e0aaae39e5d4..85f812a8307b 100644 --- a/packages/config/config/deviceClasses.json +++ b/packages/config/config/deviceClasses.json @@ -12,6 +12,7 @@ "0x01": { "label": "Remote Controller", "controlledCCs": ["Basic"], + "maySupportBasicCC": false, "specific": { "0x01": { "label": "Portable Remote Controller", @@ -164,6 +165,7 @@ "0x07": { "label": "Gateway", "zwavePlusDeviceType": "Gateway", + "maySupportBasicCC": false, "supportedCCs": [ "Manufacturer Specific", // "Security", (some devices only support S2) @@ -252,7 +254,8 @@ "specific": { "0x01": { "label": "Notification Sensor", - "zwavePlusDeviceType": "Sensor - Notification" + "zwavePlusDeviceType": "Sensor - Notification", + "maySupportBasicCC": false } } }, @@ -327,11 +330,13 @@ "specific": { "0x01": { "label": "Repeater Slave", - "zwavePlusDeviceType": "Repeater" + "zwavePlusDeviceType": "Repeater", + "maySupportBasicCC": false }, "0x03": { "label": "IR Repeater", "zwavePlusDeviceType": "IR Repeater", + "maySupportBasicCC": false, "supportedCCs": [ "Association", // V2+ "Association Group Information", @@ -624,7 +629,8 @@ "specific": { "0x01": { "label": "Basic Wall Controller", - "zwavePlusDeviceType": "Wall Controller" + "zwavePlusDeviceType": "Wall Controller", + "maySupportBasicCC": false } } }, @@ -643,7 +649,8 @@ "specific": { "0x01": { "label": "Routing Multilevel Sensor", - "zwavePlusDeviceType": "Sensor - Multilevel" + "zwavePlusDeviceType": "Sensor - Multilevel", + "maySupportBasicCC": false } } }, @@ -653,7 +660,7 @@ }, "0x31": { "label": "Meter", - "supportedCCs": ["Basic"], + "maySupportBasicCC": false, "specific": { "0x01": { "label": "Simple Meter", @@ -843,6 +850,7 @@ "label": "Secure Keypad", "zwavePlusDeviceType": "Entry Control Keypad", "requiresSecurity": true, + "maySupportBasicCC": false, "supportedCCs": [ // "Device Reset Locally", (we can't know if the device can be reset) "Entry Control", diff --git a/packages/config/src/DeviceClasses.ts b/packages/config/src/DeviceClasses.ts index afd851d7af90..004324a93970 100644 --- a/packages/config/src/DeviceClasses.ts +++ b/packages/config/src/DeviceClasses.ts @@ -152,6 +152,21 @@ export class GenericDeviceClass { this.controlledCCs = []; } + if (definition.maySupportBasicCC != undefined) { + if (definition.maySupportBasicCC !== false) { + throwInvalidConfig( + "device classes", + `maySupportBasicCC in device class ${this.label} (${ + num2hex(this.key) + }) must be false or omitted (= true)!`, + ); + } else { + this.maySupportBasicCC = false; + } + } else { + this.maySupportBasicCC = true; + } + const specific = new Map(); if (isObject(definition.specific)) { for ( @@ -190,6 +205,7 @@ export class GenericDeviceClass { public readonly requiresSecurity?: boolean; public readonly supportedCCs: readonly CommandClasses[]; public readonly controlledCCs: readonly CommandClasses[]; + public readonly maySupportBasicCC: boolean; public readonly specific: ReadonlyMap; } @@ -312,6 +328,21 @@ export class SpecificDeviceClass { ...generic.controlledCCs, ...this.controlledCCs, ]); + + if (definition.maySupportBasicCC != undefined) { + if (definition.maySupportBasicCC !== false) { + throwInvalidConfig( + "device classes", + `maySupportBasicCC in device class ${generic.label} -> ${this.label} (${ + num2hex(this.key) + }) must be false or omitted (= true)!`, + ); + } else { + this.maySupportBasicCC = false; + } + } else { + this.maySupportBasicCC = true; + } } public readonly key: number; @@ -320,4 +351,5 @@ export class SpecificDeviceClass { public readonly requiresSecurity?: boolean; public readonly supportedCCs: readonly CommandClasses[]; public readonly controlledCCs: readonly CommandClasses[]; + public readonly maySupportBasicCC: boolean; } diff --git a/packages/zwave-js/src/lib/node/Endpoint.ts b/packages/zwave-js/src/lib/node/Endpoint.ts index 6aaa6f75f7c3..ddacfecdc171 100644 --- a/packages/zwave-js/src/lib/node/Endpoint.ts +++ b/packages/zwave-js/src/lib/node/Endpoint.ts @@ -226,16 +226,39 @@ export class Endpoint implements IZWaveEndpoint { return !!this._implementedCommandClasses.get(cc)?.isControlled; } + /** Checks if this device type is allowed to support Basic CC per the specification */ + public maySupportBasicCC(): boolean { + return this.deviceClass?.specific.maySupportBasicCC + ?? this.deviceClass?.generic.maySupportBasicCC + ?? true; + } + /** Adds Basic CC to the supported CCs if no other actuator CCs are supported */ public maybeAddBasicCCAsFallback(): void { if ( !this.supportsCC(CommandClasses.Basic) + && this.maySupportBasicCC() && !actuatorCCs.some((cc) => this.supportsCC(cc)) ) { this.addCC(CommandClasses.Basic, { isSupported: true }); } } + /** Removes the BasicCC from the supported CCs if the device type forbids it */ + public removeBasicCCSupportIfForbidden(): void { + if ( + this.supportsCC(CommandClasses.Basic) + && !this.maySupportBasicCC() + ) { + // We assume that the device reports support for this CC in error, and that it actually controls it. + // TODO: Consider if we should check additional sources, like the issued commands in AGI CC + this.addCC(CommandClasses.Basic, { + isSupported: false, + isControlled: true, + }); + } + } + /** Removes the BasicCC from the supported CCs if any other actuator CCs are supported */ public hideBasicCCInFavorOfActuatorCCs(): void { // This behavior is defined in SDS14223 @@ -246,7 +269,7 @@ export class Endpoint implements IZWaveEndpoint { // Mark the CC as not supported, but remember if it is controlled this.addCC(CommandClasses.Basic, { isSupported: false }); - // If the record is now only a dummy, remove the CC + // If the record is now only a dummy, remove the CC entirely if ( !this.supportsCC(CommandClasses.Basic) && !this.controlsCC(CommandClasses.Basic) diff --git a/packages/zwave-js/src/lib/node/Node.ts b/packages/zwave-js/src/lib/node/Node.ts index d9a12cf74ba8..c292764561d0 100644 --- a/packages/zwave-js/src/lib/node/Node.ts +++ b/packages/zwave-js/src/lib/node/Node.ts @@ -1816,6 +1816,13 @@ export class ZWaveNode extends Endpoint if (this.interviewStage === InterviewStage.NodeInfo) { // Only advance the interview if it was completed, otherwise abort if (await this.interviewCCs()) { + // After interviewing the CCs, we may need to clean up the Basic CC values. + // Some device types are not allowed to support it, but there are devices that do. + // If a device type is forbidden to support Basic CC, remove the "support" portion of it + for (const endpoint of this.getAllEndpoints()) { + endpoint.removeBasicCCSupportIfForbidden(); + } + this.setInterviewStage(InterviewStage.CommandClasses); } else { return false; diff --git a/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts b/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts new file mode 100644 index 000000000000..5f549a543a3d --- /dev/null +++ b/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts @@ -0,0 +1,46 @@ +import { BasicCCValues } from "@zwave-js/cc"; +import { CommandClasses } from "@zwave-js/core"; +import { integrationTest } from "../integrationTestSuite"; + +integrationTest( + "On devices that MUST not support Basic CC, but use Basic Set to report status, ONLY currentValue should be exposed", + { + debug: true, + + nodeCapabilities: { + // Routing Multilevel Sensor, MUST not support Basic CC + genericDeviceClass: 0x21, + specificDeviceClass: 0x01, + commandClasses: [ + CommandClasses.Version, + // But it reports support if asked + CommandClasses.Basic, + ], + }, + + async testBody(t, driver, node, mockController, mockNode) { + const valueIDs = node.getDefinedValueIDs(); + t.true( + valueIDs.some((v) => BasicCCValues.currentValue.is(v)), + "Did not find Basic CC currentValue although it should be exposed", + ); + t.false( + valueIDs.some((v) => BasicCCValues.targetValue.is(v)), + "Found Basic CC targetValue although it shouldn't be exposed", + ); + t.false( + valueIDs.some((v) => BasicCCValues.duration.is(v)), + "Found Basic CC duration although it shouldn't be exposed", + ); + t.false( + valueIDs.some((v) => BasicCCValues.restorePrevious.is(v)), + "Found Basic CC restorePrevious although it shouldn't be exposed", + ); + + t.false( + valueIDs.some((v) => BasicCCValues.compatEvent.is(v)), + "Found Basic CC compatEvent although it shouldn't be exposed", + ); + }, + }, +); From 98fef879612f99ce6e2624c3b5819cbc745a6efe Mon Sep 17 00:00:00 2001 From: Dominic Griesel Date: Thu, 30 Nov 2023 13:55:02 +0100 Subject: [PATCH 2/2] fix: lint --- .../src/lib/test/compat/basicCCSupportWhenForbidden.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts b/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts index 5f549a543a3d..55fe9ef4043b 100644 --- a/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts +++ b/packages/zwave-js/src/lib/test/compat/basicCCSupportWhenForbidden.test.ts @@ -5,7 +5,7 @@ import { integrationTest } from "../integrationTestSuite"; integrationTest( "On devices that MUST not support Basic CC, but use Basic Set to report status, ONLY currentValue should be exposed", { - debug: true, + // debug: true, nodeCapabilities: { // Routing Multilevel Sensor, MUST not support Basic CC