zigpy-cli icon indicating copy to clipboard operation
zigpy-cli copied to clipboard

Add Espressif System Zigbee radio library support

Open lhespress opened this issue 1 year ago • 8 comments

lhespress avatar Feb 05 '24 11:02 lhespress

@puddly https://github.com/zigpy/zigpy-cli/pull/42 have been closed and PTAL this PR for the new name. the zigpy-espzb project on my personal account https://github.com/lhespress/zigpy-espzb. Thanks.

lhespress avatar Feb 05 '24 11:02 lhespress

Thank you!

I've made a pull request that fixes CI in your repository. Once that is tested and merged, I can write some instructions for how to create a package release and publish it to PyPI. You can currently see that CI is failing for this pull request because the zigpy-espzb package is not on PyPI.

puddly avatar Feb 05 '24 15:02 puddly

@puddly @Hedda I have published zigpy-espzb to PyPI, please check, thanks. BTW, also update pyproject.toml for "zigpy-espzb>=0.0.1".

lhespress avatar Mar 18 '24 01:03 lhespress

@lhespress nitpicking and maybe trivial, however as suggested by puddly in https://github.com/zigpy/zigpy-cli/pull/42 perhaps would it not be a a good idea before this is established if also the new radio type was renamed from znsp to either espznsp or espzb instead to it both has "esp" in the name and better match the name of the library so that it does not sound too similar to existing znp radio type that is used by the zigpy-znp radio library?

Perhaps just my dyslexia but when I am reading znsp and znp as block words then they look much too similar to me at a glance. It will be easy for new users to choose wrong protocol by mistake. So at least I think that might make it a little less likely that the radio type name is accidentally confused with znp by users, or what do you think?

https://github.com/zigpy/zigpy-cli/pull/45/commits/39f6642aed66bea1ba5145e5c587655f8fae57b6#diff-bb72967ef9d1e64ff68ad3f1a4e13e19b4cc7f05359c100016f76560fbaea52c

https://github.com/zigpy/zigpy-cli/blob/39f6642aed66bea1ba5145e5c587655f8fae57b6/zigpy_cli/const.py#L17

https://github.com/zigpy/zigpy-cli/blob/39f6642aed66bea1ba5145e5c587655f8fae57b6/zigpy_cli/const.py#L62

Hedda avatar Mar 18 '24 08:03 Hedda

Hi @lhespress , I’ve noticed that the PR has been sitting idle for a bit. I think @Hedda ’s suggestion is reasonable, everything looks good, we just need to change the name to make it more distinctive.

I am currently trying to use the ZHA integration on an ESP32C6 with the ESP NCP firmware, the work you’ve done is amazing and helpful, so I think we need to complete this PR.

If you don’t have the time, I’d be happy to step in and take care of the remaining tasks.

Maxwelltoo avatar Apr 09 '24 03:04 Maxwelltoo

@Maxwelltoo It's great if you can help.

lhespress avatar Apr 09 '24 03:04 lhespress

@lhespress Do you also plan on prioritizing writing/adding additonal unit tests in this radio library for zigpy?

  • https://github.com/lhespress/zigpy-espzb/issues/12

For reference please see the tests directory for the bellows, zigpy-znp, and zigpy-deconz radio libraries:

https://github.com/zigpy/bellows/tree/dev/tests

https://github.com/zigpy/zigpy-znp/tree/dev/tests

https://github.com/zigpy/zigpy-deconz/tree/dev/tests

Hedda avatar Apr 15 '24 07:04 Hedda

@Hedda You already posted this as an issue, there's no need to duplicate it here. Furthermore, the library is not even functional yet so it is way too early to require unit tests.

Please reduce the chatter. You've opened nearly two thirds of the 30 issues in the zigpy-zboss repository. For the zigpy-espzb repo, 9/10 are yours. This is too much.

puddly avatar Apr 15 '24 21:04 puddly