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

New radio API #848

Merged
merged 44 commits into from Jun 15, 2022
Merged

New radio API #848

merged 44 commits into from Jun 15, 2022

Conversation

puddly
Copy link
Collaborator

@puddly puddly commented Nov 10, 2021

This is a WIP implementation of #842. Database changes will come in a future PR.

Since we're going to have to edit every radio library to accommodate these changes anyways, I've renamed pre_shutdown to shutdown and also shortened NetworkInformation to NetworkInfo (and the associated state attributes). The rest of the changes more or less match what's in the linked RFC issue.

@Adminiuga
Copy link
Collaborator

Should we have an API for setting the manufacturer id ib the node descriptor? EZSP protocol has a command for it. ConBee serial protocol just added a command for it and ZNP would release (unless it already happened) a modification to firmware to allow setting the mfg id.

Right now the only unknown if it should be set permanently or dynamically.

Thoughts?

@dmulcahey for visibility

@puddly
Copy link
Collaborator Author

puddly commented Nov 15, 2021

Sure, I don't see why not. The deCONZ serial spec doesn't say what part of the node descriptor response is configurable so maybe it would allow zigpy to respond with the whole thing?

and ZNP would release (unless it already happened) a modification to firmware to allow setting the mfg id.

It's baked into the firmware, unfortunately: https://github.com/Koenkk/Z-Stack-firmware/blob/62692f2c3d06054eaa3a263ecda0b573fe60223e/coordinator/Z-Stack_3.x.0/firmware.patch#L525

@dmulcahey
Copy link
Collaborator

Should we have an API for setting the manufacturer id ib the node descriptor? EZSP protocol has a command for it. ConBee serial protocol just added a command for it and ZNP would release (unless it already happened) a modification to firmware to allow setting the mfg id.

Right now the only unknown if it should be set permanently or dynamically.

Thoughts?

@dmulcahey for visibility

should we make this an option in the UI in HA? Documented Flow could be something like:

  1. User sets mfg code to whatever it needs to be
  2. User pairs device
  3. User sets mfg code back to 0x0000

Or should we look for a way to make this dynamic similar to what Z2M did In the TI firmware so that users don't have to deal with it at all...

@Adminiuga
Copy link
Collaborator

Need to try what does setting the mfg id to 0x0000 on ezsp do. Does it actually sets to all zeroes or resets to the standard one.

@puddly
Copy link
Collaborator Author

puddly commented Nov 15, 2021

should we make this an option in the UI in HA?

Are there any other devices like these Aqara sensors? Seems like an awfully specialized user-facing setting.

Or should we look for a way to make this dynamic similar to what Z2M did In the TI firmware so that users don't have to deal with it at all...

What happens if we globally set an Aqara manufacturer ID on startup and never change it back to the stack default?

@dmulcahey
Copy link
Collaborator

should we make this an option in the UI in HA?

Are there any other devices like these Aqara sensors? Seems like an awfully specialized user-facing setting.

Or should we look for a way to make this dynamic similar to what Z2M did In the TI firmware so that users don't have to deal with it at all...

What happens if we globally set an Aqara manufacturer ID on startup and never change it back to the stack default?

I think it’s all E series Aqara sensors. I’m not sure what happens if we leave it…

@MattWestb
Copy link
Contributor

I think i have reading that some other device need having replay with there manufacture number for some requests for working OK but i dont remember winch device it was (I think it was in deCONZ form).

@Adminiuga
Copy link
Collaborator

This is what Deconz does. I guess they leave it up to the application to send the response?
dresden-elektronik/deconz-rest-plugin#5080 requires new firmware

@puddly
Copy link
Collaborator Author

puddly commented Nov 15, 2021

How do you feel about moving probing and the bellows baudrate override into ZHA?

RADIO_PROBE_CONFIGS = {
    bellows.zigbee.ControllerApplication: [{}, {zigpy.config.CONF_DEVICE_BAUDRATE: 115200}],
    zigpy_deconz.zigbee.ControllerApplication: [{}],
    zigpy_znp.zigbee.ControllerApplication: [{}],
    zigpy_zigate.zigbee.ControllerApplication: [{}],
    zigpy_xbee.zigbee.ControllerApplication: [{}],
}


async def find_compatible_radio(path: pathlib.Path | str) -> tuple[
    zigpy.application.ControllerApplication, ConfigType
]:
    for radio, overrides in RADIO_PROBE_CONFIGS.items():
        for override in overrides:
            config = {zigpy.config.CONF_DEVICE_PATH: str(path), **override}
            radio = ControllerApplication(config)

            try:
                await radio.connect()
            except Exception:
                LOGGER.debug("Failed to probe %s with %s", config, radio, exc_info=True)
            else:
                return radio, config
            finally:
                await radio.disconnect()
    else:
        raise ValueError("Failed to find compatible radio module for %s", path)

@Adminiuga
Copy link
Collaborator

How do you feel about moving probing and the bellows baudrate override into ZHA?

I don't have strong preference, the idea was that the "radio lib" would have specific knowledge to auto detect settings were applicable and then in this case should return the config back to ZHA. in other words ZHA is not supposed to know all possible radio configs, current of the future. The radio lib may poses that knowledge.

@puddly
Copy link
Collaborator Author

