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(modify): Repair the issue of channel updating #1280

Merged
merged 11 commits into from
Feb 24, 2025
Merged

fix(modify): Repair the issue of channel updating #1280

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5a3afe5
fix(modify): Repair the issue of channel switching where some devices…
danyinhao Dec 30, 2024
48ee5a7
eslint prettier fix
danyinhao Dec 30, 2024
e46ba7e
fix(modify): Extend the getNetworkParameters method to obtain the val…
danyinhao Jan 2, 2025
629088f
eslint prettier fix
danyinhao Jan 2, 2025
5b7d829
fix(modify): Implement nwkUpdateID in all adapters.
danyinhao Feb 6, 2025
276214f
Modify the retrieval of nwkUpdateID in ZStack.
danyinhao Feb 6, 2025
2f24a66
Don't ready nwkUpdateId for zstack
Koenkk Feb 8, 2025
e881e83
Whoops
Koenkk Feb 8, 2025
36b50e0
fix test failure
danyinhao Feb 11, 2025
71b7df3
note about deconz NWK_UPDATE_ID
Koenkk Feb 12, 2025
dc9b70f
The nwkUpdateID is restored from the backup in the Ember process.
danyinhao Feb 24, 2025
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
1 change: 1 addition & 0 deletions src/adapter/ember/adapter/emberAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1678,6 +1678,7 @@ export class EmberAdapter extends Adapter {
panID,
extendedPanID: ZSpec.Utils.eui64LEBufferToHex(Buffer.from(extendedPanID)),
channel,
nwkUpdateID: this.networkCache.parameters.nwkUpdateId,
};
});
}
Expand Down
1 change: 1 addition & 0 deletions src/adapter/ezsp/adapter/ezspAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ export class EZSPAdapter extends Adapter {
panID: this.driver.networkParams.panId,
extendedPanID: ZSpec.Utils.eui64LEBufferToHex(this.driver.networkParams.extendedPanId),
channel: this.driver.networkParams.radioChannel,
nwkUpdateID: this.driver.networkParams.nwkUpdateId,
};
}

