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 Espressif System Zigbee radio library support #45

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

lhespress
Copy link

No description provided.

@lhespress
Copy link
Author

lhespress commented Feb 5, 2024

@puddly #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
Copy link
Contributor

puddly commented Feb 5, 2024

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

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

@Hedda
Copy link

Hedda commented Mar 18, 2024

@lhespress nitpicking and maybe trivial, however as suggested by puddly in #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?

39f6642#diff-bb72967ef9d1e64ff68ad3f1a4e13e19b4cc7f05359c100016f76560fbaea52c

"znsp": "zigpy_espzb",

"znsp": [

@Maxwelltoo
Copy link

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

@Maxwelltoo It's great if you can help.

@Hedda
Copy link

Hedda commented Apr 15, 2024

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

puddly commented Apr 15, 2024

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

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.

4 participants