puddly commented Nov 15, 2021

That makes sense. My main goal was to remove unnecessary duplication between the radio libraries (all of them just connect/disconnect with the exception of bellows) so I guess something like this would work too as an override in the radio library:

@classmethod
async def probe(cls, device_config):
    result = await super().probe(device_config)

    if result:
        return result

    return await super().probe({**device_config, CONF_DEVICE_BAUDRATE: 115200})

On the other hand, each radio library may have different probe timeouts (e.g. ZNP's startup time is a lot longer than bellows) so maybe it's best to just leave it alone.

@coveralls
Copy link

coveralls commented Nov 16, 2021

Coverage Status

Coverage decreased (-0.1%) to 99.615% when pulling a1c654a on puddly:puddly/new-radio-settings-api into 69d5b00 on zigpy:dev.

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2021

Codecov Report

Merging #848 (a1c654a) into dev (69d5b00) will decrease coverage by 0.12%.
The diff coverage is 95.68%.

@@            Coverage Diff             @@
##              dev     #848      +/-   ##
==========================================
- Coverage   99.75%   99.62%   -0.13%     
==========================================
  Files          45       45              
  Lines        6802     6951     +149     
==========================================
+ Hits         6785     6925     +140     
- Misses         17       26       +9     
Impacted Files Coverage Δ
zigpy/state.py 95.32% <91.30%> (-3.26%) ⬇️
zigpy/application.py 98.71% <98.49%> (-0.11%) ⬇️
zigpy/config/defaults.py 100.00% <100.00%> (ø)
zigpy/exceptions.py 100.00% <100.00%> (ø)
zigpy/types/basic.py 100.00% <100.00%> (ø)
zigpy/types/named.py 100.00% <100.00%> (ø)
zigpy/util.py 100.00% <100.00%> (ø)

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 69d5b00...a1c654a. Read the comment docs.

@pipiche38
Copy link
Contributor

This is what Deconz does. I guess they leave it up to the application to send the response? dresden-elektronik/deconz-rest-plugin#5080 requires new firmware

If I can share what Zigate is doing, and that was a surprise when I saw 0000 when using the first ZNP one. Zigate is set to 0x1147.
The fix which has been developed is the same as the one from @koenk.

The best would be indeed to be able to set the manufacture code on the fly via an API. I would imagine to do it when receiving an handle_join() then I would set the manufacturer to whatever is needed so at the time the device query it, it will get what I put.

@puddly puddly force-pushed the puddly/new-radio-settings-api branch 2 times, most recently from 358e5ad to 75f494e Compare April 14, 2022 18:59
@pipiche38
Copy link
Contributor

Any plan or targeted date when it will be released ?

@Hedda
Copy link
Contributor

Hedda commented Apr 20, 2022

Any plan or targeted date when it will be released ?

FYI, puddy's GitHub project (kanban board) under zigpy org with progress overview -> https://github.com/orgs/zigpy/projects/2

Originally posted by @puddly in zigpy/zigpy-zigate#117 (comment)

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.

Copy link
Contributor

@pipiche38 pipiche38 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@puddly, while I understand Endpoint 0x01, what is the purpose of Ep: 0x02 ?

    await self.add_endpoint(
            zdo_types.SimpleDescriptor(
                endpoint=2,
                profile=zigpy.profiles.zll.PROFILE_ID,
                device_type=zigpy.profiles.zll.DeviceType.CONTROLLER,
                device_version=0b0000,
                input_clusters=[zigpy.zcl.clusters.general.Basic.cluster_id],
                output_clusters=[],
            )

@puddly puddly marked this pull request as ready for review June 9, 2022 15:15
@puddly
Copy link
Collaborator Author

puddly commented Jun 13, 2022

Alright, this PR is about as ready as it will ever be. Any final thoughts before I merge this and the corresponding PRs in all of the other radio libraries?

@SylvainPer
Copy link

SylvainPer commented Jun 13, 2022 via email

@pipiche38
Copy link
Contributor

I just did a re-run of my new-radio-API test bed updating it with your own repository and at least it is still working.

I do hope that a number of issues that we have on the legacy API will be solved, and the features that we are looking for deConz are available

Happy to test on the upstream dev branch

@dumpfheimer
Copy link

For what its worth: I just instelled zigpy, zigpy-znp and bellows from the new radio settings branches:
Everything seems to be working perfectly fine.

  1. I previously had issues with pairing new devices using a specific router (possible related to Permit to join on specific router ends in timeout zigpy-znp#148) which seem to have disappeared.
  2. Many/Most battery driven devices turned up "unavailable" until I pressed a button or they checked in by themselves. Also, I had to re-joined 3 or 4 remotes that were not used in a long time - not sure when they were lost. I believe those were the devices related to error messages I got ( Status.NWK_INVALID_REQUEST ). Not an issue, just wanted to mention it.
  3. One more random error message of a device that is behaving well: [zigpy.appdb] Error handling '_save_attribute' event with (a4:c1:38:8d:f0:52:aa:f9, 1, 0, 5, 'TS011F') params: FOREIGN KEY constraint failed

This might be a psychogical thing or maybe it is due to the fact that the network was offline for 30m but I feel like the network is more responsive and reliable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants