Add ReturnCodes to UART HIL, change abort policy
#1049
Conversation
ppannuto
added
kernel
P-Significant
There was a problem hiding this comment.
The HIL doesn't seem to say anything about what abort_receive will actually guarantee or not, which I think is fine. I would be concerned if it did match the PR description, because I think being able to implement receive_automatic in software is much more important than being able to virtualize the UART bus.
capsules/src/console.rs
Outdated
| if buffer.len() <= i { | ||
| break; | ||
| self.tx_buffer.take().map_or_else( | ||
| || panic!("Console internal consistency error"), |
There was a problem hiding this comment.
Crash in a capsule seems bad.
There was a problem hiding this comment.
Yes.., but before this was an error that was silently dropped, now it's noted. There are 28 other panic's in capsules as well, most similar consistency issues. I think that the console capsule could be re-architected a bit to make this case impossible, but that's a different PR.
There was a problem hiding this comment.
Doesn't adding return codes give it a way to not be silently dropped but without crashing the entire system?
kernel/src/hil/uart.rs
Outdated
| /// No error occurred and the command completed successfully | ||
| CommandComplete, | ||
| } | ||
|
|
||
| pub trait UART { | ||
| /// Set the client for this UART peripheral. The client will be | ||
| /// called when events finish. | ||
| fn set_client(&self, client: &'static Client); | ||
| fn set_client(&self, client: &'static Client) -> ReturnCode; |
There was a problem hiding this comment.
Why would setting a client not succeed?
ppannuto
added
the
blocked
This implements RFC #1035 for the UART HIL. It splits the hardware-specific initialization (power, clock assignment, pin assignment) from the uart-specific configuration (baud rate, parity, stop bits, flow control), which is now controlled by a new `configure` method for the UART HIL.
|
This has been rebased on top of #1073 as they conflicted. This PR also needs to more deeply consider and enumerate the possible types of failures for transmit and receive before it is ready to merge. |
Also corrects the console driver to propogate errors when they occur.
| /// active receive may complete before this call occurs, in which case the | ||
| /// `receive_callback` will be called with `CommandComplete` and this method | ||
| /// will return `EALREADY`. | ||
| fn abort_receive(&self) -> ReturnCode; |
There was a problem hiding this comment.
The PR description doesn't seem to match this comment. This doesn't say whether data will be returned or not.
In either case, returning what has been received so far was the entire purpose of adding this function. Without it, there is no way to implement a UART receiver that can take in variable length messages without doing it byte-by-byte.
|
I'll work on this PR over the weekend, to redesign the UART HIL. Let's get it over with. |
|
Agreed. I think the high-level changes should be preserved, but we can cross that bridge with the new UART HIL. |
Pull Request Overview
This updates the UART HIL to allow methods to fail. Previously, they would silently drop errors internally. The console should also bubble these errors back to userland correctly now. There are probably some more cases where underlying implementations can catch and report more errors, but this at least allows them to do that in the future.
This also updates
abort_receiveto no longer guarantee the return of partial buffers. As noted in #1045, partial buffers causes problems for virtualization, as one client's abort request requires all other clients to abort in order to recover the partial buffer. This changes our ABI semantics slightly. I do not believe anything is relying on this behavior currently, and I believe that not guaranteeing partial buffers on abort is generally a better long-term interface.Testing Strategy
Hail + various other printing apps.
Documentation Updated
/docs, or no updates are required.Formatting
make formatall.