Handle embedded_io_async::Read/Write Guarantee #235
Conversation
* Return Vec<SplitMessages> as read() from buffer may contain more than 1 messages (currently up to 2 based on MAX_SIZE) * Store partial messages in struct value until next read() * Change to postcard::to_slice_cobs() to allow individual messages to be identified * Loop read() until at least one complete message received * Loop write() until all bytes confirmed to be sent * Increase SPLIT_MESSAGE_MAX_SIZE to allow for COBS overhead * Handles read() returning < 1 message, > 1 && < 2 messages and up to < 3 messages correctly
* If message is corrupt, unlikely to respect COBS encoding so not possible to skip bytes in loop * Avoid possible situation where end_byte is greater than buffer.len()
So what's the problem? I think postcard could solve it automatically, deserialize the message from a partial filled buffer. |
|
The main problem is that the serial Read trait isn't guaranteed to read all the bytes in a message. This is very likely if the UART bus is slower than the monitor cycle, but still quite likely if the UART buffer isn't a multiple of the message size (the internal ring buffer in BufferedUart will only return a contiguous slice, so when it gets to the end of the buffer, it won't return all the data in the buffer, even if its available). The code could call postcard::take_from_bytes() after each serial read() and loop until it doesn't error, then save any used slice for the next read if necessary. This would skip (or replace) the second while loop in SerialSplitDriver::read(), but otherwise handling a remaining partial message and the possibility of two messages is still required. I thought explicitly identifying complete messages using the COBS separator was more robust than just trying to deserialise any returned bytes. |
I think it is guaranteed. The buffer of |
|
I'm not sure, maybe I misunderstood, but:
For an example, I wrote my own half-duplex UART driver as the Sofle keyboard isn't wired for full-duplex UART, and this doesn't (and can't see 3. above) return all the bytes of a message on read(): https://github.com/grabpot/rmk-sofle-dvorak |
| @@ -19,7 +20,7 @@ pub(crate) enum SplitDriverError { | |||
|
|
|||
| /// Split message reader from other split devices | |||
| pub(crate) trait SplitReader { | |||
| async fn read(&mut self) -> Result<SplitMessage, SplitDriverError>; | |||
| async fn read(&mut self) -> Result<Vec<SplitMessage, 2>, SplitDriverError>; | |||
There was a problem hiding this comment.
For the trait, each read call should read only one SplitMessage, not two. It defines "expected behavior", should not be affected by the serial implementation
|
|
||
| let mut start_byte = 0; | ||
| let mut end_byte = start_byte; | ||
| while end_byte < self.n_bytes_part { |
There was a problem hiding this comment.
The buffer size is SPLIT_MESSAGE_MAX_SIZE, so this second loop is not needed -- we can assume that after the first while loop, there's a complete SplitMessage received.
| return Err(SplitDriverError::EmptyMessage); | ||
| } | ||
|
|
||
| self.n_bytes_part += n_bytes; |
There was a problem hiding this comment.
Check if n_bytes_part > self.buffer.len()
embedded_io_async::Read/Write do not guarantee "that all available buffer space is filled", and could read/write less than buf.len(), and to handle this:
NOTE: I've only tested this with an rp2040 and my custom half-duplex UART driver. I don't expect any issues, but it should be tested with the standard BufferedUart or equivalent serial driver.