Fix xncp_set_manual_source_route keyword arguments#715
Conversation
The send-packet path called `xncp_set_manual_source_route` with `nwk=`/ `relays=` (the kwargs of the sibling `set_source_route`), but the XNCP wrapper takes `destination=`/`route=`. On firmware that advertises the `MANUAL_SOURCE_ROUTE` feature with manual source routing enabled, this raised `TypeError` instead of installing the route. The existing test mocked the method against the wrong spec (`set_source_route`), so the bad kwargs validated and the bug went unnoticed. Re-spec the mock against `xncp_set_manual_source_route` so the assertion checks the real signature.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #715 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 61 61
Lines 4147 4147
=======================================
Hits 4128 4128
Misses 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Approve — confirmed root cause and fix.
The xncp_set_manual_source_route wrapper (bellows/ezsp/__init__.py:776) has the signature (self, destination: t.NWK, route: list[t.NWK]), while the call site in send_packet was passing nwk= / relays= — those are the kwargs of the sibling set_source_route EZSP command three lines below. On any firmware advertising MANUAL_SOURCE_ROUTE with manual_source_routing enabled in the bellows config, this raised TypeError: missing a required argument: 'destination' (verified locally via inspect.signature(...).bind(...)). So the PR body is correct: manual source routing has been broken since it was introduced in #611 (merged 2024-11-09, 06eb3d92), where both the wrapper and the wrong call site landed in the same commit.
The test fix is the load-bearing half of this PR. The previous spec, app._ezsp._protocol.set_source_route, accepts nwk/relays, so assert_called_once_with(nwk=..., relays=...) passed against the bad code. Reverting bellows/zigbee/application.py against the new test gives the expected Expected: mock(nwk=...) / Actual: mock(destination=...) failure — i.e. the test now actually guards the bug.
Wire format unchanged: SetSourceRouteReq(destination=destination, source_route=route) serializes the same two NWK fields regardless of Python kwarg names, and matches the NabuCasa firmware decoder in xncp_common_extension/src/xncp_common_commands.c (payload[0..1] = destination NWK, then relay NWKs).
No external callers: GitHub code search for xncp_set_manual_source_route returns only zigpy/bellows itself, so renaming the call-site kwargs isn't a behavior change for anyone downstream — the wrapper's public signature is unchanged.
Full test suite (424 tests) passes locally on Py 3.14. Re: the TODO in the description — yes, the firmware side ought to be tested end-to-end at some point, but that's out of scope for a minimal one-line fix.
|
|
||
| app._ezsp.xncp_set_manual_source_route = AsyncMock( | ||
| return_value=None, spec=app._ezsp._protocol.set_source_route | ||
| return_value=None, spec=app._ezsp.xncp_set_manual_source_route |
There was a problem hiding this comment.
Good catch — this spec swap is what makes the rest of the diff meaningful. _protocol.set_source_route accepts nwk/relays, so assert_called_once_with(nwk=..., relays=...) validated cleanly against the bad code and the bug slipped through review of #611. With the spec now bound to the real wrapper signature (destination/route), the assertion below actually exercises the kwarg-passing pathway.
2 participants
Proposed change
This makes manual source routing* work for the first time ever...? 😅 (by fixing the names of the provided kwargs).
(*: It's only supported on certain firmware builds and requires the
manual_source_routingoption to be turned on in thebellowssection. This is not really related to the normalsource_routingoption.)Additional information
Firmware extensions (as well as the bug) were added with:
TODO
We should probably test the firmware side? Was that ever tested then?
AI summary
The send-packet path called
xncp_set_manual_source_routewithnwk=/relays=(the kwargs of the siblingset_source_route), but the XNCP wrapper takesdestination=/route=. On firmware that advertises theMANUAL_SOURCE_ROUTEfeature with manual source routing enabled, this raisedTypeErrorinstead of installing the route.The existing test mocked the method against the wrong spec (
set_source_route), so the bad kwargs validated and the bug went unnoticed. Re-spec the mock againstxncp_set_manual_source_routeso the assertion checks the real signature.