Skip to content

[RFC] An approach to providing higher level APIs #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
japaric opened this issue Jun 28, 2017 · 8 comments
Closed

[RFC] An approach to providing higher level APIs #18

japaric opened this issue Jun 28, 2017 · 8 comments
Labels

Comments

@japaric
Copy link
Member

japaric commented Jun 28, 2017

Why

We want to provide different sets of traits that operate at a higher level and
that are either tied to a specific async model (e.g. futures) or operate in
blocking mode because some drivers will be easier to built on top of those
higher level traits rather than on top of the low level traits we already have.

At the same time we want to maximize code reuse so implementers of the low level
traits should get an implementation of the higher level traits for free whenever
possible. However, we still want to give crate authors the freedom of being able
to write the implementations of the higher level traits themselves. This means
that embedded-hal should provide a default implementation of the higher
level traits for implementers of the low level traits, but in a way that makes
it possible for crate authors to override that default implementation.

How

This RFC proposes exposing such default implementations as free functions. Let's
see an example:

serial::Write is the low level trait for write operations on a serial
interface that we currently provide. The trait is non-blocking and operates at
the byte / word level:

pub trait Write<Word> {
    type Error;

    fn write(&self, word: Word) -> nb::Result<(), Self::Error>;
}

We also want to provide a blocking version of this Write trait:

pub mod blocking {
    pub trait Write {
        type Error;

        fn bwrite_all(&self, buffer: &[u8]) -> Result<(), Self::Error>;
        // ..
    }
}

But want to save authors of crates that already provide an implementation of
serial::Write for their types the effort of coming up with an implementation
of this new trait for their types. So we provide a default implementation of
blocking::Write methods for serial::Write implementers in the form of free
functions:

pub mod blocking {
    pub fn bwrite_all<S>(serial: &S, buffer: &[u8]) -> Result<(), S::Error>
    where
        S: serial::Write,
    {
        for byte in buffer {
            block!(serial.write(*byte))?;
        }
        Ok(())
    }
}

Now the crate authors can implement the new blocking::Write trait for their
types by just deferring the implementation to these free functions:

struct Serial;
struct Error;

// existing implementation
impl hal::serial::Write for Serial { type Error = Error; .. }

// just added
impl hal::blocking::Write for Serial {
    type Error = Error;

    fn bwrite_all(&self, buffer: &[u8]) -> Result<(), Error> {
        // use the default implementation
        hal::blocking::bwrite_all(self, buffer)
    }
}

However they still have the option of not using this default implementation
and writing an implementation that's better tailored for their types.

Alternatives

default impl

My original idea was to provide default blanket implementations of the higher
level traits for types that implemented the low level traits as shown below:

default impl<S> Write for S
where
    S: ::serial::Write<u8>,
{
    type Error = S::Error;

    fn bwrite_all(&self, buffer: &[u8]) -> Result<(), S::Error> {
        for byte in buffer {
            block!(self.write(*byte))?;
        }

        Ok(())
    }
}

This would leave room for overriding the default implementation through the
specialization mechanism.

However this doesn't work today. It seems that default implementations don't
support associated types (I think it's not decided whether specialization should
be allowed to change the associated type chosen by the default implementation):

error[E0053]: method `bwrite_all` has an incompatible type for trait
  --> src/blocking.rs:32:5
   |
11 |       fn bwrite_all(&self, buffer: &[u8]) -> Result<(), Self::Error>;
   |       --------------------------------------------------------------- type in trait
...
32 | /     fn bwrite_all(&self, buffer: &[u8]) -> Result<(), S::Error> {
33 | |         for byte in buffer {
34 | |             block!(self.write(*byte))?;
35 | |         }
36 | |
37 | |         Ok(())
38 | |     }
   | |_____^ expected trait `blocking::Write`, found trait `serial::Write`
   |
   = note: expected type `fn(&S, &[u8]) -> core::result::Result<(), <S as blocking::Write>::Error>`
              found type `fn(&S, &[u8]) -> core::result::Result<(), <S as serial::Write<u8>>::Error>`

error: aborting due to previous error(s)

No override mechanism

Another alternative is to provide a non overridable blanket implementation of
higher level traits for implementers of the low level traits:

// in embedded-hal

pub mod blocking {
    impl<S> Write for S
    where
        S: ::serial::Write,
    {
        type Error = S::Error;

        fn bwrite_all<S>(&self, buffer: &[u8]) -> Result<(), S::Error>
        where
            S: serial::Write,
        {
            for byte in buffer {
                block!(self.write(*byte))?;
            }
            Ok(())
        }
    }
}

With this approach crate authors won't have to implement blocking::Write
themselves. The problem is that they won't be able to override the default
implementations.

Supertraits

Yet another option is to make higher level traits supertraits of the low level
traits and provide the default implementation as default methods:

