search

bipropellant / bipropellant-protocol

defines and implements a low level protocol shared between a few different hoverboard repositories
MIT License
20 stars 17 forks source link

byte stuffing and relayability #33

Closed p-h-a-i-l closed 4 years ago

p-h-a-i-l commented 4 years ago

It is time to do the semi-annual complete protocol overhaul :)

I had two issues with the state the protocol was implemented which got me thinking:

  1. Relaying the protocol over different channels (hoverboard to relay via UART, relay to control via UDP (over Wifi))
  2. Performance problems: The UART Communication is very susceptible to disturbances, mostly EMI. Still, it felt like it should be better.

Regarding 1.: I already prepared to move almost all variables which are protocol related into the state struct. This is now done with all consequences. Multiple instances of the protocol can now be used on the same device. In order to relay messages between instances, the automatic reactions to messages needed a way to be disabled. Now reading of variables and replies to Read and Write requests are handled inside the individual code specific functions. By default: fn_defaultProcessing. Now the behaviour can be very specific, even replies with data to Writes are possible.

Regarding 2.: Up unitl now we were using two different bytes indicating the start of a new message. 0x02 and 0x04. The following length byte made sure, that the following bytes are interpreted as data, even if they were 0x02 or 0x04. This does result in some problems: If one byte of the message is lost, the SOM of the next message is interpreted as data and ignored. Both messages are lost. Even worse, when the payload (or anything) of the next message contains 0x02 or 0x04. It might take quite long until the communications is back in sync. The solution is to mask the preamble. One way is to escape all preamble bytes, but then the escaping byte needs to be escaped too. In the worst case, the payload consists of only SOM bytes and the message is blown up to the double length. Since we were using multiple preambles, even more data needs to be escaped. I decided to use COBS/R (https://github.com/cmcqueen/cobs-c#consistent-overhead-byte-stuffing--reduced-cobsr) Thanks to @thomask77 for the suggestion! Multiple changes needed to be done to the message structure, but the impact on the main repo was smaller than expected. There is only one SOM left (0x00), the difference between ACK and NOACK is encoded in the highest bit of cmd. Since cmd can never be 0x00, it is not part of the COBS/R encoded message. (as are len and CI)

First tests look very promising, haven't had time to test ASCII protocol yet. Repos compatible to the latest protocol can be found here

I'd be glad to get some comments and feedback, especially by @btsimonh and @EmerickH . This is a huge breaking change, but I think it's worth it.

Open Issues:

btsimonh commented 4 years ago

@p-h-a-i-l - i won't have time to give this the justice it deserves for the moment. hopefully one day my work will calm, and I can get back to the HB :(. However, one or two comments: 1/ from a brief read, it sounds good. 2/ you say breaking change.... Can it co-exist with the existing protocol or not? I suspect we could switch between them? (it does not bother me, as I would use the new! - but we do have quite a few people who are using the old, no?).

p-h-a-i-l commented 4 years ago

No worries, but I still appreciate you input :) I'd rather not maintain both versions of the protocol. That will create an ifdef hell and it doubles the effort in future changes. We could make a release with the old version or keep a tag. I don't know how many people are actively using the protocol. For those using the API it will be easy to update.

p-h-a-i-l commented 4 years ago

Did some testing on my EMI-problem-board yesterday. There will be no dual old and new protocol :) The performance is way better. Like between "can not drive at all" and "what EMI issues?". So the breaking change is worth it, better to force users to change than have them hunt for problems.

btsimonh commented 4 years ago

Agreed. Sound like a huge improvement :).