Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: implement workaround for some 500 series controllers replying with invalid callbacks in case of SUC route assignment failure #6370

Merged
merged 4 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/config-files/file-format.md
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,12 @@ Several command classes are refreshed regularly (every couple of hours) if they

By default, received `Basic CC::Report` commands are mapped to a more appropriate CC. Setting `disableBasicMapping` to `true` disables this feature.

### `disableCallbackFunctionTypeCheck`

By default, responses or callbacks for Serial API commands must have the same function type (command identifier) in order to be recognized. However, in some situations, certain controllers send a callback with an invalid function type. In this case, the faulty commands may be listed in the `disableCallbackFunctionTypeCheck` array to disable the check for a matching function type.

> [!NOTE] This compat flag requires command-specific support and is not a generic escape hatch.

### `disableStrictEntryControlDataValidation`

The specifications mandate strict rules for the data and sequence numbers in `Entry Control CC Notifications`, which some devices do not follow, causing the notifications to get dropped. Setting `disableStrictEntryControlDataValidation` to `true` disables these strict checks.
Expand Down
8 changes: 8 additions & 0 deletions maintenance/schemas/device-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,14 @@
"disableBasicMapping": {
"const": true
},
"disableCallbackFunctionTypeCheck": {
"type": "array",
"items": {
"type": "integer",
"minimum": 1
},
"minItems": 1
},
"disableStrictEntryControlDataValidation": {
"const": true
},
Expand Down
8 changes: 8 additions & 0 deletions packages/config/config/devices/0x0000/husbzb-1.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,13 @@
"firmwareVersion": {
"min": "0.0",
"max": "255.255"
},
"compat": {
// In some situations, AssignSUCReturnRoute and DeleteSUCReturnRoute get answered with a wrong callback function type,
// triggering Z-Wave JS's unresponsive controller check.
"disableCallbackFunctionTypeCheck": [
81, // AssignSUCReturnRoute
85 // DeleteSUCReturnRoute
]
}
}
8 changes: 8 additions & 0 deletions packages/config/config/devices/0x0086/zw090.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,13 @@
"metadata": {
"reset": "Use this procedure only in the event that the primary controller is missing or otherwise inoperable.\n\nPress and hold the Action Button on Z-Stick for 20 seconds and then release",
"manual": "https://products.z-wavealliance.org/ProductManual/File?folder=&filename=MarketCertificationFiles/1345/Z%20Stick%20Gen5%20manual%201.pdf"
},
"compat": {
// In some situations, AssignSUCReturnRoute and DeleteSUCReturnRoute get answered with a wrong callback function type,
// triggering Z-Wave JS's unresponsive controller check.
"disableCallbackFunctionTypeCheck": [
81, // AssignSUCReturnRoute
85 // DeleteSUCReturnRoute
]
}
}
19 changes: 19 additions & 0 deletions packages/config/src/devices/CompatConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,23 @@ compat option disableBasicMapping must be true or omitted`,
this.disableBasicMapping = definition.disableBasicMapping;
}

if (definition.disableCallbackFunctionTypeCheck != undefined) {
if (
!isArray(definition.disableCallbackFunctionTypeCheck)
|| !definition.disableCallbackFunctionTypeCheck.every(
(d: any) => typeof d === "number" && d % 1 === 0 && d > 0,
)
) {
throwInvalidConfig(
"devices",
`config/devices/${filename}:
when present, compat option disableCallbackFunctionTypeCheck msut be an array of positive integers`,
);
}
this.disableCallbackFunctionTypeCheck =
definition.disableCallbackFunctionTypeCheck;
}

if (definition.disableStrictEntryControlDataValidation != undefined) {
if (definition.disableStrictEntryControlDataValidation !== true) {
throwInvalidConfig(
Expand Down Expand Up @@ -568,6 +585,7 @@ compat option overrideQueries must be an object!`,
public readonly disableBasicMapping?: boolean;
public readonly disableStrictEntryControlDataValidation?: boolean;
public readonly disableStrictMeasurementValidation?: boolean;
public readonly disableCallbackFunctionTypeCheck?: number[];
public readonly enableBasicSetMapping?: boolean;
public readonly forceNotificationIdleReset?: boolean;
public readonly forceSceneControllerGroupCount?: number;
Expand Down Expand Up @@ -608,6 +626,7 @@ compat option overrideQueries must be an object!`,
"removeCCs",
"disableAutoRefresh",
"disableBasicMapping",
"disableCallbackFunctionTypeCheck",
"disableStrictEntryControlDataValidation",
"disableStrictMeasurementValidation",
"enableBasicSetMapping",
Expand Down
20 changes: 13 additions & 7 deletions packages/serial/src/message/Message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export type MessageOptions =
*/
export class Message {
public constructor(
protected host: ZWaveHost,
public readonly host: ZWaveHost,
options: MessageOptions = {},
) {
// decide which implementation we follow
Expand Down Expand Up @@ -355,12 +355,18 @@ export class Message {
/** Checks if a message is an expected callback for this message */
public isExpectedCallback(msg: Message): boolean {
if (msg.type !== MessageType.Request) return false;
// If a received request included a callback id, enforce that the response contains the same
if (
this.hasCallbackId()
&& (!msg.hasCallbackId() || this._callbackId !== msg._callbackId)
) {
return false;

// Some controllers have a bug causing them to send a callback with a function type of 0 and no callback ID
// To prevent this from triggering the unresponsive controller detection we need to forward these messages as if they were correct
if (msg.functionType !== 0 as any) {
// If a received request included a callback id, enforce that the response contains the same
if (
this.hasCallbackId()
&& (!msg.hasCallbackId()
|| this._callbackId !== msg._callbackId)
) {
return false;
}
}

return this.testMessage(msg, this.expectedCallback);
Expand Down
26 changes: 15 additions & 11 deletions packages/testing/src/MockControllerCapabilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,23 +31,27 @@ export interface MockControllerCapabilities {
watchdogEnabled: boolean;
}

export function getDefaultSupportedFunctionTypes(): FunctionType[] {
return [
FunctionType.GetSerialApiInitData,
FunctionType.GetControllerCapabilities,
FunctionType.SendData,
FunctionType.SendDataMulticast,
FunctionType.GetControllerVersion,
FunctionType.GetControllerId,
FunctionType.GetNodeProtocolInfo,
FunctionType.RequestNodeInfo,
FunctionType.AssignSUCReturnRoute,
];
}

export function getDefaultMockControllerCapabilities(): MockControllerCapabilities {
return {
firmwareVersion: "1.0",
manufacturerId: 0xffff,
productType: 0xffff,
productId: 0xfffe,
supportedFunctionTypes: [
FunctionType.GetSerialApiInitData,
FunctionType.GetControllerCapabilities,
FunctionType.SendData,
FunctionType.SendDataMulticast,
FunctionType.GetControllerVersion,
FunctionType.GetControllerId,
FunctionType.GetNodeProtocolInfo,
FunctionType.RequestNodeInfo,
FunctionType.AssignSUCReturnRoute,
],
supportedFunctionTypes: getDefaultSupportedFunctionTypes(),

controllerType: ZWaveLibraryTypes["Static Controller"],
libraryVersion: "Z-Wave 7.17.99",
Expand Down
1 change: 1 addition & 0 deletions packages/testing/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export * from "./CCSpecificCapabilities";
export * from "./MockController";
export { getDefaultSupportedFunctionTypes } from "./MockControllerCapabilities";
export * from "./MockNode";
export { ccCaps } from "./MockNodeCapabilities";
export type { PartialCCCapabilities } from "./MockNodeCapabilities";
Expand Down
34 changes: 26 additions & 8 deletions packages/zwave-js/src/lib/controller/Controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,15 @@ import {
} from "../serialapi/network-mgmt/AssignReturnRouteMessages";
import {
AssignSUCReturnRouteRequest,
type AssignSUCReturnRouteRequestTransmitReport,
AssignSUCReturnRouteRequestTransmitReport,
} from "../serialapi/network-mgmt/AssignSUCReturnRouteMessages";
import {
DeleteReturnRouteRequest,
type DeleteReturnRouteRequestTransmitReport,
} from "../serialapi/network-mgmt/DeleteReturnRouteMessages";
import {
DeleteSUCReturnRouteRequest,
type DeleteSUCReturnRouteRequestTransmitReport,
DeleteSUCReturnRouteRequestTransmitReport,
} from "../serialapi/network-mgmt/DeleteSUCReturnRouteMessages";
import {
GetPriorityRouteRequest,
Expand Down Expand Up @@ -4319,14 +4319,23 @@ ${associatedNodes.join(", ")}`,
await this.deleteSUCReturnRoutes(nodeId);

