zigpy / zigpy-znp

TI CC2531, CC13x2, CC26x2 radio support for Zigpy and ZHA
GNU General Public License v3.0
145 stars 40 forks source link

Add ZDO converter for Mgmt_Bind_req; update return format to match zigpy expectations #110

Closed reidab closed 2 years ago

reidab commented 2 years ago

This adds a ZDO converter for the Mgmt_Bind_req request and fixes the response mapping back to zigpy. This request fetches the binding table from a device.

The field names in the existing ZDO command definition didn't match the expected names at https://github.com/zigpy/zigpy/blob/77dec68ab51a9205535a0b47ae2f6f27f27ec73f/zigpy/zdo/types.py#L664-L669

Ideally, I think this should follow the same pattern used for Neighbors and Routes, with a struct defined in zigpy for the binding table collection. However, since that involves coordinating PRs across both repos and could break compatibility, we'll stick with this smaller change.

I didn't add any tests for this, because none of the similar requests like Mgmt_Lqi_req or Mgmt_Rtg_req seemed to have test coverage. I'm happy to take a stab at adding tests for it if there's a pattern anyone can point me to for testing the ZDO converters.

codecov-commenter commented 2 years ago

Codecov Report

Merging #110 (3d6b8c4) into dev (d779a3d) will increase coverage by 0.10%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #110      +/-   ##
==========================================
+ Coverage   98.72%   98.83%   +0.10%     
==========================================
  Files          44       44              
  Lines        3852     3847       -5     
==========================================
- Hits         3803     3802       -1     
+ Misses         49       45       -4     
Impacted Files Coverage Δ
zigpy_znp/zigbee/zdo_converters.py 100.00% <ø> (ø)
zigpy_znp/commands/zdo.py 100.00% <100.00%> (ø)
zigpy_znp/zigbee/application.py 96.92% <0.00%> (+0.72%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d779a3d...3d6b8c4. Read the comment docs.

puddly commented 2 years ago

Thanks for the PR!

I think replacing the BindEntry type with the identically-defined one in zigpy is good, we already do this with the Neighbor type. Could you rename the MT ZDO command's parameters back to the way they originally were and just adjust the code within the ZDO converters module?

The ZDO.MgmtBindRsp command is internal to zigpy-znp and the naming conventions in the command modules try to follow the Z-Stack API documentation. See page 205 of the Z-Stack MT docs:

image image

There may be slight differences between Z-Stack's internal (re-)definitions of the ZDO types and zigpy's (e.g. a node descriptor being \x00 instead of empty when the status isn't SUCCESS) so zigpy and zigpy-znp may not always match up.

In the end, zigpy never sees any of the internal types because this roundabout translation layer re-serializes everything into bytes to ensure zigpy sees a "normal" ZDO request and can deserialize it into its own types.

reidab commented 2 years ago

Updated to use the internal Z-Stack field names in ZDO.MgmtBindRsp. I kept the changes from Index -> StartIndex and BindTable -> BindTableList since those didn't line up with the Z-Stack docs originally.