Skip to content

Commit

Permalink
fix: Fix wrong attribute names when device manufacturer ID is missing…
Browse files Browse the repository at this point in the history 
… from message (#787)

* fix: AttributeReport: Erroneous attribute names

Remap cluster attributes names using the target device's manufacturer ID,
as certain devices fail to set the manufacturerSpecific and customerCode
data in the ZCL Frame header.

* fix: Apply fix to all commands

* fix: Made remapping of attributes mandatory

* zclFrameConverter: deviceManufacturerID is now mandatory
* Updated calls to ZclFrameConverter due to the method signature change

* test: Added new device (Legrand) to test suite

* test: Added new tests to validate PR#787
  • Loading branch information
@FabianMangold
FabianMangold authored Nov 3, 2023
1 parent fd5bf6c commit d8e4b5b
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 14 deletions.
8 changes: 4 additions & 4 deletions src/controller/controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,18 +683,18 @@ class Controller extends events.EventEmitter {
if (frame.isGlobal()) {
if (frame.isCommand('report')) {
type = 'attributeReport';
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame);
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame, device.manufacturerID);
} else if (frame.isCommand('read')) {
type = 'read';
data = ZclFrameConverter.attributeList(dataPayload.frame);
data = ZclFrameConverter.attributeList(dataPayload.frame, device.manufacturerID);
} else if (frame.isCommand('write')) {
type = 'write';
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame);
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame, device.manufacturerID);
} else {
/* istanbul ignore else */
if (frame.isCommand('readRsp')) {
type = 'readResponse';
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame);
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame, device.manufacturerID);
}
}
} else {
Expand Down
29 changes: 22 additions & 7 deletions src/controller/helpers/zclFrameConverter.ts
View file
Original file line number Diff line number Diff line change
@@ -1,34 +1,49 @@
import {ZclFrame} from '../../zcl';
import {ZclFrame, Utils as ZclUtils} from '../../zcl';
import {Cluster} from '../../zcl/tstype';

interface KeyValue {[s: string]: number | string}

function attributeKeyValue(frame: ZclFrame): KeyValue {
// Certain devices (e.g. Legrand/4129) fail to set the manufacturerSpecific flag and
// manufacturerCode in the frame header, despite using specific attributes.
// This leads to incorrect reported attribute names.
// Remap the attributes using the target device's manufacturer ID
// if the header is lacking the information.
function getCluster(frame: ZclFrame, deviceManufacturerID: number): Cluster {
let cluster = frame.Cluster;

if (!frame?.Header?.manufacturerCode && frame?.Cluster && Number.isInteger(deviceManufacturerID)) {
cluster = ZclUtils.getCluster(frame.Cluster.ID, deviceManufacturerID);
}
return cluster;
}

function attributeKeyValue(frame: ZclFrame, deviceManufacturerID: number): KeyValue {
const payload: KeyValue = {};
const cluster = getCluster(frame, deviceManufacturerID);

for (const item of frame.Payload) {
try {
const attribute = frame.Cluster.getAttribute(item.attrId);
const attribute = cluster.getAttribute(item.attrId);
payload[attribute.name] = item.attrData;
} catch (error) {
payload[item.attrId] = item.attrData;
}
}

return payload;
}

function attributeList(frame: ZclFrame): Array<string | number> {
function attributeList(frame: ZclFrame, deviceManufacturerID: number): Array<string | number> {
const payload: Array<string | number> = [];
const cluster = getCluster(frame, deviceManufacturerID);

for (const item of frame.Payload) {
try {
const attribute = frame.Cluster.getAttribute(item.attrId);
const attribute = cluster.getAttribute(item.attrId);
payload.push(attribute.name);
} catch (error) {
payload.push(item.attrId);
}
}

return payload;
}

Expand Down
4 changes: 2 additions & 2 deletions src/controller/model/device.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ class Device extends Entity {

// Update reportable properties
if (frame.isCluster('genBasic') && (frame.isCommand('readRsp') || frame.isCommand('report'))) {
for (const [key, value] of Object.entries(ZclFrameConverter.attributeKeyValue(frame))) {
Device.ReportablePropertiesMapping[key]?.set(value, this);
for (const [key, val] of Object.entries(ZclFrameConverter.attributeKeyValue(frame, this.manufacturerID))) {
Device.ReportablePropertiesMapping[key]?.set(val, this);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/controller/model/endpoint.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ class Endpoint extends Entity {

if (!options.disableResponse) {
this.checkStatus(result.frame.Payload);
return ZclFrameConverter.attributeKeyValue(result.frame);
return ZclFrameConverter.attributeKeyValue(result.frame, this.getDevice().manufacturerID);
} else {
return null;
}
Expand Down
78 changes: 78 additions & 0 deletions test/controller.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,14 @@ const mockDevices = {
1: {modelId: 'lumi.plug', manufacturerName: 'LUMI', zclVersion: 1, appVersion: 2, hwVersion: 3, dateCode: '201901', swBuildId: '1.01', powerSource: 1, stackVersion: 101},
},
},
177: {
nodeDescriptor: {type: 'Router', manufacturerCode: 4129},
activeEndpoints: {endpoints: [1]},
simpleDescriptor: {1: {endpointID: 1, deviceID: 514, inputClusters: [0, 3, 258, 4, 5, 15, 64513], outputClusters: [258, 0, 64513, 5, 25], profileID: 260}},
attributes: {
1: {modelId: ' Shutter switch with neutral', manufacturerName: 'Legrand', zclVersion: 2, appVersion: 0, hwVersion: 8, dateCode: '231030', swBuildId: '0038', powerSource: 1, stackVersion: 67},
},
},
}

const mockZclFrame = ZclFrame;
Expand Down Expand Up @@ -4971,6 +4979,76 @@ describe('Controller', () => {
expect(result.missingRouters.length).toBe(1);
expect(result.missingRouters[0].ieeeAddr).toBe('0x129');
});

// ZCLFrame with manufacturer specific flag and manufacturer code defined, to generic device
// ZCLFrameConverter should not modify specific frames!
it('Should resolve manufacturer specific cluster attribute names on specific ZCL frames: generic target device', async () => {
await controller.start();
await mockAdapterEvents['deviceJoined']({networkAddress: 129, ieeeAddr: '0x129'});
await mockAdapterEvents['zclData']({
wasBroadcast: false,
address: '0x129',
frame: ZclFrame.fromBuffer(Zcl.Utils.getCluster("closuresWindowCovering").ID, Buffer.from([28,33,16,13,1,2,240,0,48,4])),
endpoint: 1,
linkquality: 50,
groupID: 0,
});
expect(events.message.length).toBe(1);
expect(events.message[0].data).toMatchObject({calibrationMode:4});
expect(events.message[0].data).not.toMatchObject({tuyaMotorReversal:4});
});

// ZCLFrame with manufacturer specific flag and manufacturer code defined, to specific device
// ZCLFrameConverter should not modify specific frames!
it('Should resolve manufacturer specific cluster attribute names on specific ZCL frames: specific target device', async () => {
await controller.start();
await mockAdapterEvents['deviceJoined']({networkAddress: 177, ieeeAddr: '0x177'});
await mockAdapterEvents['zclData']({
wasBroadcast: false,
address: '0x177',
frame: ZclFrame.fromBuffer(Zcl.Utils.getCluster("closuresWindowCovering").ID, Buffer.from([28,33,16,13,1,2,240,0,48,4])),
endpoint: 1,
linkquality: 50,
groupID: 0,
});
expect(events.message.length).toBe(1);
expect(events.message[0].data).toMatchObject({calibrationMode:4});
expect(events.message[0].data).not.toMatchObject({tuyaMotorReversal:4});
});

// ZCLFrame without manufacturer specific flag or manufacturer code set, to generic device
it('Should resolve generic cluster attribute names on generic ZCL frames: generic target device', async () => {
await controller.start();
await mockAdapterEvents['deviceJoined']({networkAddress: 129, ieeeAddr: '0x129'});
await mockAdapterEvents['zclData']({
wasBroadcast: false,
address: '0x129',
frame: ZclFrame.fromBuffer(Zcl.Utils.getCluster("closuresWindowCovering").ID, Buffer.from([24,242,10,2,240,48,4])),
endpoint: 1,
linkquality: 50,
groupID: 0,
});
expect(events.message.length).toBe(1);
expect(events.message[0].data).toMatchObject({tuyaMotorReversal:4});
expect(events.message[0].data).not.toMatchObject({calibrationMode:4});
});

// ZCLFrame without manufacturer specific flag set or manufacturer code set, to specific device
it('Should resolve manufacturer specific cluster attribute names on generic ZCL frames: specific target device', async () => {
await controller.start();
await mockAdapterEvents['deviceJoined']({networkAddress: 177, ieeeAddr: '0x177'});
await mockAdapterEvents['zclData']({
wasBroadcast: false,
address: '0x177',
frame: ZclFrame.fromBuffer(Zcl.Utils.getCluster("closuresWindowCovering").ID, Buffer.from([24,242,10,2,240,48,4])),
endpoint: 1,
linkquality: 50,
groupID: 0,
});
expect(events.message.length).toBe(1);
expect(events.message[0].data).toMatchObject({calibrationMode:4});
expect(events.message[0].data).not.toMatchObject({tuyaMotorReversal:4});
});
});


4 comments on commit d8e4b5b

@sjorge
Copy link
Contributor

@sjorge sjorge commented on d8e4b5b Nov 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Koenkk this seems to have caused issues for Ubisys devices.
e.g. setting the vacation_mode writes to custom attribute but to read it it simply uses hvacThermostat.occupany, now the reports coming from the device match rawOutdoorTemperature

Now returns

debug 2023-11-11 19:57:36: Received Zigbee message from 'trv/office', type 'attributeReport', cluster 'hvacThermostat', data '{"rawOutdoorTemperature":1}' from endpoint 1 with groupID 0

Before it returned

debug 2023-11-11 03:42:04: Received Zigbee message from 'trv/office', type 'attributeReport', cluster 'hvacThermostat', data '{"occupancy":1}' from endpoint 1 with groupID 0

This is because with this change the manufacturer code is always enforced, so it matches

zigbee-herdsman/src/zcl/definition/cluster.ts

Line 4271 in 81ec998

rawOutdoorTemperature: {ID: 0x0002, type: DataType.struct},

Which is not valid for the Ubisys H1 for example but only for the H10, before when the manufacturer code was not set it defaulted to the default hvecThermostat cluster definition which is correct in this case, also I think the new behavior is incorrect.

The ZCL is not super clear on it IMHO, but as I read it, it seems valid for a device to use a cluster without manufacturer code to use the default attributes and with manufacturer code set to use custom ones where the attribute ID in both cases can be the same. They should still be treated as different.

I'm pretty sure the Ubisys H1/H10 are not going to be the only devices/manufacturers to be effected by this.

For the time being I've reverted my install to pre this change, I am seeing some other odd behavior too that i haven't traced down to the cause but I suspect it is also this. I'm traveling currently so it's hard to debug.

@Koenkk
Copy link
Owner

@Koenkk Koenkk commented on d8e4b5b Nov 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FabianMangold can you update your PR to only do the new logic in getCluster when the device manufacturer ID == Legrand?

@FabianMangold
Copy link
Contributor Author

@FabianMangold FabianMangold commented on d8e4b5b Nov 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, i am on it, I'll restrict the change to Legrand devices only.
Alternatively, we could remove this code alltogether, and patch the reporting on the known faulty Legrand devices.
Let me know what ever you prefer.

@sjorge is right about ZCL.
Generally speaking, manufacturers tend to use the custom attribute if they bother to create them, otherwise fall back to the generic ones and use generic frames for this purpose.
But then again, there is so many devices out there behaving completely differently.

Sorry for the regression really.

Edit: The PR is here:
#797

@Koenkk
Copy link
Owner

@Koenkk Koenkk commented on d8e4b5b Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now!

Changes will be available in the dev branch in a few hours from now. (https://www.zigbee2mqtt.io/advanced/more/switch-to-dev-branch.html)

Please sign in to comment.