try {
const result = await this.driver.sendMessage<
AssignSUCReturnRouteRequestTransmitReport
>(
const result = await this.driver.sendMessage(
new AssignSUCReturnRouteRequest(this.driver, {
nodeId,
}),
);

if (
!(result instanceof AssignSUCReturnRouteRequestTransmitReport)
) {
this.driver.controllerLog.logNode(
nodeId,
`Assigning SUC return route failed: Invalid callback received`,
"error",
);
return false;
}

const success = this.handleRouteAssignmentTransmitReport(
result,
nodeId,
Expand Down Expand Up @@ -4492,14 +4501,23 @@ ${associatedNodes.join(", ")}`,
});

try {
const result = await this.driver.sendMessage<
DeleteSUCReturnRouteRequestTransmitReport
>(
const result = await this.driver.sendMessage(
new DeleteSUCReturnRouteRequest(this.driver, {
nodeId,
}),
);

if (
!(result instanceof DeleteSUCReturnRouteRequestTransmitReport)
) {
this.driver.controllerLog.logNode(
nodeId,
`Deleting SUC return route failed: Invalid callback received`,
"error",
);
return false;
}

const success = this.handleRouteAssignmentTransmitReport(
result,
nodeId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,25 @@ export interface AssignSUCReturnRouteRequestOptions extends MessageBaseOptions {
nodeId: number;
}

function testAssignSUCReturnRouteCallback(
sent: AssignSUCReturnRouteRequest,
callback: Message,
): boolean {
// Some controllers have a bug where they incorrectly respond with DeleteSUCReturnRoute
if (
callback.host
.getDeviceConfig?.(callback.host.ownNodeId)
?.compat
?.disableCallbackFunctionTypeCheck
?.includes(FunctionType.AssignSUCReturnRoute)
) {
return true;
}
return callback.functionType === FunctionType.AssignSUCReturnRoute;
}

@expectedResponse(FunctionType.AssignSUCReturnRoute)
@expectedCallback(FunctionType.AssignSUCReturnRoute)
@expectedCallback(testAssignSUCReturnRouteCallback)
export class AssignSUCReturnRouteRequest extends AssignSUCReturnRouteRequestBase
implements INodeQuery
{
Expand Down
Loading