Replace TI.CC2531 Linq-based buffer processing by helper functions

[oblaise]

This PR insert new ByteHelper functions and replace the processing of the buffer using System.Linq by the ByteHelper.Slice function that work directly on ByteArray to reduce the overhead required to work with Linq and Enumerable<byte> on buffers.

[oblaise]

I was thinking I introduced a bug but after more detailed review it's a 1 to 1 mapping of the existing using new functions.

[Mr-Markus]

Is it an mistake that you reopened it? As I understood everything works fine

[oblaise]

Is it an mistake that you reopened it? As I understood everything works fine

The mistake was to close it -> I used it for a few days without issue.

But after creating the PR I did a quick comparison with the original java source which use a slightly different API and fields. So I closed it to check further the difference with java then I re-opened it after additional review.

[Mr-Markus]

Do you need further review on the implementation against the java project or is it finished now?

[oblaise]

Do you need further review on the implementation against the java project or is it finished now?

No it's OK from that point of view. It's identical to what is done in java.

But did you already looked into detail to the content of the payload field ? If you look for example at the

    public class ZdoSimpleDescriptor : TiDongleReceivePacket
    {
        public static ZigBeeApsFrame Create(ZToolPacket packet)
        {
            ZigBeeApsFrame apsFrame = new ZigBeeApsFrame();
            apsFrame.Cluster = ZdoCommandType.GetValueByType(ZdoCommandType.CommandType.SIMPLE_DESCRIPTOR_RESPONSE).ClusterId;
            apsFrame.DestinationEndpoint = 0;
            apsFrame.SourceAddress = (ushort)(packet.Packet[4] | (packet.Packet[5] << 8));
            apsFrame.SourceEndpoint = 0;
            apsFrame.Profile = 0;

            apsFrame.Payload = packet.Packet.Slice(5,packet.Packet[1] - 1);

            return apsFrame;
        }
    }

for me the payload should be packet.Packet.Slice(4,packet.Packet[1]) if we want to include the source address in the payload or packet.Packet.Slice(6,packet.Packet[1]-2) if we don't want to include the source address... but here the payload is taking half of the SourceAddress. That's exactly the same in the java code.

Additionally I also looked at the usage of the Payload field for the ZigBeeApsFrame that are constructed by theses classes and it seems it is only used for logging.

[Mr-Markus]

for me the payload should be packet.Packet.Slice(4,packet.Packet[1]) if we want to include the source address in the payload or packet.Packet.Slice(6,packet.Packet[1]-2) if we don't want to include the source address... but here the payload is taking half of the SourceAddress. That's exactly the same in the java code.

Additionally I also looked at the usage of the Payload field for the ZigBeeApsFrame that are constructed by theses classes and it seems it is only used for logging.

@oblaise It is a while ago that I implemented it so that I had to rethink about it. Take a look here: https://github.com/Mr-Markus/unpi-net This is my implementation of Texas Instruments UNPI and you can see packet format in the README.md

Here we build an ZclPacket:

https://github.com/Mr-Markus/ZigbeeNet/blob/d2ceef8651d0bb219f1d506a86e2a671dca1f998/libraries/ZigBeeNet.Hardware.TI.CC2531/Packet/ZToolPacket.cs#L122-L155

And the offical Z-Stack docs for ZDO_END_DEVICE_ANNCE describes following packet:

1 Byte	1 Byte	1 Byte	1 Byte	2 Bytes	8 Bytes	1 Byte
SOF=0xFE	Length=0x0B	Cmd0=0x25	Cmd1=0x0A	NwkAddr	IEEEAddr	Capabilites

So I would agree to use packet.Packet.Slice(4, packet.LEN) if 4th Byte is included. It should be the same for other packets, too

@nicolaiw What do you think?

[nicolaiw]

sounds reasonable :)

regards

[oblaise]

@Mr-Markus After a more detailed review of the code and even if this seems strange from a pure semantic perspective, the use of the LSB of the address in the raw packet as the first byte of the Payload is consistent with the parsing of the Payload in the following part of the code: https://github.com/Mr-Markus/ZigbeeNet/blob/dd5605e2f45ce6917bfc90fac80b38828d4c7546/libraries/ZigBeeNet/ZDO/Command/SimpleDescriptorResponse.cs#L62-L75

While the parent function use the first byte as TransactionId: https://github.com/Mr-Markus/ZigbeeNet/blob/dd5605e2f45ce6917bfc90fac80b38828d4c7546/libraries/ZigBeeNet/ZDO/ZdoCommand.cs#L9-L23

As a consequence, if we change the Payload construction we will break the code above.

For this reason, can you please integrate my PR as it exist today ? The PR as you reviewed it a few weeks ago doesn't change anything to the content of the Payload in the functions. It's only a change to remove linq-based related byte array parsin in TT.CC2531 library.

[oblaise]

Hello @Mr-Markus what's your opinion on this : Don't you think that we can first add a few non-breaking changes to the existing code base to push some performance improvements ? I would like to push another performance improvement change in the standard library but I would like first to be able to have this change integrated.

[Mr-Markus]

Hi @oblaise, if there are no breaking changes (like this as it is) I agree to merge this PR. I belive that you are able to run it wit CC2531 with your devices as before? I appriceate another performance improvments very much 👍

[oblaise]

Yes indeed, I'm using CC2531 and everything working as before.