Tracking & RFC: UART HIL Refinements

[ppannuto]

n.b. The goal of this issue to to serve as a central location for discussion surrounding the overall direction UART HIL updates. As there are a few orthogonal HIL issues, the actual implementation of all the issues addressed here will be broken into discrete PRs, one for each major change.

---

Tracking

• Remove initialize from UART HIL.
  • RFC in RFC: Initialize in HIL interfaces #1035
  • PR in uart: remove initialize from the UART HIL #1073
• Add generic lifetime to UART HIL
  • RFC in Tracking: Generic over static lifetimes in HIL interfaces #1074
  • PR in pending
• Add ReturnCodes to appropriate HIL methods
  • RFC in Tracking: HIL methods that can fail should have ErrorCodes #1052
  • PR in Add ReturnCodes to UART HIL, change abort policy #1049

[bradjc]

Idea: go all the way and add a UartDevice trait (like I2C and SPI) that has the UartParameters baked in.

Pros:

• More consistent.
• Can be used to separate code that needs a capability to access hardware and code that doesn't.
• Makes it easier to determine when to call configure() (only when the actual user of the bus changes does any code need to even consider calling configure() again.

Cons:

• Why put in a bunch of code and complication for an interface that is very unlikely to not need all this complexity, given how UART busses are commonly used (between exactly two devices with fixed settings)?
• How would a receiver handle getting an unpredictable ordering of UART messages at different baud rates, parity, stop bits, and flow control settings (as could happen with something like virtual_uart.rs)?

What might be more useful, and Tock-esque, is to have a UartDevice trait that:

• removes the .configure() function
• removes the .abort_receive() function

Then any normal user of the UART bus has access, it could be easily virtualized, and it doesn't have to worry about the bus settings and instead uses whatever was configured by something else (likely a board's main.rs). It also matches the use case we have so far: console and debug.rs sharing a UART channel.

Anything more advanced simply has to use the Uart trait, and then knows it has exclusive access to the uart bus and can change the settings as it pleases.

[phil-levis]

The UART trait should be split into three different traits: - Control - Receive - Transmit You have to separate Receive and Transmit because correct virtualization has very different behavior: you don’t want to allocate virtualized receive buffers when all you want is to transmit. If you separate out Control then the fact that virtualized interfaces can’t access control is explicit (the virtualizing struct does not expose Control). Generally speaking, you always want to separate control from data. For asynchronous packet interfaces, you want to separate transmit from receive. Phil

…

On Jul 5, 2018, at 4:02 PM, Brad Campbell ***@***.***> wrote: Idea: go all the way and add a UartDevice trait (like I2C and SPI) that has the UartParameters baked in. Pros: More consistent. Can be used to separate code that needs a capability to access hardware and code that doesn't. Makes it easier to determine when to call configure() (only when the actual user of the bus changes does any code need to even consider calling configure() again. Cons: Why put in a bunch of code and complication for an interface that is very unlikely to not need all this complexity, given how UART busses are commonly used (between exactly two devices with fixed settings)? How would a receiver handle getting an unpredictable ordering of UART messages at different baud rates, parity, stop bits, and flow control settings (as could happen with something like virtual_uart.rs)? What might be more useful, and Tock-esque, is to have a UartDevice trait that: removes the .configure() function removes the .abort_receive() function Then any normal user of the UART bus has access, it could be easily virtualized, and it doesn't have to worry about the bus settings and instead uses whatever was configured by something else (likely a board's main.rs). It also matches the use case we have so far: console and debug.rs sharing a UART channel. Anything more advanced simply has to use the Uart trait, and then knows it has exclusive access to the uart bus and can change the settings as it pleases. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1072 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEG3a3QN7AJPb5Qae-T2ujqi7zHYHPztks5uDpr0gaJpZM4VDH9d>.

[phil-levis]

https://groups.google.com/forum/#!topic/tock-dev/3NtJfzuz-ag

Currently, many of the Tock HILs assume they are not virtualized: there is a single client, who makes the calls and receives the callbacks. I’ll follow the TinyOS convention and call this a “dedicated” interface.

Often, you need to virtualize things. E.g., you want multiple capsules to be able to send packets. So you virtualize the underlying dedicated send interface with a queue. When a client calls send, its packet is put on the queue. The queue serializes the sends to a dedicated interface. When a particular packet send completes, the queue signals completion to the client that sent that packet. Or, you virtualize timers, so a single hardware timer can be turned into many software timers. Or, you virtualize random number generation, such that many clients needing random numbers can make requests and later get distinct random numbers.

As we start introducing virtualization throughout the Tock kernel, we’re going to need to redesign several of the HILs. I’ll try to sketch out a few principles that virtualizable interfaces need to have.

First, you need to separate control and data operations into separate traits. There are cases when a virtualized client can perform control operations. E.g., a virtualized SPI client can configure pin polarity and clock phase on the SPI, to match the device connected on the bus. However, a virtualized UART can’t change the receive speed, because that will change it for all clients and is generally expected to be constant. Similarly, a virtualized SPI client probably can’t change its chip select line. If the traits are not separated, then any client that can call data methods can call control methods. Allowing to to call control methods but then ignore them pushes a bug that could be obviously found at compile time to a hidden bug at runtime.

Second, you need to separate different data operations into separate traits. Different data operations have different virtualization approaches. Packet transmit virtualization introduces a queue, for example, and requires some careful buffer management between virtualized clients and the underlying dedicated service. Packet receive virtualization, in contrast, is either dispatch (each client has a dedicated receive based on some byte in the header, such as TCP port) or copy (received data is copied into every receive buffer).

For example, here’s our current (v1.2) UART trait:

pub trait UART {
    fn set_client(&self, client: &'static Client);
    fn init(&self, params: UARTParams);
    fn transmit(&self, tx_data: &'static mut [u8], tx_len: usize);
    fn receive(&self, rx_buffer: &'static mut [u8], rx_len: usize);
    fn abort_receive(&self);
}

pub trait Client {
    fn transmit_complete(&self, tx_buffer: &'static mut [u8], error: Error);
    fn receive_complete(&self, rx_buffer: &'static mut [u8], rx_len: usize, error: Error);
}

I’d argue this should be split into five traits:

// Was initialize — changed name to be consistent with Pat’s distinction between
// initialization and configuration
pub trait Configure {
    fn configure(&self, params: UARTParams) -> ReturnCode;
}

pub trait Transmit {
    fn set_client(&self, client: &'static TxClient) -: ReturnCode;
    fn transmit(&self, tx_data: &'static mut [u8], tx_len: usize) -> ReturnCode;
    fn abort_transmit(&self) -> ReturnCode;
}

pub trait Receive {
    fn set_client(&self, client: &'static Client) -> ReturnCode;
    fn receive(&self, rx_buffer: &'static mut [u8], rx_len: usize) -> ReturnCode;
    fn abort_receive(&self) -> ReturnCode;
}

pub trait TransmitClient {
    fn transmit_complete(&self, tx_buffer: &'static mut [u8], rval: ReturnCode);
}

pub trait ReceiveClient {
    // ReturnCode is for generic control/system errors; Error is for UART-specific
    // information, such as parity errors, etc.
    fn receive_complete(&self, rx_buffer: &'static mut [u8], rx_len: usize, rval: ReturnCode, error: Error);
}

[ppannuto]

Dropping a link to #1045 (review) here, which includes a nice summary of some problems with the UART "virtualization" as-is.