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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fae9481
fix: ZStack: throw errors when ZDO calls fail
Koenkk Jul 23, 2024
f5dd68b
process feedback
Koenkk Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 44 additions & 43 deletions src/adapter/z-stack/adapter/zStackAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {Queue, Waitress, Wait} from '../../../utils';
import {logger} from '../../../utils/logger';
import {BroadcastAddress} from '../../../zspec/enums';
import * as Zcl from '../../../zspec/zcl';
import {Status as ZdoStatus} from '../../../zspec/zdo';
import Adapter from '../../adapter';
import * as Events from '../../events';
import {
Expand All @@ -28,6 +29,7 @@ import {
import * as Constants from '../constants';
import {Constants as UnpiConstants} from '../unpi';
import {Znp, ZpiObject} from '../znp';
import {ZpiObjectPayload} from '../znp/tstype';
import {ZnpAdapterManager} from './manager';
import {ZnpVersion} from './tstype';

Expand Down Expand Up @@ -174,19 +176,17 @@ class ZStackAdapter extends Adapter {
public async getCoordinator(): Promise<Coordinator> {
return this.queue.execute<Coordinator>(async () => {
this.checkInterpanLock();
const activeEpRsp = this.znp.waitFor(UnpiConstants.Type.AREQ, Subsystem.ZDO, 'activeEpRsp');
const activeEpRsp = this.waitForAreqZdo('activeEpRsp');
await this.znp.request(Subsystem.ZDO, 'activeEpReq', {dstaddr: 0, nwkaddrofinterest: 0}, activeEpRsp.ID);
const activeEp = await activeEpRsp.start().promise;
const activeEp = await activeEpRsp.start();

const deviceInfo = await this.znp.request(Subsystem.UTIL, 'getDeviceInfo', {});

const endpoints = [];
for (const endpoint of activeEp.payload.activeeplist) {
const simpleDescRsp = this.znp.waitFor(UnpiConstants.Type.AREQ, Subsystem.ZDO, 'simpleDescRsp', {endpoint});

const simpleDescRsp = this.waitForAreqZdo('simpleDescRsp', {endpoint});
await this.znp.request(Subsystem.ZDO, 'simpleDescReq', {dstaddr: 0, nwkaddrofinterest: 0, endpoint}, simpleDescRsp.ID);

const simpleDesc = await simpleDescRsp.start().promise;
const simpleDesc = await simpleDescRsp.start();

endpoints.push({
ID: simpleDesc.payload.endpoint,
Expand Down Expand Up @@ -267,9 +267,9 @@ class ZStackAdapter extends Adapter {
* this is currently not handled, the first nwkAddrRsp is taken.
*/
logger.debug(`Request network address of '${ieeeAddr}'`, NS);
const response = this.znp.waitFor(UnpiConstants.Type.AREQ, Subsystem.ZDO, 'nwkAddrRsp', {ieeeaddr: ieeeAddr});
const response = this.waitForAreqZdo('nwkAddrRsp', {ieeeaddr: ieeeAddr});
await this.znp.request(Subsystem.ZDO, 'nwkAddrReq', {ieeeaddr: ieeeAddr, reqtype: 0, startindex: 0});
const result = await response.start().promise;
const result = await response.start();
return result.payload.nwkaddr;
}

Expand Down Expand Up @@ -309,10 +309,10 @@ class ZStackAdapter extends Adapter {
}

private async nodeDescriptorInternal(networkAddress: number): Promise<NodeDescriptor> {
const response = this.znp.waitFor(Type.AREQ, Subsystem.ZDO, 'nodeDescRsp', {nwkaddr: networkAddress});
const response = this.waitForAreqZdo('nodeDescRsp', {nwkaddr: networkAddress});
const payload = {dstaddr: networkAddress, nwkaddrofinterest: networkAddress};
await this.znp.request(Subsystem.ZDO, 'nodeDescReq', payload, response.ID);
const descriptor = await response.start().promise;
const descriptor = await response.start();

let type: DeviceType = 'Unknown';
const logicalType = descriptor.payload.logicaltype_cmplxdescavai_userdescavai & 0x07;
Expand All @@ -331,10 +331,10 @@ class ZStackAdapter extends Adapter {
public async activeEndpoints(networkAddress: number): Promise<ActiveEndpoints> {
return this.queue.execute<ActiveEndpoints>(async () => {
this.checkInterpanLock();
const response = this.znp.waitFor(Type.AREQ, Subsystem.ZDO, 'activeEpRsp', {nwkaddr: networkAddress});
const response = this.waitForAreqZdo('activeEpRsp', {nwkaddr: networkAddress});
const payload = {dstaddr: networkAddress, nwkaddrofinterest: networkAddress};
await this.znp.request(Subsystem.ZDO, 'activeEpReq', payload, response.ID);
const activeEp = await response.start().promise;
const activeEp = await response.start();
return {endpoints: activeEp.payload.activeeplist};
}, networkAddress);
}
Expand All @@ -343,10 +343,10 @@ class ZStackAdapter extends Adapter {
return this.queue.execute<SimpleDescriptor>(async () => {
this.checkInterpanLock();
const responsePayload = {nwkaddr: networkAddress, endpoint: endpointID};
const response = this.znp.waitFor(Type.AREQ, Subsystem.ZDO, 'simpleDescRsp', responsePayload);
const response = this.waitForAreqZdo('simpleDescRsp', responsePayload);
const payload = {dstaddr: networkAddress, nwkaddrofinterest: networkAddress, endpoint: endpointID};
await this.znp.request(Subsystem.ZDO, 'simpleDescReq', payload, response.ID);
const descriptor = await response.start().promise;
const descriptor = await response.start();
return {
profileID: descriptor.payload.profileid,
endpointID: descriptor.payload.endpoint,
Expand Down Expand Up @@ -694,15 +694,9 @@ class ZStackAdapter extends Adapter {

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const request = async (startIndex: number): Promise<any> => {
const response = this.znp.waitFor(Type.AREQ, Subsystem.ZDO, 'mgmtLqiRsp', {srcaddr: networkAddress});
const response = this.waitForAreqZdo('mgmtLqiRsp', {srcaddr: networkAddress});
await this.znp.request(Subsystem.ZDO, 'mgmtLqiReq', {dstaddr: networkAddress, startindex: startIndex}, response.ID);
const result = await response.start().promise;
if (result.payload.status !== ZnpCommandStatus.SUCCESS) {
throw new Error(
`LQI for '${networkAddress}' failed with error: '${ZnpCommandStatus[result.payload.status]}' (${result.payload.status})`,
);
}

const result = await response.start();
return result;
};

Expand Down Expand Up @@ -741,17 +735,9 @@ class ZStackAdapter extends Adapter {

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const request = async (startIndex: number): Promise<any> => {
const response = this.znp.waitFor(Type.AREQ, Subsystem.ZDO, 'mgmtRtgRsp', {srcaddr: networkAddress});
const response = this.waitForAreqZdo('mgmtRtgRsp', {srcaddr: networkAddress});
await this.znp.request(Subsystem.ZDO, 'mgmtRtgReq', {dstaddr: networkAddress, startindex: startIndex}, response.ID);
const result = await response.start().promise;
if (result.payload.status !== ZnpCommandStatus.SUCCESS) {
throw new Error(
`Routing table for '${networkAddress}' failed with error: '${
ZnpCommandStatus[result.payload.status]
}' (${result.payload.status})`,
);
}

const result = await response.start();
return result;
};

Expand Down Expand Up @@ -841,7 +827,7 @@ class ZStackAdapter extends Adapter {
): Promise<void> {
return this.queue.execute<void>(async () => {
this.checkInterpanLock();
const response = this.znp.waitFor(Type.AREQ, Subsystem.ZDO, `${bindType}Rsp`, {srcaddr: destinationNetworkAddress});
const response = this.waitForAreqZdo(`${bindType}Rsp`, {srcaddr: destinationNetworkAddress});

const payload = {
dstaddr: destinationNetworkAddress,
Expand All @@ -854,21 +840,14 @@ class ZStackAdapter extends Adapter {
};

await this.znp.request(Subsystem.ZDO, `${bindType}Req`, payload, response.ID);
const result = await response.start().promise;
if (result.payload.status !== 0) {
// Z-Stack only returns status 0 or 1, so not the actual error code
throw new Error(
`Failed to ${bindType} '${clusterID}' from '${destinationAddressOrGroup}/${destinationEndpoint}' ` +
`to '${sourceIeeeAddress}/${sourceEndpoint}' (${result.payload.status})`,
);
}
await response.start();
}, destinationNetworkAddress);
}

public removeDevice(networkAddress: number, ieeeAddr: string): Promise<void> {
return this.queue.execute<void>(async () => {
this.checkInterpanLock();
const response = this.znp.waitFor(UnpiConstants.Type.AREQ, Subsystem.ZDO, 'mgmtLeaveRsp', {srcaddr: networkAddress});
const response = this.waitForAreqZdo('mgmtLeaveRsp', {srcaddr: networkAddress});

const payload = {
dstaddr: networkAddress,
Expand All @@ -877,7 +856,7 @@ class ZStackAdapter extends Adapter {
};

await this.znp.request(Subsystem.ZDO, 'mgmtLeaveReq', payload, response.ID);
await response.start().promise;
await response.start();
}, networkAddress);
}

Expand Down Expand Up @@ -1117,6 +1096,28 @@ 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.

const result = this.znp.waitFor(UnpiConstants.Type.AREQ, Subsystem.ZDO, command, payload);
const start = (): Promise<ZpiObject> => {
const startResult = result.start();
return new Promise<ZpiObject>((resolve, reject) => {
startResult.promise
.then((response) => {
// Even though according to the Z-Stack docs the status is `0` or `1`, the actual code
// shows it sets the `zstack_ZdpStatus` which contains the ZDO status.
const code = response.payload.status;
if (code !== ZdoStatus.SUCCESS) {
reject(new Error(`ZDO error: ${command.replace('Rsp', '')} failed with status '${ZdoStatus[code]}' (${code})`));
} else {
resolve(response);
}
})
.catch(reject);
});
};
return {start, ID: result.ID};
}

private waitForInternal(
networkAddress: number,
endpoint: number,
Expand Down
54 changes: 36 additions & 18 deletions test/adapter/z-stack/adapter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {ZclPayload} from '../../../src/adapter/events';
import {UnifiedBackupStorage} from '../../../src/models';
import {setLogger} from '../../../src/utils/logger';
import {BroadcastAddress} from '../../../src/zspec/enums';
import {Status} from '../../../src/zspec/zdo';

const mockLogger = {
debug: jest.fn(),
Expand All @@ -32,7 +33,7 @@ const waitForResult = (payload, ID = null) => {
ID = ID || 1;
return {
start: () => {
return {promise: payload, ID};
return {promise: new Promise((r) => r(payload)), ID};
},
ID,
};
Expand Down Expand Up @@ -1159,7 +1160,7 @@ const mockZnpWaitForDefault = () => {
};

if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'activeEpRsp') {
return waitForResult({payload: {activeeplist: []}});
return waitForResult({payload: {status: Status.SUCCESS, activeeplist: []}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'stateChangeInd') {
return waitForResult({payload: {state: 9}});
} else {
Expand All @@ -1177,7 +1178,7 @@ const mockZnpWaitForStateChangeIndTimeout = () => {
};

if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'activeEpRsp') {
return waitForResult({payload: {activeeplist: []}});
return waitForResult({payload: {status: Status.SUCCESS, activeeplist: []}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'stateChangeInd') {
return;
} else {
Expand All @@ -1198,16 +1199,29 @@ const basicMocks = () => {
};

if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'activeEpRsp') {
return waitForResult({payload: {activeeplist: [1, 2, 3, 4, 5, 6, 8, 10, 11, 110, 12, 13, 47, 242]}});
return waitForResult({payload: {status: Status.SUCCESS, activeeplist: [1, 2, 3, 4, 5, 6, 8, 10, 11, 110, 12, 13, 47, 242]}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'stateChangeInd') {
return waitForResult({payload: {}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'simpleDescRsp') {
if (equals(payload, {endpoint: 1})) {
return waitForResult({payload: {endpoint: 1, profileid: 123, deviceid: 5, inclusterlist: [1], outclusterlist: [2]}});
return waitForResult({
payload: {status: Status.SUCCESS, endpoint: 1, profileid: 123, deviceid: 5, inclusterlist: [1], outclusterlist: [2]},
});
} else if (equals(payload, {endpoint: 99})) {
return waitForResult({payload: {endpoint: 99, profileid: 123, deviceid: 5, inclusterlist: [1], outclusterlist: [2]}});
return waitForResult({
payload: {status: Status.SUCCESS, endpoint: 99, profileid: 123, deviceid: 5, inclusterlist: [1], outclusterlist: [2]},
});
} else {
return waitForResult({payload: {endpoint: payload.endpoint, profileid: 124, deviceid: 7, inclusterlist: [8], outclusterlist: [9]}});
return waitForResult({
payload: {
status: Status.SUCCESS,
endpoint: payload.endpoint,
profileid: 124,
deviceid: 7,
inclusterlist: [8],
outclusterlist: [9],
},
});
}
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'nodeDescRsp') {
if (nodeDescRspErrorOnce) {
Expand All @@ -1224,7 +1238,9 @@ const basicMocks = () => {
};
}

return waitForResult({payload: {manufacturercode: payload.nwkaddr * 2, logicaltype_cmplxdescavai_userdescavai: payload.nwkaddr - 1}});
return waitForResult({
payload: {status: Status.SUCCESS, manufacturercode: payload.nwkaddr * 2, logicaltype_cmplxdescavai_userdescavai: payload.nwkaddr - 1},
});
} else if (type === Type.AREQ && subsystem === Subsystem.AF && command === 'dataConfirm') {
const status = dataConfirmCode;
if (dataConfirmCodeReset) {
Expand Down Expand Up @@ -1274,7 +1290,7 @@ const basicMocks = () => {
});
}
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'mgmtLqiRsp' && equals(payload, {srcaddr: 204})) {
return waitForResult({payload: {status: 1}});
return waitForResult({payload: {status: Status.NOT_AUTHORIZED}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'mgmtRtgRsp' && equals(payload, {srcaddr: 205})) {
if (lastStartIndex === 0) {
return waitForResult({
Expand Down Expand Up @@ -1304,17 +1320,17 @@ const basicMocks = () => {
});
}
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'mgmtRtgRsp' && equals(payload, {srcaddr: 206})) {
return waitForResult({payload: {status: 1}});
return waitForResult({payload: {status: Status.INSUFFICIENT_SPACE}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'bindRsp' && equals(payload, {srcaddr: 301})) {
return waitForResult({payload: {status: bindStatusResponse}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'unbindRsp' && equals(payload, {srcaddr: 301})) {
return waitForResult({payload: {status: bindStatusResponse}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'mgmtLeaveRsp' && equals(payload, {srcaddr: 401})) {
return waitForResult({});
return waitForResult({payload: {status: Status.SUCCESS}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'nwkAddrRsp' && payload.ieeeaddr === '0x03') {
return waitForResult({payload: {nwkaddr: 3, ieeeaddr: '0x03'}});
return waitForResult({payload: {status: Status.SUCCESS, nwkaddr: 3, ieeeaddr: '0x03'}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'nwkAddrRsp' && payload.ieeeaddr === '0x02') {
return waitForResult({payload: {nwkaddr: 2, ieeeaddr: '0x02'}});
return waitForResult({payload: {status: Status.SUCCESS, nwkaddr: 2, ieeeaddr: '0x02'}});
} else {
missing();
}
Expand Down Expand Up @@ -1958,7 +1974,7 @@ describe('zstack-adapter', () => {
};

if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'activeEpRsp') {
return waitForResult({payload: {activeeplist: [1, 2, 3]}});
return waitForResult({payload: {status: Status.SUCCESS, activeeplist: [1, 2, 3]}});
} else if (type === Type.AREQ && subsystem === Subsystem.ZDO && command === 'stateChangeInd') {
return waitForResult({payload: {state: 9}});
} else {
Expand Down Expand Up @@ -3431,7 +3447,7 @@ describe('zstack-adapter', () => {
} catch (e) {
error = e;
}
expect(error).toStrictEqual(new Error("LQI for '204' failed with error: 'FAILURE' (1)"));
expect(error).toStrictEqual(new Error("ZDO error: mgmtLqi failed with status 'NOT_AUTHORIZED' (141)"));
expect(mockQueueExecute.mock.calls[0][1]).toBe(204);
expect(mockZnpRequest).toBeCalledTimes(1);
expect(mockZnpRequest).toBeCalledWith(Subsystem.ZDO, 'mgmtLqiReq', {dstaddr: 204, startindex: 0}, 1);
Expand Down Expand Up @@ -3471,7 +3487,7 @@ describe('zstack-adapter', () => {
} catch (e) {
error = e;
}
expect(error).toStrictEqual(new Error("Routing table for '206' failed with error: 'FAILURE' (1)"));
expect(error).toStrictEqual(new Error("ZDO error: mgmtRtg failed with status 'INSUFFICIENT_SPACE' (138)"));
expect(mockQueueExecute.mock.calls[0][1]).toBe(206);
expect(mockZnpRequest).toBeCalledTimes(1);
expect(mockZnpRequest).toBeCalledWith(Subsystem.ZDO, 'mgmtRtgReq', {dstaddr: 206, startindex: 0}, 1);
Expand All @@ -3497,8 +3513,10 @@ describe('zstack-adapter', () => {
basicMocks();
await adapter.start();
mockZnpRequest.mockClear();
bindStatusResponse = 1;
await expect(adapter.bind(301, '0x129', 1, 1, 4, 'endpoint', 9)).rejects.toThrow(`Failed to bind '1' from '4/9' to '0x129/1' (1)`);
bindStatusResponse = 0x8e;
await expect(adapter.bind(301, '0x129', 1, 1, 4, 'endpoint', 9)).rejects.toThrow(
`ZDO error: bind failed with status 'DEVICE_BINDING_TABLE_FULL' (142)`,
);
});
Nerivec marked this conversation as resolved.
Show resolved Hide resolved

it('Bind group', async () => {
Expand Down