pub mod blocking {
    pub trait Write: ::serial::Write<u8> {
        fn bwrite_all<S>(&self, buffer: &[u8]) -> Result<(), S::Error>
        {
            for byte in buffer {
                block!(serial.write(*byte))?;
            }
            Ok(())
        }
    }
}

This allows specialization, but the downside of this approach is that to
implement the blocking::Write trait for your type you also have to implement
the serial::Write trait. However, this may not always be what you want: for
instance, you probably don't want this if implementing the higher level traits
for types that will be used on embedded Linux; you probably would prefer to
only implement the higher level traits for your type as the low level traits
don't map very well to Linux's I/O primitives.


cc @posborne @Kixunil

@japaric japaric added the RFC label Jun 28, 2017
@posborne
Copy link
Member

Very good writeup @japaric.

Off the bat, I think having a single implementation be required to provide the blocking implementation for an existing non-blocking implementation would be problematic. Although there are noted challenges with the await! approach for cooperative multitasking today, I would hope that whatever design we come up with for non-blocking traits would be potentially usable in a way that would allow for yielding control (the example implementation uses block!).

The supertrait approach might be realizable with some fudging under Linux for most peripherals but it would certainly lead to a potentially more leaky abstraction. Just thinking through major peripherals under Linux and if you could have something close enough to non-blocking to reasonably provide a real implementation of the nb trait.

  • Serial: Yes, possible. With non-blocking set, the kernel will actually buffer data unless full in which case it will EAGAIN.
  • SPI: Within Linux, SPI transfers use work queues so the API at the application layer may be used in a non-blocking fashion. That being said, using the low-level traits and sending a byte at a time would not work well in practice -- The kernel expects to get full transfers (or even sets of transfers) so they can be sent to hardware in an appropriate fashion. A similar higher-level non-blocking trait could be added based on these low-level (but easily implemented on an MCU) traits for something like SPI.
  • GPIO: Blocking is minimal, so a (close enough to) non-blocking API can probably be implemented well enough for basic operations.
  • ADC: I haven't worked with it much but the Linux IIO subsystem seems mature and can probably support a higher-level non-blocking API. Doing individual sample reads is probably possible as well but inadvisble -- higher-level non-blocking traits might be advisable in the same vein as what has evolved to form the Linux APIs.
  • I2C: I'll have to research more but initial investigation makes it seems like only a blocking API might be available.

Even with blocking APIs it might be possible to make use of a threadpool or similar to adapt these APIs to a non-blocking model which might have enough other benefits in aggregate. The main thread would essentially just be awaiting a result back from some other thread that is doing the work. I don't love this in general but it might be worthwhile in order to reap the other benefits of the async model like futures/tokio.

@japaric
Copy link
Member Author

japaric commented Jun 29, 2017

Very good writeup @japaric.

Thanks.

Off the bat, I think having a single implementation be required to provide the blocking implementation for an existing non-blocking implementation would be problematic.

Yes. The main proposal doesn't hardcode a trait hierarchy neither via supertraits or via blanket implementations so you can for example implement the high level blocking traits without implemeting the low level nb-style traits.

I would hope that whatever design we come up with for non-blocking traits would be potentially usable in a way that would allow for yielding control

The nb crate provides an await! macro to lift an expression that returns nb::Result into a generator to use with the async/await model. It also provides a try_nb! macro to lift the same expression into a future implementer. However both macros require the expression to not keep state in the program.

Just thinking through major peripherals under Linux and if you could have something close enough to non-blocking to reasonably provide a real implementation of the nb trait.

Note that it's possible to implement a high level futures-based set of traits without implementing the low level style traits in the case of embedded Linux. That being said I expect that at least serial would be able to directly implement the low level nb traits since serial I/O directly uses the read / write syscalls and nb is modeled after the Error::WouldBlock + try_nb! model that Tokio uses and std::io::{Read::read,Write::write} can be lifted into futures using tokio-io's try_nb!.

Even with blocking APIs it might be possible to make use of a threadpool or similar to adapt these APIs to a non-blocking model which might have enough other benefits in aggregate.

Do you know if tokio provides facilities to do that (doing I/O on another thread and having a future that indicates the completion of that I/O work)?

@hannobraun
Copy link
Member

I like this proposal. Only one minor nitpick: I don't like the name bwrite_all from the examples. It already lives in a module called blocking. write_all would be fine, in my opinion.

I've implemented your example in LPC82x HAL. I plan to submit this here later, after #22 has been merged.

@Kixunil
Copy link

Kixunil commented Dec 1, 2017

Sorry, I somehow missed this proposal. Any reason to prefer free functions instead of wrappers?

struct Blocking<T: Write<u8>> (T);

impl<T: Write<u8>> blocking::Write for Blocking<T> {
// ...
}

Another way to save authors from writing too much code:

/// Marker trait
trait DefaultBlocking {}

impl<T: Write<u8> + DefaultBlocking> blocking::Write for T {
// ...
}

