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: do not persist or map values form force-removed CCs, do not persist values from mapped CCs #6760

Merged
merged 7 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
37 changes: 36 additions & 1 deletion packages/zwave-js/src/lib/driver/Driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ import {
serializeCacheValue,
stripUndefined,
timespan,
isEncapsulationCC,
} from "@zwave-js/core";
import type {
NodeSchedulePollOptions,
Expand Down Expand Up @@ -4710,9 +4711,43 @@ ${handlers.length} left`,
}
}

private shouldPersistCCValues(cc: CommandClass): boolean {
// Always persist encapsulation CCs, otherwise interviews don't work.
if (isEncapsulationCC(cc.ccId)) return true;

// Do not persist values for a node or endpoint that does not exist
const endpoint = this.tryGetEndpoint(cc);
const node = endpoint?.getNodeUnsafe();
if (!node) return false;

// Do not persist values for a CC that was force-removed via config
if (endpoint?.wasCCRemovedViaConfig(cc.ccId)) return false;

// Do not persist values for a CC that's being mapped to another endpoint.
// FIXME: This duplicates logic in Node.ts -> handleCommand
const compatConfig = node?.deviceConfig?.compat;
if (
cc.endpointIndex === 0
&& cc.constructor.name.endsWith("Report")
&& node.getEndpointCount() >= 1
// Only map reports from the root device to an endpoint if we know which one
&& compatConfig?.mapRootReportsToEndpoint != undefined
) {
const targetEndpoint = node.getEndpoint(
compatConfig.mapRootReportsToEndpoint,
);
if (targetEndpoint?.supportsCC(cc.ccId)) return false;
}

return true;
}

/** Persists the values contained in a Command Class in the corresponding nodes's value DB */
private persistCCValues(cc: CommandClass) {
cc.persistValues(this);
if (this.shouldPersistCCValues(cc)) {
cc.persistValues(this);
}

if (isEncapsulatingCommandClass(cc)) {
this.persistCCValues(cc.encapsulated);
} else if (isMultiEncapsulatingCommandClass(cc)) {
Expand Down
13 changes: 13 additions & 0 deletions packages/zwave-js/src/lib/node/Endpoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,19 @@ export class Endpoint implements IZWaveEndpoint {
}
}

/** Determines if support for a CC was force-removed via config file */
public wasCCRemovedViaConfig(cc: CommandClasses): boolean {
if (this.supportsCC(cc)) return false;

const compatConfig = this.getNodeUnsafe()?.deviceConfig?.compat;
if (!compatConfig) return false;

const removedEndpoints = compatConfig.removeCCs?.get(cc);
if (!removedEndpoints) return false;

return removedEndpoints == "*" || removedEndpoints.includes(this.index);
}

/**
* Retrieves the version of the given CommandClass this endpoint implements.
* Returns 0 if the CC is not supported.
Expand Down
23 changes: 17 additions & 6 deletions packages/zwave-js/src/lib/node/Node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3042,6 +3042,21 @@ protocol version: ${this.protocolVersion}`;
this.markAsAwake();
}

// If the received CC was force-removed via config file, ignore it completely
const endpoint = this.getEndpoint(command.endpointIndex);
if (endpoint?.wasCCRemovedViaConfig(command.ccId)) {
this.driver.controllerLog.logNode(
this.id,
{
endpoint: endpoint.index,
direction: "inbound",
message:
`Ignoring ${command.constructor.name} because CC support was removed via config file`,
},
);
return;
}

if (command instanceof BasicCC) {
return this.handleBasicCommand(command);
} else if (command instanceof MultilevelSwitchCC) {
Expand Down Expand Up @@ -3662,11 +3677,7 @@ protocol version: ${this.protocolVersion}`;
// Since the node sent us a Basic report, we are sure that it is at least supported
// If this is the only supported actuator CC, add it to the support list,
// so the information lands in the network cache
if (!actuatorCCs.some((cc) => sourceEndpoint.supportsCC(cc))) {
sourceEndpoint.addCC(CommandClasses.Basic, {
isControlled: true,
});
}
sourceEndpoint.maybeAddBasicCCAsFallback();
}
} else if (command instanceof BasicCCSet) {
// Treat BasicCCSet as value events if desired
Expand Down Expand Up @@ -3708,7 +3719,7 @@ protocol version: ${this.protocolVersion}`;
),
command.targetValue,
);
// Since the node sent us a Basic command, we are sure that it is at least controlled
// Since the node sent us a Basic Set, we are sure that it is at least controlled
// Add it to the support list, so the information lands in the network cache
if (!sourceEndpoint.controlsCC(CommandClasses.Basic)) {
sourceEndpoint.addCC(CommandClasses.Basic, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ test.beforeEach(async (t) => {
removeAllListeners: () => {},
} as any;

driver.controller.nodes.getOrThrow = (nodeId: number) => {
const node = driver.controller.nodes.get(nodeId);
if (!node) throw new Error(`Node ${nodeId} not found`);
return node;
};

t.context = { driver, serialport };
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,10 @@ integrationTest(
},
);

integrationTest.only(
integrationTest(
"The unresponsive controller recovery does not kick in when it was enabled via config",
{
debug: true,
// debug: true,

additionalDriverOptions: {
attempts: {
Expand Down
Loading