possible bug in bellows/send_zdo_broadcast()

[Adminiuga]

I think await self._ezsp.sendBroadcast(0xfffd, aps_frame, radius, len(data), data) is being called with incorrect parameters.
I believe a correct way of calling it would be:
await self._ezsp.sendBroadcast(0xfffd, aps_frame, radius, aps_frame.sequence, data
and here's my reasoning. Per EZSP reference guide, sendBroadcast needs the following arguments:

Type	Parameter
EmberNodeID	Broadcast Address
EmberApsFrame	aps frame
uint8_t	radius
uint8_t	messageTag -- Sequence number
uint8_t	message length -- len(data)
uint8_t[]	message content

which is defined in bellows/commands.py as

'sendBroadcast': (0x36, (t.EmberNodeId, t.EmberApsFrame, t.uint8_t, t.uint8_t, t.LVBytes), (t.EmberStatus, t.uint8_t)),

the actual last parameter is of t.LVBytes type, which is represents [len(data), data], therefore you don't need to provide len(data), but you do need to provide the messageTag/sequence number, so the right way of calling it should be
await self._ezsp.sendBroadcast(0xfffd, aps_frame, radius, aps_frame.sequence, data

Thoughts?

[Adminiuga]

optionally we could push some asyncio.Future() into self._pending, so EZSP does not complain on an "unexpected send". something like:

...
        send_fut = asyncio.Future()
        self._pending[sequence] = (send_fut, None)

        v = await self._ezsp.sendBroadcast(0xfffd, aps_frame, radius,
                                           sequence, data)
        if v[0] != t.EmberStatus.SUCCESS:
            self._pending.pop(sequence)
            send_fut.cancel()
            raise DeliveryError("Message send failure {}".format((v[0], )))

        # Wait for messageSentHandler message
        v = await send_fut
        return v

similar to request() method.

[Yoda-x]

the sequence number is included in the data
data = aps_frame.sequence.to_bytes(1, 'little')
schema = zigpy.zdo.types.CLUSTERS[command][2]
data += t.serialize(args, schema)

as it is part of the payload
the ug100 document is a little bit unclear, here is a better description for the permit join request.

why do you think it's buggy? do you have a problem with joining?

[Adminiuga]

Can you check the link? I found a few KB articles from 2012/07/02 but can't figure out which one are you referencing.
So upon second look on my original message I do see some error, but what I actually meant that the order of the parameters should be different, ie _ezsp.sendBroadcast expects the following order of the parameters

Type	Parameter	implemented
EmberNodeID	Broadcast Address	correct
EmberApsFrame	aps frame	correct
uint8_t	radius	correct
uint8_t	messageTag -- Sequence number	incorrect, swapped with length
uint8_t	message length -- len(data)	incorrect -- swapped with seq # and incorrectly accounts for the length of seq #
uint8_t[]	message content	correct

however you are giving it: _ezsp.sendBroadcast(broadcastaddress, aps_frame, radius, len(data), data) where data is [sequence_number, permit_join_time, tc_significance]
in other words in your variant the sequence number is after the data length, but it should be before.
something like

        seq = aps_frame.sequence.to_bytes(1, 'little')
        schema = zigpy.zdo.types.CLUSTERS[command][2]
        data = t.serialize(args, schema)
        LOGGER.debug("zdo-broadcast: %s - %s", aps_frame, data)
        await self._ezsp.sendBroadcast(0xfffd, aps_frame, radius, seq, len(data), data)

and I still trying to figure out whether len(data) should be there at all. I'm just comparing _ezsp.sendBroadacast() to _ezsp.sendUnicast() which has very similar arrangement of arguments

yet bellows.zigbee.application.request() which leverage _ezsp.sendUnicast() does not use len(data) unless I'm missing it somewhere 😕

[Adminiuga]

so my current thinking, you don't need to provide len(data) to _ezsp.sendBroadcast(), because schema for sendBroadcast command is defined as:
'sendBroadcast': (0x36, (t.EmberNodeId, t.EmberApsFrame, t.uint8_t, t.uint8_t, t.LVBytes), (t.EmberStatus, t.uint8_t)),

so when serialize method is going to be called for t.LVBytes it is going to prefix the length automatically, so I currently believe that we need to call it like:

        seq = aps_frame.sequence.to_bytes(1, 'little')
        schema = zigpy.zdo.types.CLUSTERS[command][2]
        data = t.serialize(args, schema)
        LOGGER.debug("zdo-broadcast: %s - %s", aps_frame, data)
        await self._ezsp.sendBroadcast(0xfffd, aps_frame, radius, seq, data)

[Yoda-x]

hi, the current version works for me over 4 floors. Please test your recommendation and see how it works.
if your version is ok, please create a pull request
Thanks

[Adminiuga]

can't say your variant doesn't work. Both your and mine variants worked. fine. I've been running my version for a while now without any issues.
I'll create a pull request for a review.