Expand Down
1 change: 1 addition & 0 deletions src/adapter/tstype.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,5 @@ export interface NetworkParameters {
panID: number;
extendedPanID: string; // `0x${string}` same as IEEE address
channel: number;
nwkUpdateID?: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be optional, and support should added be in all drivers.

The change should also be tested at least in zstack and ember (the two officially supporting change channel) with at least a few devices that worked before, to make sure they still work after this change. Since it doesn't appear to be required for a change channel (based on spec & previous testing with many devices), it might have negative impact on other aspects (like some devices not expecting a changed nwkUpdateID when receiving a change channel request, and ignoring the request entirely).
In that regard, can you confirm which devices (models) are having the issue in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the main devices we have seen issues with are S31ZB, S40ZBTPB and S26R2ZB.

Regarding the necessity of changing channels, I’d like to share some real-world user experiences: in some households with complex network conditions (e.g., high Wi-Fi interference), changing channels has significantly improved the operational experience of devices. That said, I fully acknowledge that this might introduce some negative impacts, such as potential incompatibility with certain devices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment was not about changing channel (which, of course, is very useful), it was about the changes from this PR, which might fix a couple of oddly working devices, but create issues with others (since it's working without this PR for most devices, and ZigBee spec seems to mostly target PAN change for ID increment, not channel change). This is exacerbated by the fact that the 3 mentioned devices are likely the same internally (all basic sonoff plugs).

I'd like to confirm that other brands/models still work with this PR, at least Tuya, Hue, Inovelli, Ikea, Ledvance for a good sample, and including routers & end devices.
If this PR breaks any brand/model/firmware provider, it will break any according network that tries to change channel after this is merged... We need a good degree of certainty that it won't, hence actual tests with various networks.

Copy link
Contributor Author

@CubeZ2mDeveloper CubeZ2mDeveloper Feb 7, 2025 •
edited by Nerivec
Loading

Choose a reason for hiding this comment

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

Regarding the change of nwkUpdateID during channel changes, based on my understanding of the Zigbee specification (which I’ve uploaded as an image), incrementing the nwkUpdateID value is necessary. Additionally, when I performed channel changes using the same dongle on ZHA, devices that failed to work properly after a channel change in Z2M were able to function normally after a channel change in ZHA.

I fully understand your concerns about the potential impact of this PR on devices that are already working correctly. The brands you mentioned (Tuya, Hue, Inovelli, Ikea, Ledvance) indeed require comprehensive testing. However, due to limited device availability, I can only test with the devices I currently have. I will provide you with a detailed list of tested devices shortly, so you can see the coverage of my testing.

05-3474-23-csg-zigbee-specification-compressed_Page_1
05-3474-23-csg-zigbee-specification-compressed_Page_2

Copy link
Collaborator

@Nerivec Nerivec Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I updated images to latest rev.
I find that part of the spec is not very clear:

  • could be specific: Upon receipt of a Mgmt_NWK_Unsolicited_Enhanced_Update_notify message
  • logic is optional/not strictly defined: MAY do the following

Which, at best explains why it's not needed in most cases, most stacks likely just ignore it, because it's easier and not required.
There are two scenario that concern me:

  • a device stack could ignore the update request if the nwkUpdateID is different from current due to some weirdness in implementation (or some leftovers from older spec revisions)
  • a coordinator stack could be overriding the nwkUpdateID logic internally, resulting in mismatch

Also, it would seem the devices you mentioned have a firmware quirk no matter what, because it shouldn't matter for routers that are online at the time of broadcast (they should just switch channel).

I don't have enough devices either for something like this. I had a few users test it out when it was first implemented to confirm with a bigger sample.
Koenkk will confirm zstack impl (no other way than retrieving from NV?) and run some tests too.
Once we confirm with a few (non-affected) brands, it should be fine to merge.
Need to fix the zeroes for other stacks though:

And also, this is likely to require some tests to be updated in Z2M repo since it changes the return type of https://github.com/Koenkk/zigbee2mqtt/blob/dev/lib/zigbee.ts#L216

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. However, I tried to start z2m by restoring from a backup and setting the nwkUpdateID to 1, but after forming the network, it seems that the nwkUpdateID was not set to 1.
The version of z2m is 2.1.1.
d70f84ff098e7cf51805dca53c3136b1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you make the changes so the value from the backup would be used? Have to pass one more param to the function I linked above, then replace that line with nwkUpdateId,
When called from restored path (FORM_BACKUP), you set it to whatever is in backup, when called from reset path (FORM_CONFIG), you set it to zero.
Have to do roughly the same for ezsp.
I'm not sure how that works out for zstack. @Koenkk can you check?
Others don't support backup anyway...
Several tests will need fixing after this is properly added.

Copy link
Owner

Choose a reason for hiding this comment

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

@CubeZ2mDeveloper for zstack you have to write it in the NV item which was previously read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During the restore process of ezsp and zstack, the nwkUpdateID has been set to the nwkUpdateId from the backup.

ezsp

zigbee-herdsman/src/adapter/ezsp/driver/driver.ts

Line 357 in 76d3de1

parameters.nwkUpdateId = backup!.networkUpdateId;

zstack

zigbee-herdsman/src/adapter/z-stack/adapter/adapter-backup.ts

Line 275 in 76d3de1

nib.nwkUpdateId = backup.networkUpdateId;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nerivec Yes, I have made the changes as you described.

}
21 changes: 18 additions & 3 deletions src/controller/controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,17 @@ export class Controller extends events.EventEmitter<ControllerEventMap> {
const netParams = await this.getNetworkParameters();
const configuredChannel = this.options.network.channelList[0];
const adapterChannel = netParams.channel;
// According to the Zigbee specification:
// When broadcasting a Mgmt_NWK_Update_req to notify devices of a new channel, the nwkUpdateId parameter should be incremented in the NIB and included in the Mgmt_NWK_Update_req.
// The valid range of nwkUpdateId is 0x00 to 0xFF, and it should wrap back to 0 if necessary.
let nwkUpdateID = netParams.nwkUpdateID ?? 0;
if (++nwkUpdateID > 0xff) {
nwkUpdateID = 0x00;
}

if (configuredChannel != adapterChannel) {
logger.info(`Configured channel '${configuredChannel}' does not match adapter channel '${adapterChannel}', changing channel`, NS);
await this.changeChannel(adapterChannel, configuredChannel);
await this.changeChannel(adapterChannel, configuredChannel, nwkUpdateID);
}
}

Expand Down Expand Up @@ -496,11 +503,19 @@ export class Controller extends events.EventEmitter<ControllerEventMap> {
/**
* Broadcast a network-wide channel change.
*/
private async changeChannel(oldChannel: number, newChannel: number): Promise<void> {
private async changeChannel(oldChannel: number, newChannel: number, nwkUpdateID: number): Promise<void> {
logger.warning(`Changing channel from '${oldChannel}' to '${newChannel}'`, NS);

const clusterId = Zdo.ClusterId.NWK_UPDATE_REQUEST;
const zdoPayload = Zdo.Buffalo.buildRequest(this.adapter.hasZdoMessageOverhead, clusterId, [newChannel], 0xfe, undefined, 0, undefined);
const zdoPayload = Zdo.Buffalo.buildRequest(
this.adapter.hasZdoMessageOverhead,
clusterId,
[newChannel],
0xfe,
undefined,
nwkUpdateID,
undefined,
);

await this.adapter.sendZdo(ZSpec.BLANK_EUI64, ZSpec.BroadcastAddress.SLEEPY, clusterId, zdoPayload, true);
logger.info(`Channel changed to '${newChannel}'`, NS);
Expand Down
2 changes: 2 additions & 0 deletions test/adapter/ember/emberAdapter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2249,6 +2249,7 @@ describe('Ember Adapter Layer', () => {
panID: DEFAULT_NETWORK_OPTIONS.panID,
extendedPanID: ZSpec.Utils.eui64LEBufferToHex(Buffer.from(DEFAULT_NETWORK_OPTIONS.extendedPanID!)),
channel: DEFAULT_NETWORK_OPTIONS.channelList[0],
nwkUpdateID: 0,
} as TsType.NetworkParameters);
expect(mockEzspGetNetworkParameters).toHaveBeenCalledTimes(0);
});
Expand All @@ -2260,6 +2261,7 @@ describe('Ember Adapter Layer', () => {
panID: DEFAULT_NETWORK_OPTIONS.panID,
extendedPanID: ZSpec.Utils.eui64LEBufferToHex(Buffer.from(DEFAULT_NETWORK_OPTIONS.extendedPanID!)),
channel: DEFAULT_NETWORK_OPTIONS.channelList[0],
nwkUpdateID: 0,
} as TsType.NetworkParameters);
expect(mockEzspGetNetworkParameters).toHaveBeenCalledTimes(1);
});
Expand Down
100 changes: 59 additions & 41 deletions test/controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,44 @@ const mockLogger = {
error: vi.fn(),
};

const mockDummyBackup: Models.Backup = {
networkOptions: {
panId: 6755,
extendedPanId: Buffer.from('deadbeef01020304', 'hex'),
channelList: [11],
networkKey: Buffer.from('a1a2a3a4a5a6a7a8b1b2b3b4b5b6b7b8', 'hex'),
networkKeyDistribute: false,
},
coordinatorIeeeAddress: Buffer.from('0102030405060708', 'hex'),
logicalChannel: 11,
networkUpdateId: 0,
securityLevel: 5,
znp: {
version: 1,
},
networkKeyInfo: {
sequenceNumber: 0,
frameCounter: 10000,
},
devices: [
{
networkAddress: 1001,
ieeeAddress: Buffer.from('c1c2c3c4c5c6c7c8', 'hex'),
isDirectChild: false,
},
{
networkAddress: 1002,
ieeeAddress: Buffer.from('d1d2d3d4d5d6d7d8', 'hex'),
isDirectChild: false,
linkKey: {
key: Buffer.from('f8f7f6f5f4f3f2f1e1e2e3e4e5e6e7e8', 'hex'),
rxCounter: 10000,
txCounter: 5000,
},
},
],
};

const mockAdapterEvents = {};
const mockAdapterWaitFor = vi.fn();
const mockAdapterSupportsDiscoverRoute = vi.fn();
Expand All @@ -56,6 +94,7 @@ const mocksendZclFrameToGroup = vi.fn();
const mocksendZclFrameToAll = vi.fn();
const mockAddInstallCode = vi.fn();
const mocksendZclFrameToEndpoint = vi.fn();
const mockApaterBackup = vi.fn(() => Promise.resolve(mockDummyBackup));
let sendZdoResponseStatus = Zdo.Status.SUCCESS;
const mockAdapterSendZdo = vi
.fn()
Expand Down Expand Up @@ -318,44 +357,6 @@ const getCluster = (key) => {
return cluster;
};

const mockDummyBackup: Models.Backup = {
networkOptions: {
panId: 6755,
extendedPanId: Buffer.from('deadbeef01020304', 'hex'),
channelList: [11],
networkKey: Buffer.from('a1a2a3a4a5a6a7a8b1b2b3b4b5b6b7b8', 'hex'),
networkKeyDistribute: false,
},
coordinatorIeeeAddress: Buffer.from('0102030405060708', 'hex'),
logicalChannel: 11,
networkUpdateId: 0,
securityLevel: 5,
znp: {
version: 1,
},
networkKeyInfo: {
sequenceNumber: 0,
frameCounter: 10000,
},
devices: [
{
networkAddress: 1001,
ieeeAddress: Buffer.from('c1c2c3c4c5c6c7c8', 'hex'),
isDirectChild: false,
},
{
networkAddress: 1002,
ieeeAddress: Buffer.from('d1d2d3d4d5d6d7d8', 'hex'),
isDirectChild: false,
linkKey: {
key: Buffer.from('f8f7f6f5f4f3f2f1e1e2e3e4e5e6e7e8', 'hex'),
rxCounter: 10000,
txCounter: 5000,
},
},
],
};

let dummyBackup;

vi.mock('../src/adapter/z-stack/adapter/zStackAdapter', () => ({
Expand All @@ -368,9 +369,7 @@ vi.mock('../src/adapter/z-stack/adapter/zStackAdapter', () => ({
getCoordinatorIEEE: mockAdapterGetCoordinatorIEEE,
reset: mockAdapterReset,
supportsBackup: mockAdapterSupportsBackup,
backup: () => {
return mockDummyBackup;
},
backup: mockApaterBackup,
getCoordinatorVersion: () => {
return {type: 'zStack', meta: {version: 1}};
},
Expand Down Expand Up @@ -1562,6 +1561,25 @@ describe('Controller', () => {
const changeChannelSpy = vi.spyOn(controller, 'changeChannel');
await controller.start();
expect(mockAdapterGetNetworkParameters).toHaveBeenCalledTimes(1);
const zdoPayload = Zdo.Buffalo.buildRequest(false, Zdo.ClusterId.NWK_UPDATE_REQUEST, [15], 0xfe, undefined, 1, undefined);
expect(mockAdapterSendZdo).toHaveBeenCalledWith(
ZSpec.BLANK_EUI64,
ZSpec.BroadcastAddress.SLEEPY,
Zdo.ClusterId.NWK_UPDATE_REQUEST,
zdoPayload,
true,
);
expect(await controller.getNetworkParameters()).toEqual({panID: 1, channel: 15, extendedPanID: '0x64c5fd698daf0c00'});
expect(changeChannelSpy).toHaveBeenCalledTimes(1);
});

it('Change channel on start when nwkUpdateID is 0xff', async () => {
mockAdapterStart.mockReturnValueOnce('resumed');
mockAdapterGetNetworkParameters.mockReturnValueOnce({panID: 1, extendedPanID: '0x64c5fd698daf0c00', channel: 25, nwkUpdateID: 0xff});
// @ts-expect-error private
const changeChannelSpy = vi.spyOn(controller, 'changeChannel');
await controller.start();
expect(mockAdapterGetNetworkParameters).toHaveBeenCalledTimes(1);
const zdoPayload = Zdo.Buffalo.buildRequest(false, Zdo.ClusterId.NWK_UPDATE_REQUEST, [15], 0xfe, undefined, 0, undefined);
expect(mockAdapterSendZdo).toHaveBeenCalledWith(
ZSpec.BLANK_EUI64,
Expand Down
Loading