This way all the author has to do is mark his type with DefaultBlocking trait.

@japaric
Copy link
Member Author

japaric commented Dec 1, 2017

@hannobraun you mean the free function? yes, that one can be named just write_all because modules provide scoping. I don't think the bwrite_all method should be renamed write_all though because you could have a futures variant of write_all with similar signature and you want to disambiguate them by name because it's easier to do so (if they have the same name then you have to type <MyType as SomeTrait>::some_method(&mut my_instance)).

@Kixunil I like the marker trait idea! I don't like the wrapper idea because it makes the difference between having a default implementation and a custom implementation too visible in end user code and I don't think the end user would be interested in how e.g. blocking::Write is implemented. For instance:

// in some third party library
fn foo<W>(w: &mut W ..) where W: blocking::Write { .. }

// user code
let mut w1: impl Write = ..;
let mut w2: impl Write = ..;

// w1 uses the default implementation
foo(&mut Blocking(w1));

// w2 has a custom implementation
foo(&mut w2);

// why calling foo should be different in each case?

Or maybe the wrapper is meant to be used in some other way?


For the record I have implemented two generic libraries / drivers using this idea (and the embedded-hal traits) and have tested that both can be used with Cortex-M microcontrollers (no_std) and the Raspberry Pi (std).

@hannobraun
Copy link
Member

@japaric

you mean the free function? yes, that one can be named just write_all because modules provide scoping. I don't think the bwrite_all method should be renamed write_all though because you could have a futures variant of write_all with similar signature and you want to disambiguate them by name because it's easier to do so (if they have the same name then you have to type <MyType as SomeTrait>::some_method(&mut my_instance)).

I did mean the trait method too, but your argument is convincing. I presume all of those traits would be in the prelude, making the collision problem relevant for all code that imports that.

@Kixunil
I've never come across that marker trait pattern before. I like it, very clever!

japaric pushed a commit that referenced this issue Jan 16, 2018
[RFC] v0.1.0 release

## Motivation

We want to get some traits out to encourage people to write more driver crates, generic crates that
interface external components, using these traits.

IMPORTANT A v0.1.0 does *not* mean we are going to stop adding new traits. It also does *not* mean
that we are stuck forever with the traits we already have; we can still tweak the existing traits
provided a good rationale to do so is presented.

## Detailed design

This RFC proposes releasing as "stable" all the traits that have been proved useful in building
proof of concept driver crates.

The other traits will be released as "unstable". They'll be hidden behind the "unstable" Cargo
feature.

Stable traits:

- `PwmPin`
- `Timer`
- `blocking::delay::Delay{Ms,Us}`
- `blocking::i2c::{Read,Write,WriteRead}`
- `blocking::spi::{Transfer,Write}`
- `serial::{Read,Write}`
- `digital::{OutputPin}`
- `spi::{FullDuplex}`

These have been proved viable in the reference implementation, [`stm32f30x-hal`], and several generic
drivers: [`mpu9250`], [`l3gd20`], [`lsm303dlhc`], [`mfrc522`], [`tb6612fng`]

[`stm32f30x-hal`]: https://github.com/japaric/stm32f30x-hal
[`mpu9250`]: https://github.com/japaric/mpu9250
[`l3gd20`]: https://github.com/japaric/l3gd20
[`lsm303dlhc`]: https://github.com/japaric/lsm303dlhc
[`mfrc522`]: https://github.com/japaric/mfrc522
[`tb6612fng`]: https://github.com/japaric/tb6612fng

Unstable traits:

- `Capture`
- `Pwm`
- `Qei`

The rationale of these being unstable is in the crate source code.

## Unresolved questions

- Should `OutputPin::is_{low,high}` be renamed to `OutputPin::is_output_{low,high}` to prevent a
  potential collision with a future `InputPin` trait that provides similarly named methods?

Now it's your chance to bikeshed all the names and the module hierarchy to your heart content.

## Changes in this PR

- Added `Default` marker traits as per #18 (comment)
- Blocking I2C traits
- Delay traits
- All doc tests have been unignored.
- The traits mentioned above have been feature gated

closes #25

cc @hannobraun @ahixon
@Kixunil
Copy link

Kixunil commented Jan 21, 2018

I somehow missed updates again. :D So the main advantage of wrapper over marker trait is that one could forget to implement blocking version. This can be solved by Write: blocking::Write bound, but maybe there are cases when it's too restrictive?

@japaric
Copy link
Member Author

japaric commented Feb 14, 2018

This has been implemented.

@japaric japaric closed this as completed Feb 14, 2018
peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
18: Move to digital v2 traits r=nastevens a=dbrgn

The v1 traits are deprecated because they don't have a way to report errors.

Gets rid of the deprecation warnings (which are hard errors until rust-embedded#17 is merged).

Co-authored-by: Danilo Bargen <mail@dbrgn.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants