search

Koenkk / zigbee-herdsman

A Node.js Zigbee library
MIT License
454 stars 278 forks source link

Improve broadcasting via `sendZclFrameToAll` #1028

Closed burmistrzak closed 1 week ago

burmistrzak commented 2 weeks ago

Just adds an optional parameter to the sendZclFrameToAll method so sleepy end devices (i.e. 0xFFFF) can be included in broadcast messages. 😊

Seems to be required for this to work properly.

Nerivec commented 2 weeks ago

I'd say no boolean, just a variable for the address:

/** Broadcast to all routers. */
export const BROADCAST_ADDRESS = 0xFFFC;
/** Broadcast to all non-sleepy devices. */
export const BROADCAST_RX_ON_WHEN_IDLE_ADDRESS = 0xFFFD;
/** Broadcast to all devices, including sleepy end devices. */
export const BROADCAST_SLEEPY_ADDRESS = 0xFFFF;

These should be with Zigbee spec types, usable everywhere.

And the adapter function becomes

public abstract sendZclFrameToAll(endpoint: number, zclFrame: ZclFrame, sourceEndpoint: number, address: number = BROADCAST_ADDRESS): Promise<void>;

Note:

ZigBee specifies three different broadcast addresses that reach different collections of nodes. Broadcasts are normally sent only to routers. Broadcasts can also be forwarded to end devices, either all of them or only those that do not sleep. Broadcasting to end devices is both significantly more resource-intensive and significantly less reliable than broadcasting to routers.

burmistrzak commented 2 weeks ago

I'd say no boolean, just a variable for the address:

Funnily enough, that was my initial idea. 😊

These should be with Zigbee spec types, usable everywhere.

Sure, is adapter.ts the right place for such definitions?

And the adapter function becomes

public abstract sendZclFrameToAll(endpoint: number, zclFrame: ZclFrame, sourceEndpoint: number, address: number = BROADCAST_ADDRESS): Promise<void>;

We'll have to keep the current behavior, so BROADCAST_RX_ON_WHEN_IDLE_ADDRESS should be default.

ZigBee specifies three different broadcast addresses that reach different collections of nodes. Broadcasts are normally sent only to routers. Broadcasts can also be forwarded to end devices, either all of them or only those that do not sleep. Broadcasting to end devices is both significantly more resource-intensive and significantly less reliable than broadcasting to routers.

For the mentioned use.case, reaching every sleeping ZED seems kinda important. 😉

Nerivec commented 2 weeks ago

Only 6 use cases of sendZclFrameToAll currently, all are for GP, so not complicated to refactor. Better to use the proper default broadcast (FFFC) and change calls as needed to FFFD or FFFF by specifying it.

burmistrzak commented 2 weeks ago

Only 6 use cases of sendZclFrameToAll currently, all are for GP, so not complicated to refactor. Better to use the proper default broadcast (FFFC) and change calls as needed to FFFD or FFFF by specifying it.

Sounds reasonable. Will do that next, when I'm done here.

burmistrzak commented 2 weeks ago

These should be with Zigbee spec types, usable everywhere.

@Nerivec Not entirely sure what you mean by that. Can you please elaborate?

Edit: Do you mean something similar to status.ts, e.g. Zcl.Broadcast.NON_SLEEPY_DEVICES? 🤔

Nerivec commented 2 weeks ago

@Koenkk Not sure what convention we want to use for naming, but since we're gonna introduce the zdo spec too, I figure we can put global stuff at the root and zcl/zdo as subfolders. Feel free to rename! (I didn't move the content of the zcl folder here, since that's a bigger refactor. I also didn't add the million or so consts that can be introduced to remove magic numbers/strings throughout the code 😁)

I added a test that mirrors the requirements of the Bosch in https://github.com/Koenkk/zigbee-herdsman-converters/pull/7427. I think it makes more sense that way? ZHC can just do

 await meta.device.triggerBroadcast(255, 1, BroadcastAddress.SLEEPY, Zcl.Direction.CLIENT_TO_SERVER, 'boschSmokeDetectorSiren', Zcl.Clusters.ssIasZone.ID, {data: index});
burmistrzak commented 2 weeks ago

Not sure what convention we want to use for naming, but since we're gonna introduce the zdo spec too, I figure we can put global stuff at the root and zcl/zdo as subfolders.

Ah, I see! So I wasn't that far off with my guess. 😅

ZHC can just do

 await meta.device.triggerBroadcast(255, 1, BroadcastAddress.SLEEPY, Zcl.Direction.CLIENT_TO_SERVER, 'boschSmokeDetectorSiren', Zcl.Clusters.ssIasZone.ID, {data: index});

Very elegant solution! Makes it generally more useful for converter developers. Excellent job! 🤝

burmistrzak commented 2 weeks ago

Can we export BroadcastAddress (or better zspec) at the root, so using it from ZHC is more convenient?

Nerivec commented 2 weeks ago

Waiting for Koenkk's input on naming & co, so we can export this properly. The spec folder structure will affect a lot of stuff, better to start with a solid base.

Nerivec commented 1 week ago

I figure, might as well keep entirely in line with the rest, so zclCommandToAll seems the best option?

@Koenkk Can you confirm if this.customClusters should be passed in this scenario (to getCluster and to ZclFrame.create)? Same question for this.manufacturerID to getCluster (that's what endpoint is using)? Seems a bit conflicting between using device (this) and using options.

burmistrzak commented 1 week ago

I figure, might as well keep entirely in line with the rest, so zclCommandToAll seems the best option?

Agree. 👍 And thanks for cleaning up after me, looking good so far. 🤝

burmistrzak commented 1 week ago

Waiting for Koenkk's input on naming & co, so we can export this properly. The spec folder structure will affect a lot of stuff, better to start with a solid base.

@Koenkk The proposed folder structure seems sensible, but we'll eventually have to move ZCL to the Zspec directory.

Ultimately, it's your decision. 😉

Nerivec commented 1 week ago

The move will be in a separate PR. It was just the right time to create the base for it since we needed a couple of "global zigbee" consts for this.

burmistrzak commented 1 week ago

The move will be in a separate PR. It was just the right time to create the base for it since we needed a couple of "global zigbee" consts for this.

Sounds good. Should we already export BroadcastAddress (e.g. as Zspec.BroadcastAddress.SLEEPY), before the "big move" PR comes around? Or do we want to keep these things in separate PRs?

Koenkk commented 1 week ago

@burmistrzak let's do this in a separate PR such that we can also merge https://github.com/Koenkk/zigbee-herdsman-converters/pull/7427 before the new release.