mgmt-gateway: Refine serial console messages#737
Conversation
1. We can now accept partial messages 2. We no longer erroneously duplicate output on the uart if we receive a resent packet
Add an offset to detect lost data; continue reading from usart even if that means discarding buffered data.
|
|
||
| // Ensure we always have space for at least 16 bytes (the stm32h7 usart | ||
| // FIFO depth), even if that means discarding old data. | ||
| if available_rx_space < 16 { |
There was a problem hiding this comment.
Can we log an error here, even though MGS will also notice it?
There was a problem hiding this comment.
By "log" do you mean a ringbuf entry or something else?
| } | ||
|
|
||
| fn drain_flushed_data(&mut self, n: usize) { | ||
| self.from_rx.drain(..n); |
There was a problem hiding this comment.
Under the hood, this is going to ptr::copy the remaining data to shift it over by n (source), so it could do a bunch of extra work!
It seems like the "correct" data structure here is a circular buffer, although that makes serialize_with_trailing_data a little trickier, because the trailing data could be split over two slices.
Do you think it's worth fixing this now, or leaving a TODO?
There was a problem hiding this comment.
heapless::Deque looks promising (and already has an as_slices method!)
There was a problem hiding this comment.
Switched to heapless::Deque in 215a1a0, replacing all the drains with loops over pop_front()s and rewriting the bit around available_rx_space. It's a big enough change to probably be worth a second look, if you don't mind.
| // Ensure we always have space for at least 16 bytes (the stm32h7 usart | ||
| // FIFO depth), even if that means discarding old data. | ||
| if available_rx_space < 16 { | ||
| self.from_rx.drain(..(16 - available_rx_space)); |
There was a problem hiding this comment.
(same concern about drain being expensive here)
| // Have we already seen _all_ of this data? If so, just reply that | ||
| // we're ready for the data that comes after it. | ||
| if skip >= data.len() as u64 { | ||
| return Ok(offset + data.len() as u64); |
There was a problem hiding this comment.
Should we return self.serial_console_write_offset here instead?
e.g. if
self.serial_console_write_offset = 128
offset = 64
data.len() = 32
we want the host to start sending us data at offset 128, not 96.
(or will the host get confused that we've managed to write more data than it thinks it's sent us?)
There was a problem hiding this comment.
(or will the host get confused that we've managed to write more data than it thinks it's sent us?)
Exactly this, at least as currently implemented. In practice we should never see skip > data.len() here, unless something has gone horribly wrong, because the host should never go backwards in time, and only moves forward when we ack its messages. We may see skip == data.len() if our ack gets lost and the host resends the same packet it most recently sent, though.
This avoids potentially-expensive drains and lets us rewrite the logic to pull data out of the USART FIFO more directly.
* Refine receiving serial console messages from MGS 1. We can now accept partial messages 2. We no longer erroneously duplicate output on the uart if we receive a resent packet * Refine sending serial console messages to MGS Add an offset to detect lost data; continue reading from usart even if that means discarding buffered data. * Switch from ArrayVec to Deque for USART buffers This avoids potentially-expensive drains and lets us rewrite the logic to pull data out of the USART FIFO more directly.
* Refine receiving serial console messages from MGS 1. We can now accept partial messages 2. We no longer erroneously duplicate output on the uart if we receive a resent packet * Refine sending serial console messages to MGS Add an offset to detect lost data; continue reading from usart even if that means discarding buffered data. * Switch from ArrayVec to Deque for USART buffers This avoids potentially-expensive drains and lets us rewrite the logic to pull data out of the USART FIFO more directly.
2 participants
This is the hubris half to oxidecomputer/omicron#1651 that lets us address a few lingering TODOs: