Packet format - stop byte recognized as start byte during resynchronization about bldc HOT 9 CLOSED

The only robust approach to mitigate the described problem is to implement a transparent channel model, explicitly separating data characters from control characters (e.g., by means of escape sequences). All other approaches, including the one proposed, are prone to the described failure mode, although with the proposed fix it would be marginally less likely to occur. Examples of transparent channel protocols:

• https://en.wikipedia.org/wiki/High-Level_Data_Link_Control (also see PPP encap)
• https://github.com/Zubax/popcop (this one is specifically designed for embedded systems and is robust)

from bldc.

https://en.wikipedia.org/wiki/Consistent_Overhead_Byte_Stuffing is a good alternative to HDLC due to its consitent overhead that is always known.

from bldc.

@andresv , @pavel-kirienko - thanks for your links, I learned a few new things.

However it seems that those protocols are optimized for a different case - namely low BER, high utilization. For example the COBS article says "COBS does, however, require up to 254 bytes of lookahead. Before transmitting its first byte, it needs to know the position of the first zero byte (if any) in the following 254 bytes", which makes it not practical for typical VESC traffic in my opinion.

Popcop is also a nice protocol, however I don't see how it shortens the recovery in case the delimiter is corrupted - both packets the preceding and following packets are lost. Well, yes, the "worst case scenario" is better than the current situation, and seems comparable to my proposal.

In my proposal the probability of loosing more than the affected packet due to header corruption would be much lower - at least two errors that corrupt the header AND the CRC in compatible ways. For the PopCop it would be BER*8/packet size.

And my suggestion is a smaller change, that can be done in backward-compatible way which is important for keeping the ability to upgrade older FW versions with newer VESC tool.

from bldc.

Lookahead is not a problem. Typical usecase should be following:

#define _COBS_CEILING_(x,y) (((x) + (y) - 1) / (y))
#define COBS_MAX_OVERHEAD(MAX_PAYLOAD_LEN) (_COBS_CEILING_(MAX_PAYLOAD_LEN, 254) + 1) // +1 is for ending 0x00 byte

#define VESC_MAX_PAYLOAD_LEN 532 // random size, i do not know what it is actually
uint8_t txbuf[VESC_MAX_PAYLOAD_LEN + COBS_MAX_OVERHEAD(VESC_MAX_PAYLOAD_LEN)];


size_t cobs_frame_size = cobs_encode((uint8_t*)some_vesc_msg, txbuf, vesc_msg_len);
send_to_serial(txbuf, cobs_frame_size);

from bldc.

Popcop is also a nice protocol, however I don't see how it shortens the recovery in case the delimiter is corrupted - both packets the preceding and following packets are lost. Well, yes, the "worst case scenario" is better than the current situation, and seems comparable to my proposal.

Any transparent channel protocol offers a deterministic recovery time: the first uncorrupted packet will be unconditionally accepted. Its deterministic recovery properties are also invariant to the payload exchanged over the channel.

Neither of those are true for any protocol that relies on magic sequences, such as the current protocol utilized by VESC or your proposed improvement. While your solution does indeed make the average case better, it fails to improve the worst case, which, considering that we are applying it to a robust real-time system (motor control is a serious business), is an important shortcoming.

Consider what happens to your protocol if:

• the magic header is corrupted;
• the payload of the packet whose header is corrupted contains a sequence of data which looks like a valid header with a very long payload length.

Alternatively:

• the connection is established (e.g. a serial link is attached) while a packet transfer was in progress; the receiving side missed the header;
• the remaining part of the packet contains a sequence of data which looks like a valid header with a very long payload length.

As Andres has implied, the look-ahead requirement of COBS is unlikely to cause trouble; additionally, as one can see upon a closer look at the description, the look-ahead buffer can be arbitrarily reduced at the cost of higher overhead; the case where the lookahead buffer is one byte long almost approaches the traditional escape-sequence-based transparent channel implementation (such as HDLC).

Popcop is also a nice protocol, however I don't see how it shortens the recovery in case the delimiter is corrupted - both packets the preceding and following packets are lost.

A delimeter byte demarcates both the beginning and the end of each packet, so there are two delimeters per packet; however, this question is not exactly relevant, see above.

from bldc.

The proposed protocol would help a bit, but I would prefer to keep backwards compatibility with the old code so that older versions of VESC Tool can figure out if the VESC has newer firmware. This compatibility comes at the cost that a bit error can make a packet interpreted in the old way, which is much more likely than the header in the new format being corrupted such that it passes the header CRC.

A complete redesign of the protocol would probably be best so that there is separation between data and control characters, as @pavel-kirienko suggested. Even something simple such as coding the data in base64 and having non-base64 control characters would eliminate this problem almost completely at the cost of some overhead.

Then again, the way the protocol is now the worst case is not too bad. As the packet is thrown if a length of more than 512 bytes is decoded, the worst case is that the length is set to 512 bytes. If the shortest packet of e.g. set current is used, which will be 10 bytes, it takes 52 of those packets to get out of that state. If the current is updated at 100 Hz, this is a delay of 0.5 s. Not great, but not too bad either.

I think the best approach would be to make an alternate UART mode that can be selected, which uses packets with separate data and control characters. With USB this is not really a problem in practice, and it should not be a problem for the CAN-implementation either.

One question: did you actually experience problems in practice because of this, or is this a just-in-case improvement?

from bldc.

well, for our use case it did cause much larger delays in practice, since the packet frequency is much lower than 100Hz due to other system limitations. We've seen 40-60 second delays.

Both switchable protocol implementation and byte stuffing approach seem good options. I think base64 could be a bit harder to debug by eyeballing the hex dump though.

from bldc.

Hi,

I don't get all the details, but is it that complicated to keep old protocol only for FW version and FW update commands ?
If the new protocol uses a different start byte, the selection between old and new ones will be easy, won't it ?

from bldc.

Protecting the header length with a second CRC is a pretty reasonable approach (rosserial made a similar change a few years ago). That said, there are lots of robust serial encapsulation schemes out there already— if you're breaking compatibility anyway, it might make sense to just switch to something established, particularly since the CAN protocol will be unaffected.

from bldc.