zigpy / zigpy-znp

TI CC2531, CC13x2, CC26x2 radio support for Zigpy and ZHA
GNU General Public License v3.0
145 stars 40 forks source link

Replace internal network state objects with `zigpy.state` #85

Closed puddly closed 2 years ago

puddly commented 3 years ago

The only feature zigpy.state lacks right now is a way to keep track of device NWK and IEEE addresses, like in https://github.com/zigpy/open-coordinator-backup/blob/main/samples/z2m-sample-1.json#L53-L56.

Once that is done, the entire Z-Stack coordinator state can be fully represented by the new zigpy.state types. While this is not strictly necessary with https://github.com/zigpy/zigpy-znp/pull/73, I've not yet tested the side effects of restoring a network backup with the child table completely erased.

My idea is to have zigpy behave something like this in the future and requiring radio libraries to implement only load_network_info and update_network_info:

async def form_network(self, *, force=False):
    # Or maybe do this inside of `startup()`?
    try:
        await self.load_network_info()
    except ValueError:
        pass

    # Do nothing if the `nwk_update_id` has not changed
    if not force and self.state.network_information.nwk_update_id == self.config[NWK_UPDATE_ID]:
        return

    # Otherwise, sync the network settings with what's in the zigpy config
    # XXX: how do you synchronize YAML config changes with ones done within ZHA's UI???
    await self.update_network_info()
codecov-commenter commented 3 years ago

Codecov Report

Merging #85 (c5ce329) into dev (6bcd7f4) will increase coverage by 0.04%. The diff coverage is 96.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #85      +/-   ##
==========================================
+ Coverage   98.71%   98.75%   +0.04%     
==========================================
  Files          44       44              
  Lines        3727     3768      +41     
==========================================
+ Hits         3679     3721      +42     
+ Misses         48       47       -1     
Impacted Files Coverage Δ
zigpy_znp/tools/common.py 100.00% <ø> (ø)
zigpy_znp/types/structs.py 100.00% <ø> (ø)
zigpy_znp/znp/security.py 94.24% <94.02%> (+1.54%) :arrow_up:
zigpy_znp/api.py 98.17% <95.20%> (-1.83%) :arrow_down:
zigpy_znp/tools/network_backup.py 97.95% <95.83%> (-2.05%) :arrow_down:
zigpy_znp/const.py 100.00% <100.00%> (ø)
zigpy_znp/tools/network_restore.py 100.00% <100.00%> (ø)
zigpy_znp/types/named.py 100.00% <100.00%> (ø)
zigpy_znp/zigbee/application.py 96.53% <100.00%> (+0.62%) :arrow_up:

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 6bcd7f4...c5ce329. Read the comment docs.

Adminiuga commented 3 years ago

I was on the fence whether to keep the NWK address of the node for key. Is this device list actually for the devices, like "children" or is this the list of "APS Link keys" for APS encrypted communication? If for later, the https://github.com/zigpy/zigpy/blob/fb71d982bfff6bcd96c4c046c8637295802d190f/zigpy/state.py#L59 the Key has partner_ieee, similar to APS key table structure ezsp keeps internally. in other words, for the key table an attempt was made to keep the bare minimum if the purpose of this table is to store APS Link Keys. Let me know what you think

puddly commented 3 years ago

Is this device list actually for the devices, like "children" or is this the list of "APS Link keys" for APS encrypted communication?

It's a list of both, which is ambiguous now that I think about it (i.e one can't distinguish a child with an APS link key from a non-child).

I'm currently populating the key_table as you described (https://github.com/zigpy/zigpy-znp/pull/85/files#diff-963ec1db33f28713991ce851c9e14363cf2422d7ab7c68b4acd73ab35475549cR142) but to represent the entire backup state, we'd need some way to keep track of children. Z-Stack's device tables require both the IEEE and NWK addresses (but I think EmberZNet only really needs the IEEE?).

What about keeping a list of devices like this?

@dataclass
class NetworkDevice:
    ieee: t.EUI64
    nwk: t.NWK | None  # I'll have to test how to get Z-Stack to deal with these
    key: Key | None
    is_child: bool
Adminiuga commented 3 years ago

does ZNP restoration need children or FFDs too?

What about keeping a list of devices like this?

I think that would work. Any suggestions how to call the list of these in app state? devices is a bit ambiguous, cause I don't want to keep all devices on the network there. neighbor_list maybe?

puddly commented 3 years ago

does ZNP restoration need children or FFDs too?

The only devices present in my Z-Stack coordinator backup are:

So it looks like only devices with APS link keys and RFD children. They all require NWK and IEEE addresses.

Any suggestions how to call the list of these in app state?

Since a device can be a "partner" with a key and/or a child, maybe immediate_family 😄? What about having two lists, one for children (with or without keys), and another for just partner devices with only keys? That would remove the need for the is_child attribute. On the other hand, that attribute is useful to have directly attached to the network info device object...

Maybe related_devices? partners? I'm okay with just devices and a comment explaining that it's not every single device.

Adminiuga commented 3 years ago

Yeah, i think having two different lists is a better approach

puddly commented 2 years ago

I've almost got this working with ZNP. form_network is independent of Z-Stack and can be moved into zigpy if other radio libraries replace form_network with write_network_info and load_network_info:

    async def form_network(self):
        """
        Clears the current config and forms a new network with a random network key,
        PAN ID, and extended PAN ID.
        """

        channel = self.config[conf.CONF_NWK][conf.CONF_NWK_CHANNEL]
        channels = self.config[conf.CONF_NWK][conf.CONF_NWK_CHANNELS]
        pan_id = self.config[conf.CONF_NWK][conf.CONF_NWK_PAN_ID]
        extended_pan_id = self.config[conf.CONF_NWK][conf.CONF_NWK_EXTENDED_PAN_ID]
        network_key = self.config[conf.CONF_NWK][conf.CONF_NWK_KEY]

        if pan_id is None:
            pan_id = random.SystemRandom().randint(0x0000, 0xFFFE + 1)

        if extended_pan_id is None:
            extended_pan_id = ExtendedPanId(os.urandom(8))

        if network_key is None:
            network_key = t.KeyData(os.urandom(16))

        # Override `channels` with a single channel if one is explicitly set
        if channel is not None:
            channels = t.Channels.from_channel_list([channel])

        network_info = zigpy.state.NetworkInformation(
            extended_pan_id=extended_pan_id,
            pan_id=pan_id,
            nwk_update_id=self.config[conf.CONF_NWK][conf.CONF_NWK_UPDATE_ID],
            nwk_manager_id=0x0000,
            channel=channel,
            channel_mask=channels,
            security_level=5,
            network_key=zigpy.state.Key(
                key=network_key,
                tx_counter=0,
                rx_counter=0,
                seq=0,
                partner_ieee=None,
            ),
            tc_link_key=None,
            key_table=[],
            neighbor_table=[],
            stack_specific={"zstack": {"tclk_seed": os.urandom(16).hex()}},
        )

        node_info = zigpy.state.NodeInfo(
            nwk=0x0000,
            ieee=None,
            logical_type=zdo_t.LogicalType.Coordinator,
        )

        await self.write_network_info(network_info=network_info, node_info=node_info)

Thoughts?

puddly commented 2 years ago

The main problem with this general approach are radio quirks:

Adminiuga commented 2 years ago

I like the idea.

Is this a firmware limitation? Is it possible to change this behavior?

Not really possible to change this. Has to do with the flash page erasing

Need to check deconz, IIRC you can update ieee address in deCONZ, but couldn't find anything in official documentation.

Zigate -- need to check this one.

puddly commented 2 years ago

Updated to work with zigpy@dev.

At this point I'm only converting to JSON for presentation, all other operations use the zigpy state objects:

Once a zigpy point release is made, we can:

Adminiuga commented 2 years ago

Do you need just zigpy release or pushed to HA too?

puddly commented 2 years ago

Doesn't really matter, updated versions of both packages will probably go in the same PR to HA core.

puddly commented 2 years ago

Sorry, I think I misread your comment. Just a zigpy release, so I can specify the minimum version in setup.py.