Closed puddly closed 2 years ago
Merging #109 (4edfb13) into dev (d37ac29) will decrease coverage by
0.09%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## dev #109 +/- ##
==========================================
- Coverage 98.76% 98.66% -0.10%
==========================================
Files 44 44
Lines 3882 3907 +25
==========================================
+ Hits 3834 3855 +21
- Misses 48 52 +4
Impacted Files | Coverage Δ | |
---|---|---|
zigpy_znp/api.py | 97.55% <100.00%> (-0.28%) |
:arrow_down: |
zigpy_znp/commands/zdo.py | 100.00% <100.00%> (ø) |
|
zigpy_znp/zigbee/application.py | 95.85% <100.00%> (-0.76%) |
:arrow_down: |
zigpy_znp/zigbee/device.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 d37ac29...4edfb13. Read the comment docs.
@Adminiuga I'm trying to find a clean zigpy equivalent to the following ZNP-specific code that discovers the IEEE address of an unknown device:
nwk = 0x1234 # The NWK address of an unknown device
ieee_addr_rsp = await self._znp.request_callback_rsp(
request=c.ZDO.IEEEAddrReq.Req(
NWK=nwk,
RequestType=c.zdo.AddrRequestType.SINGLE,
StartIndex=0,
),
RspStatus=t.Status.SUCCESS,
callback=c.ZDO.IEEEAddrRsp.Callback(partial=True, NWK=nwk),
timeout=5,
)
Since we don't have a Device
object, await device.zdo.IEEE_addr_req(...)
won't work unless the radio library creates a temporary device object to send the request, receive the response, and get deleted. It's really hacky but it works.
Similar code in bellows
directly uses ezsp.lookupEui64ByNodeId
.
Any thoughts on how to implement this in zigpy (if it even belongs there)? Maybe a module-level function like zigpy.zdo.broadcast
?
@puddly , to discovers the IEEE address of unknown device, don't you think that the best is to rely on the Neighbour table from all routers ? From my end this is what I came to a conclusion as this is the only way for end device (which are not listen on idle)
Good idea, I did not think to use app.topology
!
Once I figure out a way to make the device discovery code independent of zigpy-znp, I think this would be a good way to avoid sending IEEE_addr_req
when it isn't necessary.
Basically here is the way I'm doing.
If I get an unknown shortId, then I look to the latest topology report that I have in house. If I'm not able to get an IEEE (basically the shortId is not found), then I trigger a new report and drop that message.
This mean that the message is drop for now, but when I'll get an other one from the same device, I might be able to reconnect and get the device back known.
@puddly, I did some test with the broadcast, and while the broadcast is going through, it looks like it is not a correct format Let me know if you would like me to do more test
This is the working paquet with the mainstream version (original)
2022-01-11 20:34:25,769 INFO : [ MainThread] zdp_raw_nwk_update_request - [00] ffff Queue Length: 0
2022-01-11 20:34:25,774 INFO : [ MainThread] sendData - [0] RAW-COMMAND {'Profile': 0, 'Cluster': 56, 'TargetNwk': 65533, 'TargetEp': 0, 'SrcEp': 0, 'Sqn': 0, 'payload': '0000001000fe000000', 'AddressMode': 2} 65533 Queue Length: 0
2022-01-11 20:34:25,776 DEBUG : [ MainThread] zigate_get_nwk_state
2022-01-11 20:34:25,778 INFO : [ MainThread] sendData - [None] REQ-NWK-STATUS None None Queue Length: 1
2022-01-11 20:34:25,813 DEBUG :Intercepted a ZDO request: dst_addr=AddrModeAddress(mode=<AddrMode.Broadcast: 15>, address=0xFFFD), dst_ep=0, src_ep=0, cluster=56, sequence=14, options=TransmitOptions.NONE, radius=48, data=b'\x00\x00\x00\x10\x00\xfe\x00\x00\x00'
2022-01-11 20:34:25,818 DEBUG :Intercepted AP ZDO request 56({'NwkUpdate': NwkUpdate(ScanChannels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, nwkUpdateId=0)}) and replaced with ZDO.MgmtNWKUpdateReq.Req(Dst=0xFFFD, DstAddrMode=<AddrMode.Broadcast: 15>, Channels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, ScanCount=0, NwkManagerAddr=0x0000)
2022-01-11 20:34:25,821 DEBUG :Sending request: ZDO.MgmtNWKUpdateReq.Req(Dst=0xFFFD, DstAddrMode=<AddrMode.Broadcast: 15>, Channels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, ScanCount=0, NwkManagerAddr=0x0000)
2022-01-11 20:34:25,838 DEBUG :Received command: ZDO.MgmtNWKUpdateReq.Rsp(Status=<Status.SUCCESS: 0>)
This is the non-working paquet with your zdo-refactor version (this current PR)
2022-01-11 20:28:16,791 INFO : [ MainThread] zdp_raw_nwk_update_request - [00] ffff Queue Length: 0
2022-01-11 20:28:16,794 INFO : [ MainThread] sendData - [0] RAW-COMMAND {'Profile': 0, 'Cluster': 56, 'TargetNwk': 65533, 'TargetEp': 0, 'SrcEp': 0, 'Sqn': 0, 'payload': '0000001000fe000000', 'AddressMode': 2} 65533 Queue Length: 0
2022-01-11 20:28:16,799 DEBUG :Sending request: AF.DataRequestExt.Req(DstAddrModeAddress=AddrModeAddress(mode=<AddrMode.Broadcast: 15>, address=0xFFFD), DstEndpoint=0, DstPanId=0x0000, SrcEndpoint=0, ClusterId=56, TSN=20, Options=<TransmitOptions.NONE: 0>, Radius=48, Data=b'\x00\x00\x00\x10\x00\xFE\x00\x00\x00')
I think this may be a bug with your code because the only change is that now zigpy-znp is sending exactly the bytes that you provide it.
The below zigpy code:
await zigpy.zdo.broadcast(
app,
zdo_t.ZDOCmd.Mgmt_NWK_Update_req,
0x0000, # group id (ignore)
0, # radius
zdo_t.NwkUpdate(
ScanChannels=t.Channels.from_channel_list([20]),
ScanDuration=zdo_t.NwkUpdate.CHANNEL_CHANGE_REQ,
nwkUpdateId=1,
),
broadcast_address=t.BroadcastAddress.ALL_ROUTERS_AND_COORDINATOR,
)
Produces the following packet:
AF.DataRequestExt.Req(
DstAddrModeAddress=AddrModeAddress(mode=<AddrMode.Broadcast: 15>, address=0xFFFC),
DstEndpoint=0,
DstPanId=0x0000,
SrcEndpoint=0,
ClusterId=56,
TSN=6,
Options=<TransmitOptions.NONE: 0>,
Radius=0,
Data=b'\x06\x00\x00\x10\x00\xFE\x01'
)
I'm not sure where those two trailing 00 00
bytes are coming from in your packet capture but it's not zigpy.
Any thoughts on how to implement this in zigpy (if it even belongs there)? Maybe a module-level function like zigpy.zdo.broadcast?
Yeah, there's no clean way to do it. And currently zigpy relies on existing and initialized Device
instance, even if ZDO frame and at least the ZCL header frame could be deserialized without having the device to be fully known/existing. I looked into refactoring it, but did not like how it looked so abandoned it.
Since we don't have a Device object, await device.zdo.IEEE_addr_req(...) won't work unless the radio library creates a temporary device object to send the request, receive the response, and get deleted. It's really hacky but it works.
I think this was one of the approaches tried in the past and yes it is hacky 🤷 So we can't store temporary device or devices with an unknown IEEE or NWK directly in zigpy (well, we could store devices with unknown NWK until NWK gets discovered, but that would may break or return an incorrect device if we self.get_device(nwk=0xFFFE)
(or whatever the unknown nwk address is)
Should we have a list in zigpy of "ephemeral" devices, for which we currently don't know IEEE or NWK and extend self.get_device()
such that it searches the self._devices
dict and if not found then searches the ephemeral devices list?
@puddly you are totally right, this was on my end. sorry for that. Will test today against your branch and will let you know, but I'm quiet optimistic
@puddly so the frame is now correct and is pretty similar to the one sent with the current mainstream. However it looks like the coordinator is not acting as when I request network info later, it is still on the previous channel.
Can it be that we broadcast the message, but the controller doesn't do the move ?
@puddly I was trying to send the Channel update to the controller. My idea was when we do the broadcast most-likely the controller doesn't see. And I got this message , No handler for ZDO
2022-01-12 15:01:05,764 DEBUG :Received command: ZDO.MsgCbIncoming.Callback(Src=0x0000, IsBroadcast=<Bool.false: 0>, ClusterId=56, SecurityUse=0, TSN=0, MacDst=0x0000, Data=b'\x00\x00\x10\x00\xFE\x01')
2022-01-12 15:01:05,781 DEBUG :[0x0000:zdo] ZDO request ZDOCmd.Mgmt_NWK_Update_req: [NwkUpdate(ScanChannels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, nwkUpdateId=1)]
2022-01-12 15:01:05,783 DEBUG :[0x0000:zdo] No handler for ZDO request:ZDOCmd.Mgmt_NWK_Update_req([NwkUpdate(ScanChannels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, nwkUpdateId=1)])
This message is fine, as zigpy does not handle those requests. I'd assume ZStack should take care of it? Did it change the channel?
Z-Stack doesn't seem to process some ZDO commands when they're sent this way. It doesn't open joins when you send a ZDOCmd.Mgmt_Permit_Joining_req
directly to 0x0000
but does when you send a c.ZDO.MgmtPermitJoinReq.Req
.
@Adminiuga You think this is something that should be fixed by the radio library via https://github.com/zigpy/zigpy/pull/855? Or should we just recommend using the radio API after sending the ZDO broadcast to all other devices on the network?
await self.load_network_info()
await self.write_network_info(node_info=self.node_info, network_info=self.network_info.replace(channel=25))
Possibly with a helper method that can be re-implemented by radio libraries that take 30s to form a network from scratch (i.e. ZNP)?
await self.update_network_info(network_info={"channel": 25})
This message is fine, as zigpy does not handle those requests. I'd assume ZStack should take care of it? Did it change the channel?
No the controller didn't change its channel. The broadcast is done , but unfortunately the controller stay.
I guess the answer from @puddly is that we must go through the native ZDO feature for each Controller
using the radio API after sending the ZDO broadcast to all other devices on the network?
When sending a broadcast out, do you get the broadcast loopback in? If not, send the broadcast, send another one unicast to coordinator and add a listener handler for nwk update request (zigpy does those now in pr zigpy#855) to translate that request into native stack api request
yep, that what I did an the Unicast to coordinator is producing this error message No handler for ZDO
2022-01-12 15:01:05,764 DEBUG :Received command: ZDO.MsgCbIncoming.Callback(Src=0x0000, IsBroadcast=<Bool.false: 0>, ClusterId=56, SecurityUse=0, TSN=0, MacDst=0x0000, Data=b'\x00\x00\x10\x00\xFE\x01')
2022-01-12 15:01:05,781 DEBUG :[0x0000:zdo] ZDO request ZDOCmd.Mgmt_NWK_Update_req: [NwkUpdate(ScanChannels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, nwkUpdateId=1)]
2022-01-12 15:01:05,783 DEBUG :[0x0000:zdo] No handler for ZDO request:ZDOCmd.Mgmt_NWK_Update_req([NwkUpdate(ScanChannels=<Channels.CHANNEL_20: 1048576>, ScanDuration=254, nwkUpdateId=1)])
So I might need the #855 , but I'm also using the PR from @puddly, so will wait for you guys to merge
When sending a broadcast out, do you get the broadcast loopback in?
It looks like it. Unfortunately, Z-Stack makes things interesting and also sends the same ZDO request/response indications in response to its own internal commands.
This is a problem because creating a ZDOCoordinator:ZDOEndpoint:handle_mgmt_nwk_update_req
method causes a recursive cycle because when it sends a c.ZDO.MgmtNWKUpdateReq.Req
request, that will generate a Mgmt_NWK_Update_req
ZDO event, triggering handle_mgmt_nwk_update_req
again...
Do other coordinators respond to these ZDO commands properly? The Conbee worked with my energy scan test but EZSP did not. No clue about ZiGate or XBee.
I suppose I can unsubscribe from all ZDO requests (i.e. any command below 0x8000
) and internally forward ZDO broadcasts to zigpy but maybe it makes more sense to create dedicated methods for all of these ZDO requests that are independent of zigpy's ZDO
endpoint?
This is a problem because creating a ZDOCoordinator:ZDOEndpoint:handle_mgmt_nwk_update_req method causes a recursive cycle because when it sends a c.ZDO.MgmtNWKUpdateReq.Req request, that will generate a Mgmt_NWK_Update_req ZDO event, triggering handle_mgmt_nwk_update_req again...
I meant for the ZDOCoordinator:ZDOEndpoint:handle_mgmt_nwk_update_req()
not to send a ZDO reply, but to call ZStack API directly and change the channel. Also, why it should send out a c.ZDO.MgmtNWKUpdateReq.Req
, shouldn't that be a c.ZDO.MgmtNWKUpdateReq.Rsp
?
Do other coordinators respond to these ZDO commands properly? The Conbee worked with my energy scan test but EZSP did not for EZSP it depends, some are intercepted by the stack and are not forwarded to the app (although seems to be configurable) and those not handled by the stack directly are forwarded to the application. EZSP does handle the Nwk_Update_Req as I used it to change the
nwk_update_id
but to call ZStack API directly and change the channel. Also, why it should send out a
c.ZDO.MgmtNWKUpdateReq.Req
, shouldn't that be ac.ZDO.MgmtNWKUpdateReq.Rsp
?
That's indeed my implementation. Here's a concrete example for handle_mgmt_permit_joining_req
:
# I send a Z-Stack internal request to permit joins (since raw ZDO doesn't do anything)
===> ZDO.MgmtPermitJoinReq.Req(...)
# Z-Stack confirms it
<=== ZDO.MgmtPermitJoinReq.Rsp(Status=<Status.SUCCESS: 0>)
# But also indicates that it received a ZDO `Mgmt_Permit_Joining_req`
<=== ZDO.MsgCbIncoming.Callback(Src=0x0000, IsBroadcast=<Bool.false: 0>, ClusterId=54, SecurityUse=0, TSN=0, MacDst=0x0000, Data=b'\x00\x01')
# Then sends back its internal response
<=== ZDO.MgmtPermitJoinRsp.Callback(Src=0x0000, Status=<Status.SUCCESS: 0>)
# And also that it sent back a ZDO `Mgmt_Permit_Joining_rsp`
<=== ZDO.MsgCbIncoming.Callback(Src=0x0000, IsBroadcast=<Bool.false: 0>, ClusterId=32822, SecurityUse=0, TSN=0, MacDst=0x0000, Data=b'\x00')
# Zigpy receives both, triggering the coordinator `Mgmt_Permit_Joining_req` handler again
[0x0000:zdo] ZDO request ZDOCmd.Mgmt_Permit_Joining_req: [0, <Bool.true: 1>]
[0x0000:zdo] ZDO request ZDOCmd.Mgmt_Permit_Joining_rsp: [<Status.SUCCESS: 0>]
There's no way to distinguish someone calling coord.zdo.Mgmt_Permit_Joining_req()
from Z-Stack indicating that it received a Mgmt_Permit_Joining_req
request, which for some reason happens when you send a Z-Stack internal command.
The only real purpose of subscribing to ZDO requests is because Z-Stack forwards broadcasts as well. Maybe I can do that internally...
# But also indicates that it received a ZDO `Mgmt_Permit_Joining_req` <=== ZDO.MsgCbIncoming.Callback(Src=0x0000, IsBroadcast=<Bool.false: 0>, ClusterId=54, SecurityUse=0, TSN=0, MacDst=0x0000, Data=b'\x00\x01')
Interesting. And it is not a broadcast. So when ZStack API is called directly it also creates a callback, like if the message was received directly over the air?
I was mistaken about broadcasts, they don't get sent back to the coordinator.
Since there's no real benefit to subscribing to ZDO request events, I can instead subscribe only to responses (to forward them to zigpy) and then trigger ControllerApplication.handle_message
directly and create my own loopback requests. This seems to be working now, including the network update broadcast.
@puddly , I took your last commit and indeed I do confirm that it works.
@puddly can it be that cluster 0x0013 is filtered and I'm not receiving it ?
for instance when pairing a new device, I'm getting those logs, but never get the handle_message() with the 0x0013 Device Announcement message
2022-01-13 19:51:14,072 INFO :TC device join: ZDO.TCDevInd.Callback(SrcNwk=0x18BF, SrcIEEE=7c:b0:3e:aa:0a:08:12:5e, ParentNwk=0x0000)
2022-01-13 19:51:14,076 INFO :Device 0x18bf (7c:b0:3e:aa:0a:08:12:5e) joined the network
2022-01-13 19:51:14,077 DEBUG : [ ZigpyCom_18] AppZnp - get_device ieee:7c:b0:3e:aa:0a:08:12:5e nwk:None
2022-01-13 19:51:14,078 DEBUG :Device 7c:b0:3e:aa:0a:08:12:5e changed id (0x5860 => 0x18bf)
2022-01-13 19:51:14,080 DEBUG : [ ZigpyCom_18] AppZnp - get_device found device: <Device model=None manuf=None nwk=0x5860 ieee=7c:b0:3e:aa:0a:08:12:5e is_initialized=False>
2022-01-13 19:51:14,084 DEBUG :Sending request: ZDO.ExtRouteDisc.Req(Dst=0x18BF, Options=<RouteDiscoveryOptions.UNICAST: 0>, Radius=30)
2022-01-13 19:51:14,086 DEBUG : [ ZigpyCom_18] AppZnp - get_device ieee:7c:b0:3e:aa:0a:08:12:5e nwk:None
2022-01-13 19:51:14,092 DEBUG : [ ZigpyCom_18] AppZnp - get_device found device: <Device model=None manuf=None nwk=0x5860 ieee=7c:b0:3e:aa:0a:08:12:5e is_initialized=False>
2022-01-13 19:51:14,102 DEBUG :Received command: ZDO.ExtRouteDisc.Rsp(Status=<Status.SUCCESS: 0>)
2022-01-13 19:51:14,436 DEBUG :Received command: ZDO.EndDeviceAnnceInd.Callback(Src=0x18BF, NWK=0x18BF, IEEE=7c:b0:3e:aa:0a:08:12:5e, Capabilities=<MACCapabilities.AllocateShortAddrDuringAssocNeeded|RXWhenIdle|MainsPowered|Router: 142>)
2022-01-13 19:51:14,438 DEBUG :Command was not handled: ZDO.EndDeviceAnnceInd.Callback(Src=0x18BF, NWK=0x18BF, IEEE=7c:b0:3e:aa:0a:08:12:5e, Capabilities=<MACCapabilities.AllocateShortAddrDuringAssocNeeded|RXWhenIdle|MainsPowered|Router: 142>)
2022-01-13 19:51:17,424 DEBUG :Received command: AF.IncomingMsg.Callback(GroupId=0x0000, ClusterId=25, SrcAddr=0x88E1, SrcEndpoint=1, DstEndpoint=1, WasBroadcast=<Bool.false: 0>, LQI=117, SecurityUse=<Bool.false: 0>, TimeStamp=10543060, TSN=0, Data=b'\x01\x35\x01\x00\x66\x11\x00\x03\x02\x00\x04\x11', MacSrcAddr=0x88E1, MsgResultRadius=29)
You are right, I am filtering all clusters below 0x8000. I think there needs to be a list of exceptions.
So either a list of exception, or a way to enable them during the startup for instance. So any user of zigpy-znp can indeed select what they want to filter and what they don't want.
As I rely on it , I was not able to discover any new devices ;-)
Thanks for the change, it works now.
Just a question, why filtering some of the ZDO events ? Why not leaving to the upper level to handle them and eventually drop them ?
If you have a use case that requires being notified of ZDO requests (Z-Stack will automatically answer 99% of them so you won't be able to reply), feel free to submit a PR later. For now I'd like to merge this change with the minimal feature set that keeps ZHA functional.
For instance is the Node Descriptor Request not filtered ? [edit] the reason, is just to be able to response with the Manufacturer code that is needed.
the reason, is just to be able to response with the Manufacturer code that is needed.
Z-Stack sends its own response that's unfortunately hard-coded in the firmware. You'd be sending a duplicate response.
These are just indications. Z-Stack automatically responds to all but a select few. In fact, the only one I'm aware of is Mgmt_NWK_Update_req
when you're changing the channel.
Ok. so I agree with you feel free to merge your PR . I think we are fine as well
Z-Stack has a
ZDO.MsgCallbackRegister
command that allows an application to register callbacks for specific ZDO command IDs. While Z-Stack will still send its normal, pre-parsed callbacks to zigpy-initiated ZDO requests, it will now also send a callback containing the raw ZDO response.This greatly reduces the amount of ZNP-specific code and will allow for some features to be moved into zigpy. For example:
This codebase is a WIP but I'm currently able to get new devices to join and all of the initialization-specific ZDO requests are handled properly by zigpy, with no intermediate translation done by zigpy-znp.