zigpy / bellows

A Python 3 project to implement EZSP for EmberZNet devices
GNU General Public License v3.0
177 stars 86 forks source link

fixed missing length of array in EZSP setSourceRoute from V8 onward #607

Closed dumpfheimer closed 6 months ago

dumpfheimer commented 6 months ago

Happy new year, guys!

I might be mistaken, or might have missed an internal mechanism that sends array lengths, but I am somewhat confident that there is a bug in the setSourceRoute function from EZSP version 8 onward.

This is the code from Gecko SDI: https://github.com/SiliconLabs/gecko_sdk/blame/gsdk_4.3/protocol/zigbee/app/util/ezsp/command-functions.h#L1718-L1734

This is the method signature:

EmberStatus ezspSetSourceRoute(
  EmberNodeId destination,
  uint8_t relayCount,
  uint16_t *relayList)

I believe the length of the array is missing. This PR would fix that.

codecov[bot] commented 6 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d3ba62e) 99.73% compared to head (cb52969) 99.69%.

Files Patch % Lines
bellows/zigbee/application.py 60.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #607 +/- ## ========================================== - Coverage 99.73% 99.69% -0.04% ========================================== Files 74 74 Lines 4987 4990 +3 ========================================== + Hits 4974 4975 +1 - Misses 13 15 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

puddly commented 6 months ago

Thanks for finding the undocumented setSourceRoute still hiding in EZSP!

The LVList type is used here specifically so that you don't need to keep track of the number of elements in the list: it prepends the length when serialized:

https://github.com/zigpy/bellows/blob/d3ba62eca9a758090089086ff5db088d72f19c19/bellows/types/basic.py#L128-L131

dumpfheimer commented 6 months ago

Ah, nice! Well, that's what "reasonably confident" is worth, I guess.

Are list types automatically wrapped as LVList? Or is there a potential danger of passing a "normal" list to _set_source_route and then the header would be missing?

For example: Can I call _set_source_route like this? (from application.py, I would expect the packet to 0x0001 to be sent over 1 hop (=0x0002))

self._set_source_route(0x0001, [0x0002])

Or do I need to wrap it in an LVList, something like this? self._set_source_route(0x0001, LVList([0x0002]))

puddly commented 6 months ago

Are list types automatically wrapped as LVList?

Yep, any command you send will be serialized according to the schema and the arguments will be coerced to the correct type.

I wasn't sure if the re-introduction of source routing was doing anything so I did confirm the changes with Wireshark and the source routing bit is indeed flipped and our manual routes are present in the packet 🎉!

dumpfheimer commented 6 months ago

Perfect! Thanks for the explanation! And sorry for the unnecessary PR.