-
Notifications
You must be signed in to change notification settings - Fork 310
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
Conversation
@@ -1117,6 +1096,27 @@ class ZStackAdapter extends Adapter { | |||
}); | |||
} | |||
|
|||
private waitForAreqZdo(command: string, payload?: ZpiObjectPayload): {start: () => Promise<ZpiObject>; ID: number} { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 (?)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Continuation of #1125
Fixes Koenkk/zigbee2mqtt#22492