search

zigpy / zigpy-cli

Command line interface for zigpy
GNU General Public License v3.0
44 stars 12 forks source link

Add Espressif System Zigbee radio library support #45

Open lhespress opened 6 months ago

lhespress commented 6 months ago

@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.

puddly commented 6 months ago

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.

lhespress commented 5 months ago

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

Hedda commented 5 months ago

@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

Maxwelltoo commented 4 months ago

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.

lhespress commented 4 months ago

@Maxwelltoo It's great if you can help.

Hedda commented 4 months ago

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

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

puddly commented 4 months ago

@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.