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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 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
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
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
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
Collaborator

@Nerivec Nerivec Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't returning zero be a problem though? If you ever change the channel twice (or more) without resetting the network in-between but restarting Z2M, that would no longer be properly incremented (0, 1, 0, 1...).
For the coordinator it might be ok (assuming it always replaces with the NIB value), but how does it fare for the ZDO request being broadcasted? Is the request being altered with the new NIB value too?
From what I can tell, the issue first identified by this PR is not on the coordinator's side (since both zstack and ember appear to just ignore that param), it's on the broadcasted ZDO to devices.

Copy link
Owner

Choose a reason for hiding this comment

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

If you ever change the channel twice (or more) without resetting the network in-between but restarting Z2M, that would no longer be properly incremented (0, 1, 0, 1...).

What I tried is changing the channel multiple times, it was always properly incremented (1,2,3).

ZDO request being broadcasted? Is the request being altered with the new NIB value too?

Yes, note that zstack doesn't even ignore the value, it isn't even passed to it since nwkUpdateId is not in

zigbee-herdsman/src/adapter/z-stack/znp/definition.ts

Line 1364 in 877e905

zdoClusterId: ZdoClusterId.NWK_UPDATE_REQUEST,

Copy link
Collaborator

Choose a reason for hiding this comment

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

As expected, this param is a mess...
Have you tried sniffing the ZDO broadcast that the coordinator then sends over the network to see what value the nwkUpdateId has?

Copy link
Owner

Choose a reason for hiding this comment

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

Have you tried sniffing the ZDO broadcast that the coordinator then sends over the network to see what value the nwkUpdateId has?

Yes, it is incremented by 1 after every channel change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For deconz, I think this should do, can you confirm with your adapter?

NWK_UPDATE_ID: 0x24,
const nwkUpdateId = await this.driver.readParameterRequest(PARAM.PARAM.Network.NWK_UPDATE_ID);

Although the network params changing is all screwed up in deconz, so, won't do much good for changing channel.
#1257 should solve that, but requires extensive testing...

Remains zboss.

}
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:
Nerivec marked this conversation as resolved.
Show resolved Hide resolved
// 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