Repository navigation
Fix UTIL.AssocAdd parameter order to match the firmware - #284
Conversation
zigpy-znp serialized `UTIL.AssocAdd` as NWK, IEEE, NodeRelation. Koenkk's `MT_UtilAssocAdd`, the firmware side of this command since build 20201026, reads the IEEE address first: `AssocAddNew(BUILD_UINT16(pBuf[8], pBuf[9]), pBuf, pBuf[10])`. zigbee-herdsman sends the same order (ieeeadr, nwkaddr, noderelation). With the old order the firmware took the NWK address plus the low six IEEE bytes as the IEEE address and the top two IEEE bytes as the NWK address, so the restore step of the `send_packet` recovery created a phantom child instead of re-adding the device it had just removed. Send IEEE, NWK, NodeRelation. Callers use keyword arguments and are unaffected.
| Src=0x821F, | ||
| Status=t.ZDOStatus.NOT_SUPPORTED, | ||
| ) | ||
|
|
There was a problem hiding this comment.
I don't think this needs unit testing, to be honest. This is a definition error and is fixed by the definition correction.
There was a problem hiding this comment.
Thanks, done: the added tests are removed in 18e135f. The device-free byte check stays in the description as evidence.
| ) | ||
|
|
||
| # Z2M firmware: proxy call to AssocAddNew | ||
| # Z2M firmware: proxy call to AssocAddNew. The IEEE address comes first: the |
There was a problem hiding this comment.
There's no need to update the comment to describe how the firmware works.
There was a problem hiding this comment.
Thanks, done: the comment is back to its original wording in 18e135f.
zigpy-review-bot
left a comment
There was a problem hiding this comment.
The new field order is correct. I checked the schema change against Koenkk's firmware and the new tests against the PR branch.
Verified
- Koenkk's
MT_UtilAssocAddincoordinator/Z-Stack_3.x.0/firmware.patch(current master) callsAssocAddNew(BUILD_UINT16(pBuf[Z_EXTADDR_LEN], pBuf[Z_EXTADDR_LEN + 1]), pBuf, pBuf[Z_EXTADDR_LEN + 2]), so the wire layout is IEEE (bytes 0-7), NWK (bytes 8-9), relation (byte 10). The new schema matches it. - zigbee-herdsman's
assocAdddefinition uses the same order: ieeeadr, nwkaddr, noderelation. - The only caller in the repo (
zigpy_znp/zigbee/application.py) and the request-comparing tests intests/application/test_requests.pyall use keyword arguments, so nothing else needs to change. - On the PR head (62b5932) the full suite passes: 393 passed.
- The new tests check the raw bytes against a parser written like the firmware, which the existing tests could not do because they build both sides from the same schema. That is the right way to pin this.
Nit (non-blocking)
The PR description still contains the literal placeholder #PR1 in several places (the intro, the compatibility table and the "Interaction with #PR1" section). It autolinks to nothing, so it would be good to replace it with the real PR number, or drop those references if that PR is not open yet.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #284 +/- ##
==========================================
- Coverage 98.60% 98.54% -0.06%
==========================================
Files 43 43
Lines 3581 3582 +1
==========================================
- Hits 3531 3530 -1
- Misses 50 52 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This PR corrects the field order of
UTIL.AssocAddto match Koenkk's Z-Stack firmware: IEEE, NWK, relation.The existing schema sends NWK before IEEE, so the firmware interprets parts of the two addresses as a different
address pair. zigbee-herdsman already uses this order in its
assocAddcommand definition.I found this while investigating IKEA remotes that became unreachable after association recovery under ZHA.
The recovery removes a child entry and, if the send still fails, tries to restore it. In the test network, that
restore returned
SUCCESSbut created an entry under a different IEEE and short address, leaving the real deviceabsent. I could predict the unexpected address pair from the request bytes and read it back from the table. A
second predicted pair also appears in my home network's zigpy backup; the detailed traces are below.
With the corrected byte order, the test coordinator finds the intended device under its IEEE and short address
again. This is a small serialization change: it preserves the recovery policy. The byte-level check is kept in this description as a device-free script (Evidence 3)
rather than as a unit test, following review feedback.
Successful delivery after restoration remains to be measured, and existing incorrect entries are not removed by
this change. The related lookup and restore checks are covered separately in #PR1.
Proposed change
The schema now matches the firmware; the only caller passes keyword arguments, so no call site changes.
Verification
devat34098a5pytest tests/test_commands.pypytest tests(full suite)pre-commit run --all-files(ruff, ruff-format, codespell, mypy)No new unit tests: a definition error is fixed by the definition correction. The device-free script in Evidence 3
pins the byte layout: against
devit prints the two phantom pairs, against this branch the intended pairs. Runlocally with Python 3.12 and zigpy 2.3.0.
How the mismatch occurs
zigpy-znp,
zigpy_znp/commands/util.pyL480-L494 at34098a5, unchanged sinceae0f446(2020-11-11):Koenkk's
MT_UtilAssocAdd,coordinator/Z-Stack_3.x.0/firmware.patchL269-L285 at5e4db67(build 20250321),identical since the command was added in
83a282f(build 20201026);Z_EXTADDR_LENis 8:zigbee-herdsman,
src/adapter/z-stack/znp/definition.tsL2444-L2455 atc1dcb11:A real request (test network, 2026-10-21 01:46:49:
NWK=0x725E, IEEE=68:0a:e2:ff:fe:32:db:95, CHILD_RFD):The same device from request to table, in Evidence 1. These are separate steps of one test-network session series, not
one atomic transaction: the tool soft-resets the coordinator at the start and end of every run, and a removal of the
phantom at 01:57:41 was undone by such a reset one second later.
68:0a:e2:ff:fe:32:db:95returns0xFFFE: no entry afterUTIL.AssocRemove5e 72 95 db 32 fe ff e2 0a 68 01e2:ff:fe:32:db:95:72:5e; NWK from bytes 8-9:0x680ASUCCESS; the real IEEE is still absent (01:46:49.137); entry0x680Aunder the misread IEEE (01:51:19, 01:52:41)95 db 32 fe ff e2 0a 68 5e 72 010x725E,CHILD_RFD(01:58:39.012); the phantom stays at index 0One possible explanation is the distinction between the C function and its MT command. The schema's comment
refers to
AssocAddNew, and the schema follows that function's parameter order (shortAddr, extAddr, nodeRelation).The MT handler expects a different wire order: IEEE first. That correspondence may explain how the mismatch arose,
although I have not confirmed its historical cause.
Several aspects make the mismatch easy to miss. This caller is reached when association recovery needs to restore
an entry after an unsuccessful send, and the handler returns
SUCCESSwithout reporting whether the intendedentry was created. The existing recovery tests compare request objects using the same command schema on both
sides, so they do not independently check the firmware's byte layout. Evidence 3 adds that check as a script.
Evidence
1. Test network: one zigpy-znp
AssocAddleaves the device out of the table and creates child0x680Aunder the misread IEEE, still present after two coordinator resets; the same add with pre-swapped fields creates the real child0x725E.Test coordinator with Koenkk's build 20250321, zigpy-znp 1.1.1, raw MT commands; log abridged, raw answers shortened.
2. Home network: the phantom pair (IEEE and NWK) predicted from a logged recovery request appears in the address table of the zigpy network backup.
3. Device-free check: serializes the request with zigpy-znp and parses it like
MT_UtilAssocAdd; ondevit reproduces both phantom entries, with this PR its assertion fails because the firmware reads the intended pair.Output on
dev(34098a5):5e 72 95 db 32 fe ff e2 0a 68 01 -> IEEE e2:ff:fe:32:db:95:72:5e, NWK 0x680A, relation 1,4b 43 9b 1b 70 fe ff 14 2e 84 01 -> IEEE 14:ff:fe:70:1b:9b:43:4b, NWK 0x842E, relation 1,OK. With this PR it prints95 db 32 fe ff e2 0a 68 5e 72 01 -> IEEE 68:0a:e2:ff:fe:32:db:95, NWK 0x725E, relation 1and fails.4. Test network: in this PR's byte order, an add for an IEEE the firmware already holds updates that entry in place, short address included; no duplicate.
Same coordinator and build, 2026-10-21, both adds with the fields pre-swapped as in Evidence 1; log abridged.
Compatibility and risk
UTIL.AssocAdd(0x64): Koenkk's Z-Stack 3.x.0 coordinator builds since 20201026 (checked at83a282fand5e4db67). Builds derived from his patch were not checked individually.UTIL.AssocRemoveexists,UTIL.AssocAdddoes not (Koenkk's changelog). The recovery removes the child; if the retry fails, the restore raisesCommandNotRecognizedin thefinallyblock and the child stays removed. Pre-existing and unchanged by this PR. zigbee-herdsman checks the two build dates separately.UTIL.AssocRemovefails withCommandNotRecognizedand the original error is raised, as before.AssocAddNewas "Create a new or update a previous association" (assoc_list.hL166); in the corrected order an add for an IEEE the firmware already holds rewrote that entry, short address included (Evidence 4, measured on build 20250321, one run each). Such a restore can overwrite a newer address. #PR1 checks for a changed host address and an existing firmware entry before restoration; its description also explains the remaining races after those checks.UTIL.AssocFindDeviceover all indices lists them.self._znp.version >= 3.30(application.pyL947).Interaction with #PR1
#PR1 adds checks for missing child entries (
0xFFFE) and for newer information available before restoration.Its description sets out the remaining concurrency limits. The two patches apply independently, but interact at runtime: with this PR alone, a recovery started from an
invalid lookup now sends the real IEEE with NWK
0xFFFEand relationPARENTto the firmware, instead of thepreviously misparsed bytes. What the firmware does with that entry was not measured. #PR1's tests mock the request
objects; this PR adds the independent byte-layout checks. Reviewing the two changes together would help assess both
when restoration is appropriate and how its command is encoded.
Prior art and references
34098a5:commands/util.pyL480-L494(schema),
zigbee/application.pyL1003-L1014(only caller, restore in
finally), L962-L975(
AssocRemove,CommandNotRecognized). Introduced on 2020-11-11 (schemaae0f446, caller65dae7b/0b04aec),first released in 0.3.0.
MT_UtilAssocAddL269-L285 at5e4db67(build 20250321);first version L123-L139 at
83a282f(build 20201026);CHANGELOG: 20200805
"Expose
AssocRemovefunction", 20201026"Expose
AssocAddfunction".lpf2-8.30.01.01:AssocAddNew(uint16_t shortAddr, byte *extAddr, byte nodeRelation), "Create a new or update a previous association",assoc_list.hL165-L169.c1dcb11:assocAdd,definition.tsL2444-L2455;supportsAssocRemove(20200805) andsupportsAssocAdd(20201026),zStackAdapter.tsL237-L243.Prepared with the help of an AI assistant (Claude); I have read the change and the tests and take responsibility
for them.