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

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

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

reidab
Copy link
Contributor

@reidab reidab commented Dec 13, 2021

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
Copy link

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%) ⬆️

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
Copy link
Collaborator

puddly commented Dec 13, 2021

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
Copy link
Contributor Author

reidab commented Dec 13, 2021

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.

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.

3 participants