Skip to content

Conversation

@manup
Copy link
Contributor

@manup manup commented Jul 3, 2025

The old ZGP frame parser used some magic numbers which often shoot over the actual received buffer. In the new adapter implementation frames are exposed with their actual size, which as side effect caused the frame parser to have invalid negative offsets.

The new implementation is a port from deCONZ code without fixed magic numbers.

Tested with a old Hue Tap and Friends of Hue switch.

Note that the old implementation likely didn't support "joining" without ZGP proxies since any frame length >= 30 was treated as commissioning command (0x04), which the controller rejected.

Issue: Koenkk/zigbee2mqtt#27888

The old ZGP frame parser used some magic numbers which often shoot over  the actual received buffer.
In the new adapter implementation frames are exposed with their actual size, which as side effect caused the frame parser to have invalid negative offsets.

The new implementation is a port from deCONZ code without fixed magic numbers.

Tested with a old Hue Tap and Friends of Hue switch.

Note that the old implementation likely didn't support "joining" without ZGP proxies since the any frame length >= 30 was treated as commissioning command, which the controller rejected.

Issue: Koenkk/zigbee2mqtt#27888
@manup manup marked this pull request as draft July 4, 2025 09:17
Refactored ZGP parsing getting rid of switch() and magic numbers.
Also reviewed against specification and tested again with Hue Tap and Friends of Hue switches.
@manup manup force-pushed the deconz_fix_zgp branch from 4504b75 to cf76f26 Compare July 4, 2025 09:48
@manup manup requested a review from Nerivec July 4, 2025 09:49
@manup
Copy link
Contributor Author

manup commented Jul 4, 2025

@Nerivec thanks a lot for the review 👍 The updated PR hopefully addresses all comments expect the initial buffer copy which I'd like to address in another PR to get rid of DataView.

Copy link
Owner

@Koenkk Koenkk left a comment

Choose a reason for hiding this comment

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

Looks good! Please move out of draft once this is ready for merge

Copy link
Collaborator

@Nerivec Nerivec left a comment

Choose a reason for hiding this comment

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

👍

Note: enum != const enum (only the latter is inlined at compile time) https://www.typescriptlang.org/docs/handbook/enums.html#const-enums

@manup manup marked this pull request as ready for review July 4, 2025 13:08
@manup
Copy link
Contributor Author

manup commented Jul 4, 2025

👍

Note: enum != const enum (only the latter is inlined at compile time) https://www.typescriptlang.org/docs/handbook/enums.html#const-enums

Good to know, guess there are quite a few places where this can be applied in future PRs.

@Koenkk Koenkk merged commit 6632085 into Koenkk:master Jul 4, 2025
1 check passed
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.

3 participants