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

Implement the new zigpy radio API #117

Merged
merged 18 commits into from
Jun 26, 2022
Merged

Conversation

puddly
Copy link
Collaborator

@puddly puddly commented Feb 10, 2022

zigpy-zigate is the last radio library remaining.

Parent issues:

@pipiche38 I do not have a ZiGate so this code is untested beyond the unit tests. They pass with the modified version of zigpy.

@puddly puddly mentioned this pull request Feb 10, 2022
6 tasks
@puddly
Copy link
Collaborator Author

puddly commented Feb 10, 2022

Zigpy will be migrating to the simpler radio API in the very near future so it would be great if we can merge the new features in https://github.com/zigbeefordomoticz/zigpy-zigate back into zigpy/zigpy-zigate.

ZHA users will definitely benefit from broadcast support, since zigpy can't currently send a permit broadcast to all routers.

@pipiche38
Copy link

I give a try but it didn't work.

Feb 10 22:34:40 rasp domoticz[25092]:     self.run()
Feb 10 22:34:40 rasp domoticz[25092]:   File "/usr/lib/python3.9/threading.py", line 910, in run
Feb 10 22:34:40 rasp domoticz[25092]:     self._target(*self._args, **self._kwargs)
Feb 10 22:34:40 rasp domoticz[25092]:   File "/var/lib/domoticz/plugins/Domoticz-Zigbee/Classes/ZigpyTransport/zigpyThread.py", line 73, in zigpy_thread
Feb 10 22:34:40 rasp domoticz[25092]:     asyncio.run(
Feb 10 22:34:40 rasp domoticz[25092]:   File "/usr/lib/python3.9/asyncio/runners.py", line 44, in run
Feb 10 22:34:40 rasp domoticz[25092]:     return loop.run_until_complete(main)
Feb 10 22:34:40 rasp domoticz[25092]:   File "/usr/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
Feb 10 22:34:40 rasp domoticz[25092]:     return future.result()
Feb 10 22:34:40 rasp domoticz[25092]:   File "/var/lib/domoticz/plugins/Domoticz-Zigbee/Classes/ZigpyTransport/zigpyThread.py", line 91, in radio_start
Feb 10 22:34:40 rasp domoticz[25092]:     self.app = App_zigate(config)
Feb 10 22:34:40 rasp domoticz[25092]: TypeError: Can't instantiate abstract class App_zigate with abstract method shutdown

And that is not my part. Will see how to work on that except if you have any idea ...

@puddly
Copy link
Collaborator Author

puddly commented Feb 10, 2022

Zigpy was updated too. You will need to install modified version of zigpy and zigpy-znp to test it:

# May need to uninstall zigpy-znp, zigpy, and zigpy-zigate first
pip install \
    git+https://github.com/puddly/zigpy.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-znp.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-zigate.git@puddly/new-radio-api

@pipiche38
Copy link

pipiche38 commented Feb 10, 2022

Ok. I'll see what we can do, because it is breaking what we have done on fork() this will not be an easy step.
This is some work to do and we cannot do without merging our todays work. So for sure we are not going to test your PR.

@Hedda
Copy link
Contributor

Hedda commented Feb 11, 2022

I do not have a ZiGate so this code is untested beyond the unit tests. They pass with the modified version of zigpy.

@fairecasoimeme Do you know any ZiGate users of Home Assistant's ZHA integration (or the Jeedom plugin) who could test this?

@zoic21 Do you know any ZiGate users of Jeedom Official Zigbee Plugin who could test modified versions of zigpy and zigpy-znp?

As per #116 and zigpy/zigpy#880 there is currently no active developers or testers of https://github.com/zigpy/zigpy-zigate today.

Zigpy was updated too. You will need to install modified version of zigpy and zigpy-znp to test it:

# May need to uninstall zigpy-znp, zigpy, and zigpy-zigate first
pip install \
    git+https://github.com/puddly/zigpy.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-znp.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-zigate.git@puddly/new-radio-api

@Kratax
Copy link

Kratax commented Feb 11, 2022

I do not have a ZiGate so this code is untested beyond the unit tests. They pass with the modified version of zigpy.

@fairecasoimeme Do you know any ZiGate users of Home Assistant's ZHA integration (or the Jeedom plugin) who could test this?

@zoic21 Do you know any ZiGate users of Jeedom Official Zigbee Plugin who could test modified versions of zigpy and zigpy-znp?

As per #116 and zigpy/zigpy#880 there is currently no active developers or testers of https://github.com/zigpy/zigpy-zigate today.

Zigpy was updated too. You will need to install modified version of zigpy and zigpy-znp to test it:

# May need to uninstall zigpy-znp, zigpy, and zigpy-zigate first
pip install \
    git+https://github.com/puddly/zigpy.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-znp.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-zigate.git@puddly/new-radio-api

Hi, I can test the combination of zigate usb v1 and home assistant. Installed in docker on raspi4… just need some description how to do it and how to roll back if not working well…

@pipiche38
Copy link

I do not have a ZiGate so this code is untested beyond the unit tests. They pass with the modified version of zigpy.

@fairecasoimeme Do you know any ZiGate users of Home Assistant's ZHA integration (or the Jeedom plugin) who could test this?

@zoic21 Do you know any ZiGate users of Jeedom Official Zigbee Plugin who could test modified versions of zigpy and zigpy-znp?

As per #116 and zigpy/zigpy#880 there is currently no active developers or testers of https://github.com/zigpy/zigpy-zigate today.

Zigpy was updated too. You will need to install modified version of zigpy and zigpy-znp to test it:

# May need to uninstall zigpy-znp, zigpy, and zigpy-zigate first
pip install \
    git+https://github.com/puddly/zigpy.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-znp.git@puddly/new-radio-settings-api \
    git+https://github.com/puddly/zigpy-zigate.git@puddly/new-radio-api

What about bellows ?

@puddly
Copy link
Collaborator Author

puddly commented Feb 12, 2022

It's implemented as well: zigpy/bellows#445. See zigpy/zigpy-cli#2 for links to PRs for all of the other radio libraries.

The only change is adding start_network and disconnect, and then moving high-level network formation code into zigpy so radios don't have to implement form_network directly. Everything else stays the same.

The API is provisional so if you think it needs to be changed, this would be the time to make those changes relating to network formation (or maybe adding set_tx_power and set_led as standard methods?).

@zigbeefordomoticz
Copy link

zigbeefordomoticz commented Feb 12, 2022 via email

@puddly
Copy link
Collaborator Author

puddly commented Feb 12, 2022

This should in theory already be possible but I have not tested it with zigate:

# This is already done when the network is started
await self.load_network_info()

await self.write_network_info(
    node_info=self.node_info,
    network_info=self.network_info.replace(
        extended_pan_id=zigpy.types.EUI64.convert("aa:bb:cc:dd:11:22:33:44")
    ),
)

It is expected to work with any radio library implementing the new API.

@pipiche38
Copy link

It's implemented as well: zigpy/bellows#445. See zigpy/zigpy-cli#2 for links to PRs for all of the other radio libraries.

The only change is adding start_network and disconnect, and then moving high-level network formation code into zigpy so radios don't have to implement form_network directly. Everything else stays the same.

The API is provisional so if you think it needs to be changed, this would be the time to make those changes relating to network formation (or maybe adding set_tx_power and set_led as standard methods?).

So I'm afraid that I didn't understand your PR, and as your PR cannot be easily merged with our fork, I'll see how do it later when free time.

@doudz
Copy link
Collaborator

doudz commented Feb 13, 2022

Many tests are failing, could you please fix your code or fix tests ?

@puddly
Copy link
Collaborator Author

puddly commented Feb 13, 2022

This is a draft PR that makes zigpy-zigate compatible with the changes in zigpy/zigpy#848. All tests pass when the above branch of zigpy is installed.

@doudz
Copy link
Collaborator

doudz commented Feb 13, 2022

For information, I just get a spare Rpi 3, I will set up a permanent test platform for HA + zigate (with pizigate)

@pipiche38
Copy link

pipiche38 commented Feb 13, 2022 via email

@puddly
Copy link
Collaborator Author

puddly commented Feb 13, 2022

What's incompatible? The only breaking change is app.pan_id and similar properties becoming app.state.network_info.pan_id and pre_shutdown being renamed to shutdown. It's a fairly minimal change.

That being said, it's a draft PR. If you have concrete suggestions for changes before I merge the zigpy PR, let me know.

@zigbeefordomoticz
Copy link

zigbeefordomoticz commented Feb 13, 2022 via email

@puddly
Copy link
Collaborator Author

puddly commented Feb 13, 2022

I'm not sure I understand. Here are the exact changes:

  • startup is now handled by zigpy instead of the radio library.
  • form_network is now handled by zigpy instead of the radio library.
  • pre_shutdown has been renamed to shutdown.
  • update_network is gone, no radio library implemented it consistently.
  • write_network_info and load_network_info have been added.
  • self.pan_id, extended_pan_id, channel, ieee, etc. have been moved into self.state.network_info.* and .node_info.*.

I can re-add the properties that mimic the old network info properties like self.pan_id but otherwise what else is there to do to make it relatively backwards compatible?

@Hedda
Copy link
Contributor

Hedda commented Feb 14, 2022

Hi, I can test the combination of zigate usb v1 and home assistant. Installed in docker on raspi4… just need some description how to do it and how to roll back if not working well…

@Kratax FYI, a step-by-step guide is lacking but there is some indirectly related discussion about how to best test a fork in #104

The best would probably be if have the opportunity to set up a totally separate test environment with Home Assistant and ZHA on another computer (or another virtual machine) then perhaps move your ZiGate adapter saved a backup of your Zigbee network to file so have the option to restore (if you do not happen to have access to more than one ZiGate adapters)

@Hedda
Copy link
Contributor

Hedda commented Feb 14, 2022

What's incompatible? The only breaking change is app.pan_id and similar properties becoming app.state.network_info.pan_id and pre_shutdown being renamed to shutdown. It's a fairly minimal change.

I guess he means that would be nice for them if pre_shutdown and NetworkInformation would be listed deprecated when add new API but could still use old API in parallel until they get a chance to implement the new API properties? So think he is asked if could for example map pre_shutdown to shutdown and NetworkInformation to NetworkInfo so that they can use both for a while during a transition period in order to not break backwards compatibility in one single release?

I understand larger projects usually does so in order to give third-party applications using their library as dependencies a grace period. The benefit to third-party applications is that the breaking change is postponed but they still get notified that it is coming and old properties will be removed, however, the downside for upstream is that need to track deprecated changes and plan to remove them later.

@pipiche38
Copy link

What I mean is very simple, you bring new API without breaking backward compatibility.
I just tried now, to work on the new API, nor Zigate, nor ZNP works.

So I just gave up, and will continue to use the old radio Api for now until someone is able to do the required changes on our side.

@SylvainPer
Copy link

SylvainPer commented Feb 20, 2022

Hello @puddly ,
I encounter some problem with the load_network_info function (https://github.com/puddly/zigpy-zigate/blob/bc2e6a4ae13f572efca1714a3b355638474f6a6f/zigpy_zigate/zigbee/application.py#L62)
Seems that zigpy calls it with some argument and it's not managed.

Edit: removed the error

@puddly
Copy link
Collaborator Author

puddly commented Feb 23, 2022

You are correct, load_network_info(self, *, load_devices=False) -> None: is the correct signature.

@fairecasoimeme sent me a ZiGate USB (thank you!) so I will be able to fully test this PR when it arrives.

@pipiche38
Copy link

@puddly how would you like us to do for the PR ?
We can push straight what we have done, which can be brutal, or do you want us to break it down in small PRs ?

@puddly
Copy link
Collaborator Author

puddly commented Feb 28, 2022

@pipiche38 I'm fixing up a few problems with my changes to zigpy-zigate now that I can test it out. We can figure out a good way to merge https://github.com/zigbeefordomoticz/zigpy-zigate/ into zigpy once I can test it out with a live ZHA network.

To make testing easier (and so I can implement write_network_info), do you know of a way to read/write the ZiGate PDM at runtime with normal firmware? There is a E_SL_MSG_LOAD_PDM_RECORD_REQUEST = 0x0201 command but it doesn't seem to be implemented in my firmware.

@pipiche38
Copy link

pipiche38 commented Mar 1, 2022

@pipiche38 I'm fixing up a few problems with my changes to zigpy-zigate now that I can test it out. We can figure out a good way to merge https://github.com/zigbeefordomoticz/zigpy-zigate/ into zigpy once I can test it out with a live ZHA network.

great news ... thanks for the update

To make testing easier (and so I can implement write_network_info), do you know of a way to read/write the ZiGate PDM at runtime with normal firmware? There is a E_SL_MSG_LOAD_PDM_RECORD_REQUEST = 0x0201 command but it doesn't seem to be implemented in my firmware.

I have no clue, for me the firmware and NXP stack is very close and don'' have much access. Even the PDM structure is not known, and we face some corruption issue that cannot be troubleshoot. Please check with @fairecasoimeme who is the firmware provider for the ZiGate

@pipiche38
Copy link

#120

@puddly
Copy link
Collaborator Author

puddly commented Mar 23, 2022

I'm not looking to significantly rewrite the zigpy-zigate library in this PR, only to make it compatible with the new radio API and fix whatever bugs exist that prevent a network from at least functioning.

At the moment the only task left is to write unit tests for the bellows changes and to runtime test zigpy-xbee. Once that is done and the PRs are merged, we can deal with fixing zigpy-zigate's request methods.

@Hedda Hedda mentioned this pull request Apr 20, 2022
@pipiche38
Copy link

@puddly , I found that https://github.com/fairecasoimeme/ZiGate/blob/5a64452e3cc4f32afe217a332d0c9b1d97e17d84/ModuleRadio/Firmware/src/ZiGate/Source/ZigbeeNodeControlBridge/app_start.c#L1191 which can be interested for you as regards to a Backup/Restore solution.
If we take in consideration this procedure is used when the PDM size is changed during a firmware upgrade, we might think that can be used for Backup and then Restore.

@puddly puddly marked this pull request as ready for review June 19, 2022 21:16
@puddly puddly requested a review from doudz June 21, 2022 21:18
@puddly
Copy link
Collaborator Author

puddly commented Jun 24, 2022

@doudz Could you make a release of zigpy-zigate incorporating this PR? Or, if you're okay with it, I can make a 0.9.0 release?

I'm hoping to get the new radio library changes into HA's upcoming 2022.7.0 beta.

@doudz
Copy link
Collaborator

doudz commented Jun 26, 2022

Hi!
I think you should make the release yourself, I haven't followed the latest changes ;)

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.

7 participants