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: ZStack: throw errors when ZDO calls fail #1128

Merged
merged 2 commits into from
Jul 26, 2024
Merged

fix: ZStack: throw errors when ZDO calls fail #1128

merged 2 commits into from
Jul 26, 2024

Conversation

Koenkk
Copy link
Owner

@Koenkk Koenkk commented Jul 23, 2024

Continuation of #1125

Fixes Koenkk/zigbee2mqtt#22492

@Koenkk Koenkk requested a review from Nerivec July 23, 2024 19:12
@@ -1117,6 +1096,27 @@ class ZStackAdapter extends Adapter {
});
}

private waitForAreqZdo(command: string, payload?: ZpiObjectPayload): {start: () => Promise<ZpiObject>; ID: number} {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The status checking is implemented directly in the Zdo spec with BuffaloZdo.readResponse(clusterId, buffer); link, so you could remove some of the logic here, but I guess that would mean bypassing the ZpiObject parsing for AREQ/ZDO and using the spec instead?
(it would automatically add support for r23 too, since the spec should be r23 compliant)

Copy link
Owner Author

Choose a reason for hiding this comment

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

but I guess that would mean bypassing the ZpiObject parsing for AREQ/ZDO and using the spec instead?

The difference with ember is that there is one ZDO response callback where you get the full buffer, for zstack there is a callback per ZDO type, so we do not have the full buffer. So I don't think we can use this for zstack (?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like zstack is modding it (like not passing the transaction seq num?), so that wouldn't work as-is. 😢
If it's just the seq num, I guess we could make a param in the spec to not skip 1 byte when reading?

Copy link
Owner Author

Choose a reason for hiding this comment

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

So it means we need to reconstruct the buffer again from the parsed ZDO message?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Glanced at the code... I think something like this should allow a smooth-ish transition.

in ZpiObject:

const ZDO_RSP_COMMAND_ID_TO_CLUSTER_ID: Map<number, Zdo.ClusterId> = new Map([
    [128, Zdo.ClusterId.NETWORK_ADDRESS_RESPONSE],
    // etc.
]);
    public static fromUnpiFrame(frame: UnpiFrame): ZpiObject {
        if (frame.type === Type.AREQ && frame.subsystem === Subsystem.ZDO) {
            const clusterId = ZDO_RSP_COMMAND_ID_TO_CLUSTER_ID.get(frame.commandID);

            return new ZpiObject(
                frame.type,
                frame.subsystem,
                Zdo.ClusterId[clusterId],
                frame.commandID,// or clusterId?
                BuffaloZdo.readResponse(clusterId, frame.data, false),
                null,// will this be ok? (avoids lookup)
            );
        }

       // ...

in BuffaloZdo:

    public static readResponse(clusterId: ZdoClusterId, buffer: Buffer, hasOverhead: boolean = true): unknown {
        const buffalo = new BuffaloZdo(buffer, hasOverhead ? ZDO_MESSAGE_OVERHEAD : 0); // set pos to skip `transaction sequence number`
       // ...

This would require matching the Zdo spec payloads throughout the code (object keys, all typed though) and likely some tweaks to response waiters (cluster name instead of command name?). And handling the ZDO status throwing where fromUnpiFrame is called.

@Koenkk Koenkk merged commit e47812b into master Jul 26, 2024
2 checks passed
@Koenkk Koenkk deleted the fix/zstack-zdo branch July 26, 2024 20:51
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.

problem wih ember driver and joining develco devices
2 participants