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: AttributeReport: Erroneous attribute names #787

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

FabianMangold
Copy link
Contributor

Reparse 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.

Hi @Koenkk,
with reference to the issue mentioned here: Koenkk/zigbee-herdsman-converters#6333
What do you think? Is this acceptable ?

Cheers


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

function attributeKeyValue(frame: ZclFrame): KeyValue {
function attributeKeyValue(frame: ZclFrame, manufacturerID?: number): KeyValue {
Copy link
Owner

@Koenkk Koenkk Oct 27, 2023

Choose a reason for hiding this comment

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

Please make this argument mandatory and name it deviceManfacturerID

payload[attribute.name] = item.attrData;
Debug.log(`Attr: ${item.attrId} (${attribute.name}) -> ${item.attrData}}`);
Copy link
Owner

Choose a reason for hiding this comment

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

I propose to not add any debug logging here, it will pollute the log files a lot.

@Koenkk
Copy link
Owner

Koenkk commented Oct 27, 2023

Looks good!

Could you make similar changes to attributeList (it suffers from the same issue)

@@ -683,7 +683,13 @@ class Controller extends events.EventEmitter {
if (frame.isGlobal()) {
if (frame.isCommand('report')) {
type = 'attributeReport';
data = ZclFrameConverter.attributeKeyValue(dataPayload.frame);
// Certain devices (e.g. Legrand/4129) fail to set the manufacturerSpecific flag and
Copy link
Owner

Choose a reason for hiding this comment

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

Please move these comments to zclFrameConverters.ts, the only change needed here is:

data = ZclFrameConverter.attributeKeyValue(dataPayload.frame, device.manufacturerID);

@FabianMangold
Copy link
Contributor Author

FabianMangold commented Oct 27, 2023

Thanks for your feedback @Koenkk.
I've updated the PR accordingly, but that brought changes to 2 additional models (Endpoint and Device).
The tests are failing for now, i'll take a look shortly.

@Koenkk
Copy link
Owner

Koenkk commented Oct 27, 2023

@FabianMangold no need to hurry, I will merge this after the next z2m release so we have enough time to test this in dev.

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.
* zclFrameConverter: deviceManufacturerID is now mandatory
* Updated calls to ZclFrameConverter due to the method signature change
@FabianMangold
Copy link
Contributor Author

FabianMangold commented Oct 28, 2023

Hello @Koenkk,
No worries, it is already fixed. I have integrated all of your suggestions, but had to implement some null checks due to the bogus frames used in the tests!
Hope that is OK now, let me know.

@Koenkk
Copy link
Owner

Koenkk commented Oct 29, 2023

Looks good!

Would you mind adding a test case to controller.test.ts to test the specific case for the Legrand device?

@FabianMangold
Copy link
Contributor Author

Of course, let me check how this can be mocked best, not sure how to do this yet :).

@FabianMangold
Copy link
Contributor Author

Hi @Koenkk,

I've added 4 new tests to the suite, to validate all use-cases.
Let me know if this is OK
Thanks a lot.

@Koenkk Koenkk merged commit d8e4b5b into Koenkk:master Nov 3, 2023
1 check passed
@Koenkk
Copy link
Owner

Koenkk commented Nov 3, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants