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

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

Conversation

CubeZ2mDeveloper
Copy link

When I update channels in zigbee2mqtt and restart, I find that some devices fail to be controlled. I discovered that when changing channels, the value of the nwkUpdateId field remains at 0.

I referred to the ZigBee specification and found the instructions regarding the modification of the nwkUpdateId field when changing channels:

"The network manager should broadcast a Mgmt_NWK_Update_req notifying devices of the new channel. The broadcast shall be to all devices with RxOnWhenIdle equal to TRUE. The network manager is responsible for incrementing the nwkUpdateId parameter from the NIB and including it in the Mgmt_NWK_Update_req. The network manager shall set a timer based on the value of apsChannelTimer upon issue of a Mgmt_NWK_Update_req that changes channels and shall not issue another such command until this timer expires. However, during this period, the network manager can complete the above analysis. However, instead of changing channels, the network manager would report to the local application using Mgmt_NWK_Update_notify and the application can force a channel change using the Mgmt_NWK_Update_req."

let nwkUpdateId: number = 0x00;
const isSupportsBackup = await this.adapter.supportsBackup();
if (isSupportsBackup) {
const backup = await this.adapter.backup(this.getDeviceIeeeAddresses());
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think making a new backup is necessary here? Can't we get the value from the existing backup?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed.
Probably the property should be added to the adapter type NetworkParameters, since it's part of that, and then can be used throughout controller without issue (cached on start).
Remains to see if all adapters provide it though.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will try to change it to the implementation method you suggested.

@Koenkk Koenkk requested a review from Nerivec December 30, 2024 13:54
@Nerivec
Copy link
Collaborator

Nerivec commented Dec 30, 2024

Was this tested against the described problem?
Some devices are expected to not switch (bad design), some devices require activation to wake them up, then they switch after a few seconds (as described in docs).
I'd expect if this was required, all devices would fail to switch, which they do not (I've not found a device, even crappy Tuya, that doesn't switch with current implementation, on my end, several users also tested it and reported no issue).

From what I can tell, that quoted paragraph of the spec is for Upon receipt of a Mgmt_NWK_Unsolicited_Enhanced_Update_notify message, which this is not.

In the paragraph about the reception of nwk update req by a device, that value doesn't seem to matter.

3) If the ScanDuration parameter is equal to 0xfe, the message is a command to change channels. The device SHALL do the following.
  a) If the nwkNextChannelChange value in the NIB is non-zero, do the following. Compare the channel to change received over the air to the value in the NIB. If the values do not match, do the following:
    i) Follow the Error Response procedure setting the status to NOT_AUTHORIZED.
    ii) The request SHALL be dropped and no more processing SHALL take place.
  b) If there is more than 1 channel indicated in the ScanChannels bitmap (if the message is Mgmt_NWK_Update_req) or in the ScanChannelsListStructure (if the message is Mgmt_NWK_Enhanced_Update_req), then this is an invalid request. Do the following:
    i) Follow the Error Response procedure setting the status to INV_REQUESTTYPE.
    ii) The request SHALL be dropped and no more processing SHALL take place.
  c) The receiving device SHALL determine if the channel is one within the range of all supported channels.
    i) Examine the SupportedChannels element for each entry in the nwkMacInterfaceTable, and determine if there is a match within the received ScanChannels bitmap or ScanChannelsListStructure.
    ii) If no match is found, do the following:
      (1) Follow the Error Response procedure setting the status to INV_REQUESTTYPE.
      (2) The request SHALL be dropped and no more processing SHALL take place.
    iii) If a match is found, perform a channel change.
      (1) Execute a MLME-SET.request for the PIB value phyCurrentPage.
      (2) Execute a MLME-SET.request for the PIB value phyCurrentChannel.
      (3) No further processing SHALL be done.

As far as I can tell, the case where the nwkUpdateId would be used would be if a router decides to rejoin (for some reason), and it finds the same network on at least two different nodes, then it should pick the node with the higher nwkUpdateId (if any). But that scenario should not matter for a channel change, since routers receive the broadcast, and switch immediately. Leaving any offline router eventually powered back on, with only the new network available to rejoin.

The spec is a bit lacking on that nwkUpdateId, which I noted in the implementation a while back:

// TODO: What does "This value is set by the Network Channel Manager prior to sending the message." mean exactly??
// (isn't used/mentioned in EmberZNet, confirmed working if not set at all for channel change)
// for now, allow to bypass with undefined, otherwise should throw if undefined and duration passes below conditions (see NwkEnhancedUpdateRequest)
if (nwkUpdateId !== undefined && (duration === 0xfe || duration === 0xff)) {
this.writeUInt8(nwkUpdateId);
}

Short version: I don't expect it would solve the problem at hand?
I'm also a little worried about changing this considering the various stacks involved, because it was tested rather extensively during implementation, and after by several users and no particular issue was found that couldn't be attributed to devices being "too cheap". Hopefully, at worse, it does nothing... but I half expect that most devices just don't use the value at 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().mockReturnValue(mockDummyBackup);
Copy link
Collaborator

Choose a reason for hiding this comment

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

const mockAdapterBackup = vi.fn(() => Promise.resolve(mockDummyBackup));

should provide more accurate typing/testing (and fix typo).
(A lot of others also need refactoring, but might as well add new ones properly.)

@CubeZ2mDeveloper
Copy link
Author

I added the same device to Home Assistant, then changed the channel. I found that it can be controlled normally in Home Assistant, and I also discovered that Home Assistant will increase the value of nwkUpdateID.

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.

4 participants