CRTP Protocol - Syslink packet dispatching

[Nyothan]

Hi everyone,

I'm currently translating all the communication system of the Crazyflie into Ada.

Looking deeper in the code, It seems to me that the last byte of a received Syslink packet can be lost. Here is the discussed code:

Code: Select all

  void radiolinkSyslinkDispatch(SyslinkPacket *slp)
  {
    static SyslinkPacket txPacket;
    if (slp->type == SYSLINK_RADIO_RAW)
    {
      slp->length--; // Decrease to get CRTP size.
      xQueueSend(crtpPacketDelivery, &slp->length, 0);
      ledseqRun(LINK_LED, seq_linkup);
      // If a radio packet is received, one can be sent
      if (xQueueReceive(txQueue, &txPacket, 0) == pdTRUE)
      {
        ledseqRun(LINK_DOWN_LED, seq_linkup);
        syslinkSendPacket(&txPacket);
      }
    } else if (slp->type == SYSLINK_RADIO_RSSI)
  	{
  		//Extract RSSI sample sent from radio
  		memcpy(&rssi, slp->data, sizeof(uint8_t));
  	}
  }

This line is the one that troubles me:

Code: Select all

      xQueueSend(crtpPacketDelivery, &slp->length, 0);

As the 'crtpPacketDelivery' queue is defined as a queue containing 'CRTPPacket' structs (32-Byte), if 'xQueueSend' read 32 Bytes at the specified address (&slp->length), it seems to me that there is still one byte of 'slp->data' that is not read.

Am I misunderstanding something?

[arnaud]

Hi,

At first sight this code looks indeed really wrong, but actually it works. Though I admit that there is many things that should be better written there ....

I had to look at freertos doc to understand: http://www.freertos.org/a00117.html. The freertos queues works by copy and copy a fixed amount of data. Here the queue is setup for a full crtp structure: https://github.com/bitcraze/crazyflie-f ... link.c#L74 so the xQueueSend will always copy something like 33+ bytes (the size of the crtpPacket structure, the length byte plus header plus data).

We write syslinkPacket in the queue but in a function a bit bellow we read crtpPacket. So we are 'converting' the structure format and it happens that the crtpPacket length is the data-lenght without header (this is the reason of "slp->length--", header is one byte long). Finally the pointer to length is given to xQueueSend because the syslinkPacket structure memory storage is | lenght | data | so giving the address to length and copying so much bytes will copy length and data.

This code works because the memory configuration of crtpPacket and syslinkPacket structures are the same.

I hope I have been clear enough, it took me a little while to re-understand it. Next time we refactor this part I will try to make it more 'robust' (I am a bit scared that just modifying one of the structure would break everything).

[Nyothan]

Well I understant that you want to "convert" the Syslink packet into a CRTPPacket by "filling" the CRTPPacket with the Sysling length (previously decreased, to remove the 'type' component of the Syslink packet).

But the problem is that the size of the CRTPPacket struct is 32 bytes and not 33 (I verified it by printing 'sizeof (CRTPPacket)'), so It will copy 32 bytes from the adress of the 'length' component of the Syslink packet: it will not copy the last byte of the 'data' component of the Syslink packet, because the 'data' has 32 bytes in a Syslink packet, so, since we are copying the 'length' component too, the whole data we want to copy measures 33 bytes.

XQueueSend will "fill" the CRTPPacket like this:

CRTPPacket in the queue, deduced from the Sysling packet just received ('slp') :

|slp.length - 1|slp.data(0 .. 30)|

I know this is old and not good looking code, and I understand that you don't want to change this, but it looks definitively wrong to me since we are loosing 1 byte of data. But maybe we are not loosing information because we want only these 32 bytes: 1 byte for the length and the other 31 bytes for the CRTP Header (1) and the CRTP Data (30). It depends on how the syslink packets are made, and I didn't look to it so I can't say

[arnaud]

This is an 'Historical' bug: in the original Crazyflie firmware the packet MTU was set to 31 instead of 32 and this limited the data part of CRTP packet to 30. When the firmware was re-factored a bit for Crazyflie 2 this was kept to stay compatible with older version. The problem comes from this line: https://github.com/bitcraze/crazyflie-f ... crtp.h#L33

Now, it happens that the normal client is never sending 32bytes packet (the only time it could happen is when setting up log blocks, and the existing log blocks are not that big).

I just changed the packet size back to 32 (data size to 31) and all 32bytes packets gets dropped somewhere (I can see that in the console). So there is a bug somewhere that prevent using full sized packets. I posted a bug about it: https://github.com/bitcraze/crazyflie-f ... /issues/48.

As for the syslink packets they can handle up to 32 bytes data, the NRF51 does not care about the radio packet content and just forward packets back and forth from the radio to the UART, so there should not be any problem there (I am not saying it is bug free, just that it should work .