How should this crate work?

diondokter commented 4 years ago

Hey everyone! I could use some feedback on this, so any is appreciated.

Goal of the crate

I've done a talk at the Oxidize conf about writing the 'best' device driver. There was some interest, so I want to see where this can go.

The goal is to make a toolkit so that writing device drivers will be easier and faster to do.

Some features it should have:

no_std and using the stable rust compiler
Assist in creating common low-level layers (like registers, memory space, streaming data)
Allow for building small and extensive high level layers
Unify device features like pin assignment and power/sleep management
Allow the device-driver author to decide how extensively the driver should be written, but give a clear path for expansion

General structure

Every driver has to implement the Device trait. This is the base trait that signifies the high level driver interface. The functionality would also be implemented as traits.

/// General Device trait
pub trait Device {
    type Interface;
    type Error;

    /// Create a new instance of the device with the given interface
    fn new(interface: Self::Interface) -> Result<Self, Self::Error>
        where Self: Sized;

    fn reset(&mut self) -> Result<(), Self::Error>;
}

/// This device contains registers
pub trait RegisterDevice<'a> : Device {
    type RegisterSet: RegisterSet<'a, Self::Interface, Self::Error>;

    /// Get access to the registers. This access borrows the interface.
    /// *NOTE* Using this can conflict with high-level functionality. Make sure not to break any assumptions that the crate makes.
    fn registers(&'a mut self) -> Self::RegisterSet;
}

/// This is the low-level access to the registers
pub trait RegisterSet<'a, Interface, Error> : Sized {
    fn new(interface: &'a mut Interface) -> Self
        where Self: Sized + 'a;
}

/// This device has a memory space (like eeprom, or a radio rx/tx buffer)
pub trait MemoryDevice<'a> : Device {
    type MemorySpace: MemorySpace<'a, Self::Interface, Self::Error>;

    /// Get access to the memory space. This access borrows the interface.
    /// *NOTE* Using this can conflict with high-level functionality. Make sure not to break any assumptions that the crate makes.
    fn memory(&'a mut self) -> Self::MemorySpace;
}

/// This is the memory space
pub trait MemorySpace<'a, Interface, Error> : Sized {
    fn new(interface: &'a mut Interface) -> Self
        where Self: Sized + 'a;
}

/// This device can be powered on and off
pub trait PowerManagedDevice : Device {
    fn power_off(&mut self) -> Result<(), Self::Error>;
    fn power_on(&mut self) -> Result<(), Self::Error>;
}

/// This device can be put to sleep
pub trait SleepManagedDevice : Device {
    type SleepMode;

    fn sleep(&mut self, mode: Self::SleepMode) -> Result<(), Self::Error>;
    fn wakeup(&mut self) -> Result<(), Self::Error>;
}

Motivation for using traits like this (especially the power-related ones):
One could have a project where power efficiency is very important. You could then have an iterator over all devices and turn them all off when it's time to go into deep sleep.
It also falls in line with implementing the component abstraction traits.

Other functionality not covered by the existing traits can be implemented by the driver author. Though I expect this should only be high-level functionality like reading out a temperature value or parsing a gps string.

Creating low-level functionality

Writing the low level functionality can be a lot of work depending on the type of device. It could be simple like one memory space for an EEPROM, but it could also be 50 registers with 16 fields each.

So especially for registers, some help would be nice.

Register API

The API to access registers can be the same as the API used in the PAC crates for the Arm MCU's. (See embedded book)

My Oxidize talk was mainly about this. Here's an example of the code that can create such an API: gist. (scroll down to line 178 to see the API in action) However, implementing this by hand, like in the gist, is a lot of boring work. So there needs to be a means of being able to skip all the boilerplate and just focus on the register definitions themselves.

One solution is to create a macro. This is the solution I picked for a device driver I wrote. The syntax was something along the lines of this:

// The registers of my_device have a wordsize of u16

define_registers!(my_device, {
    /// Read and write register docs
    register_name, register_address, RW {
        /// Write only field docs. FieldType must implement From<u16>
        field_name: FieldType = WO 0..8;
        /// Read and Write field docs
        field_name_2: bool = RW 8..=8;
    },
    /// Read only register docs
    register_name_2, register_address, RO {
        /// Field docs
        field_name: u16= RO 0..16;
    }
});

This works pretty well in my experience. This macro could also be expanded with reset/default values and other thing that will be found along the way.

Another option would be to use some kind of file-based definition. This has the advantage that we could maybe join other initiatives. One of which is CyanoByte. This could help even more in creating device drivers for Rust. However, CyanoByte does not seem to be a very active project, only supports I2C devices as of yet and uses YAML (which I personally don't like).

I don't know of any other such project. If you do, please let me know!

If we don't adhere to an existing standard, then we could do our thing and could maybe create a file structure in TOML.

What do you think is better? Macro's, generation or maybe something else?

Memory space API

I don't know what this should look like yet. Maybe you've got ideas?
Usually you've got either single byte/word access and/or (limited) streaming access where the address pointer automatically increments.

Streaming data API

Another common thing you see are devices that just push out data whenever it's got it. Usually this is done over Serial. An example of this is the L80 GPS that spits out some NMEA strings every second. This to me seems like the most difficult API to create because the data being streamed can be very different from device to device.

Conclusion

Do you see something I've not covered? Do you have opinions you'd like to voice?

Any feedback is welcome! Even if it's that the effort would be wasted and better spent somewhere else or that the ambition is too big to get right.

I'd love to get people to use this, but before they do, they need to like it.

TheZoq2 commented 4 years ago

Great initiative, the things you showed in your talk looked super interesting!

/// NOTE Using this can conflict with high-level functionality. Make sure not to break any assumptions that the crate makes.

What does this mean?

What do you think is better? Macro's, generation or maybe something else?

Both have merits. Macros would probably be a bit less expressive (maybe), may increase compile time(?), and often breaks autocompletion in rls/rust-analyser. On the other hand, a file based approach would need to generate a new crate, be run separately from the compiler, probably. It would make it easier to re-use the data for other purposes though...

The API to access registers can be the same as the API used in the PAC crates for the Arm MCU's. (See embedded book)

Quick thought: the PAC interface is neat, but the high amount of zero cost abstraction can be annoying in debug mode. I believe there is a project for writing another PAC-like interface that uses less "expensive" zero cost abstraction, but I can't remember the name right now, peripheral something something.

Might be worth looking into though.

tl8roy commented 4 years ago

Memory space API

I am thinking of writing a storage embedded hal trait. The current interface in my head is read(usize) -> Word, write(usize, Word), read_slice(usize, [Word]), write_slice(usize,[Word]). So I would think the memory space API would want to use something similar.

Streaming data API

I think a generic Streaming API is probably in the too hard basket, I think there is more value in dealing with the actual data like ATAT does for AT devices or a GPS format crate.

Sleep

Some kind of Sleep trait would be nice too. But it is also very device specific.

hargoniX commented 4 years ago

I don't really like the high level traits here to be honest, for example the power trait.

/// This device can be powered on and off
pub trait PowerManagedDevice : Device {
    fn power_off(&mut self) -> Result<(), Self::Error>;
    fn power_on(&mut self) -> Result<(), Self::Error>;
}

forces the developer away from using type states for this transition, in case a certain set of functionality is only supported for the chip as long as it is powered down/up. While we can represent that with type states and PhantomData without this trait in order to enforce that this driver is not misused by the consumer when we handle this sort of state transition via this trait that seems impossible to me. Same goes for stuff like the reset() function of

/// General Device trait
pub trait Device {
    type Interface;
    type Error;

    /// Create a new instance of the device with the given interface
    fn new(interface: Self::Interface) -> Result<Self, Self::Error>
        where Self: Sized;

    fn reset(&mut self) -> Result<(), Self::Error>;
}

Maybe some people would instead like to change a type state that indicates the chip is in its reset state instead of , also impossible with this API.

ryan-summers commented 4 years ago

I have to agree with @hargoniX on this - when we force device driver crates to conform to some strictly defined interface, it can make it hard to adopt. For example, often-times, a device driver may want to take in use-once GPIO when instantiating a device (e.g. tied to reset lines or asynchronous edge trigger signals) to bring the device up from a cold boot. With the proposed Device::new(), that's not really possible. I think in general, we should try to implement this while imposing the minimal amount of requirements on the driver developer.

/// This device can be powered on and off
pub trait PowerManagedDevice : Device {
    fn power_off(&mut self) -> Result<(), Self::Error>;
    fn power_on(&mut self) -> Result<(), Self::Error>;
}

/// This device can be put to sleep
pub trait SleepManagedDevice : Device {
    type SleepMode;

    fn sleep(&mut self, mode: Self::SleepMode) -> Result<(), Self::Error>;
    fn wakeup(&mut self) -> Result<(), Self::Error>;
}

In regards to power states, devices can often have a multitude of powerstates, such as a radio that is in a low-power polling mode (WOR), but is not completely powered off. I think power state management is a bit complex here. I would also start off simple here and start with just the register API and work up from there.

What do you think is better? Macro's, generation or maybe something else?

I would go for the macro approach personally, due to the following:

Easy to update the definitions when a mistake is found (don't have to run some external tool to do the codegen each change)
Stores the register definitions directly next to the code that uses it, which increases visibility.
Reduces the development overhead (only one single source file required)
Simpler to develop - just a rust macro instead of file definitions + external tooling

eldruin commented 4 years ago

I think it is worth mentioning register-rs here.

diondokter commented 4 years ago

To summarize what I've heard so far:

There's a slight preference to using macros for low level generation.
Maybe the generation parts should be separate from this effort?
There are concerns about the restrictiveness of using traits to define functionality.
People really like to do typestate.

/// NOTE Using this can conflict with high-level functionality. Make sure not to break any assumptions that the crate makes.

What does this mean?

The high level code may set a register to a certain value and expect it to stay that way. If you then change it without the high level code knowing about it, then it could start malfunctioning. I do feel you'd still want access, though, because not all high-level layers are complete and you may need other functionality that hasn't been implemented.

Quick thought: the PAC interface is neat, but the high amount of zero cost abstraction can be annoying in debug mode. I believe there is a project for writing another PAC-like interface that uses less "expensive" zero cost abstraction, but I can't remember the name right now, peripheral something something.

You can easily optimize the crates you're using from debug mode by adding this to your Cargo.toml:

[profile.dev.package.my-chip-driver]
opt-level = 3

Memory space API

I am thinking of writing a storage embedded hal trait. The current interface in my head is read(usize) -> Word, write(usize, Word), read_slice(usize, [Word]), write_slice(usize,[Word]). So I would think the memory space API would want to use something similar.

It could be nice for this library to use that trait. Then any library expecting some storage could use devices that use this toolkit.

Streaming data API

I think a generic Streaming API is probably in the too hard basket, I think there is more value in dealing with the actual data like ATAT does for AT devices or a GPS format crate.

You're probably right. I'm hoping someone has a good idea for this. Otherwise, these kind of devices may simply be out of scope to provide low level api generation for.

Sleep

Some kind of Sleep trait would be nice too. But it is also very device specific.

In regards to power states, devices can often have a multitude of powerstates, such as a radio that is in a low-power polling mode (WOR), but is not completely powered off. I think power state management is a bit complex here.

Yes, very specific. But I think it's possible to generalize. If anything more specific is needed, then the user would simply not call it through the trait API, but via the crate's custom high-level layer.

I have to agree with @hargoniX on this - when we force device driver crates to conform to some strictly defined interface, it can make it hard to adopt. For example, often-times, a device driver may want to take in use-once GPIO when instantiating a device (e.g. tied to reset lines or asynchronous edge trigger signals) to bring the device up from a cold boot. With the proposed Device::new(), that's not really possible. I think in general, we should try to implement this while imposing the minimal amount of requirements on the driver developer.

I agree as well. The traits proposed here are only to give an idea of the direction I see this going. Defining good ones will be the most challenging part of this crate. For example, there should be a way with extra pins like the reset pin.

adamgreig commented 4 years ago

Quick thought: the PAC interface is neat, but the high amount of zero cost abstraction can be annoying in debug mode. I believe there is a project for writing another PAC-like interface that uses less "expensive" zero cost abstraction, but I can't remember the name right now, peripheral something something.

I think you're thinking of https://github.com/adamgreig/stm32ral which provides a very lightweight and quick to compile macro-based interface which looks like:

use stm32ral::{modify_reg, gpio};
let gpioa = gpio::GPIOA::take().unwrap();
modify_reg!(gpio, gpioa, MODER, MODER1: Input, MODER2: Output, MODER3: Input);

The crate provides modules for each peripheral and register which contains constants for values you might write to the fields. The macro is expanded into a simple mask-and-shift expression, so it produces small and fast code even without any compiler optimisations turned on, which makes debugging a lot easier too. Personally I prefer the syntax compared to the closure-with-method-chaining of svd2rust, too.

tl8roy commented 4 years ago

Memory space API

I am thinking of writing a storage embedded hal trait. The current interface in my head is read(usize) -> Word, write(usize, Word), read_slice(usize, [Word]), write_slice(usize,[Word]). So I would think the memory space API would want to use something similar.

It could be nice for this library to use that trait. Then any library expecting some storage could use devices that use this toolkit.

I will try to make some progress in the next week or 2 then.

eldruin commented 4 years ago

There has been some discussion about a flash/memory API here. I think you can skip the usize is most of those methods, since the slice will already tell you how long it is. Here is an example of a generic EEPROM driver.

diondokter commented 4 years ago

There has been some discussion about a flash/memory API here. I think you can skip the usize is most of those methods, since the slice will already tell you how long it is. Here is an example of a generic EEPROM driver.

The usize is probably the start address.

eldruin commented 4 years ago

Thanks. My brain immediately saw a pointer+length like in C :)

hargoniX commented 4 years ago

There is also this https://github.com/jonas-schievink/spi-memory which might be a good playground to try some approaches

regexident commented 4 years ago

A device trait like this would assume that a device can only use one interface:

pub trait Device {
    type Interface;
    // ...
}

In order to nicely allow for providing multiple interfaces the trait would have to look more like this:

pub trait Device<Interface> {
    // ...
}

impl Device<I2c> for Foo42 { … }

impl Device<Spi> for Foo42 { … }

(Your talk's code matches the latter, for what it's worth)

diondokter commented 4 years ago

(Your talk's code matches the latter, for what it's worth)

You're quite right! Didn't think of that.

therealprof commented 4 years ago

Something like a proc macro creating the interface from a yaml (or similar) description would be nice. 😅

hargoniX commented 4 years ago

For generating from yaml files I'd like to point at https://github.com/google/cyanobyte, as of now the "standard" they have is just I2C but since the file format is essentially only a YAML defined via a JSON Schema we should be able to fork it and add required changes (maybe even upstream them as cyanobyte was planning to look into SPI etc anyways?). Plus we'd already have a nice set of files in that repo we can easily create test drivers with and try them out against real hardware while developing this crate.

ryankurte commented 4 years ago

hey this is a super interesting project! i would love to have a better mechanism for describing registers / fields / etc. than the current write-it-all-out-by-hand approach. it's probably a bunch more primitive than what is proposed here but, i have been playing with similar abstractions across the few radio driver crates i have, trying to extract commonality and smooth the driver writing / maintenance situation.

it's all a little cursed due to the outstanding transactional SPI PR but, some links if you're interested: trying to abstract over underlying SPI / UART protocols [1] (though i haven't yet implemented the UART one), as well as common driver functions [2] [3] to simplify driver type bounds, providing blocking [4] and nonblocking [5] accessors over classical / c-style do/poll/done drivers, and hal abstractions to simplify writing utilities to test and interact with drivers [6].

hargoniX commented 4 years ago

It would also be pretty cool if we could have a separation of registers and protocols used to control said registers, for example OpenBSD has an IC directory containing register definitions https://github.com/openbsd/src/tree/master/sys/dev/ic as well as several bus specific directories such as I2C, SPI, PCI etc. in the directory above the IC one. I think this separation of bus and registers might be a pretty interesting thing to have, of course for code re-usage but also because if we have the register definition strictly separate from the bus specific implementation writing mock tests would be a lot easier for the developer.

hargoniX commented 4 years ago

As it seems like the acivity on this issue regarding new features we'd like to have has stopped for now. What I get from this is that there is definitely a desire to have 2 parts to this crate

Low level APIs inspired by PAC design that provide a unified way of accessing the information (in whatever form it should be) on the chip the driver is being written for. This lower level API should definitely be auto generated as well, just like a PAC. The biggest questions remaining I can see are:
- Should we generate it with a (proc-)macro or from a file instead (like svd2rust for example)
- We can agree on the PAC like interface for register based chips but how do we handle streaming and memory based API? (would be nice to see some actual code proposed for this @tl8roy mentioned he wanted to get something going a bit ago, did something drop out of that?)
Higher Level APIs The main point for the traits proposed here was "not generic enough" if I understood everything correctly.

In order to discuss these questions it might be reasonable to split the current thread up into a few more issues so we can discuss everything that's separate separately?

(If something is missing from the questions I collected feel free to bring it up of course)

diondokter commented 4 years ago

Yes that's what I got as well from the responses. I've been working on and off to get some basic thing going to get feedback on.

For now it will be a normal macro that will help you create a device with registers. The high level stuff can wait for I think. For defining memory space, I'd like to use this when it eventually lands: https://github.com/rust-embedded/embedded-hal/pull/241

Don't know how soon I can finish it, but I'll have some time off in September which I hope proves to be productive for this.

emosenkis commented 4 years ago

What about async vs blocking?

diondokter commented 4 years ago

I've released a first version! It only does registers for now.

https://crates.io/crates/device-driver

Try it out and look at the example if you want. If you have ideas, then please post them here! For specific problems, please open a new issue. For now I'll keep this open for general discussions.

Also, I don't have a lot of Open Source experience. So I'm open for process/style suggestions as well 🙂

TheZoq2 commented 4 years ago

Great news :tada: I'll look into this when I find time :)

Also, I don't have a lot of Open Source experience. So I'm open for process/style suggestions as well slightly_smiling_face

From my f103_hal experience, keeping a structured changelog is really good. For every new feature or fix, add a note to the changelog. Generally, this is much easier to look through than commit messages and makes upgrading particularly breaking changes much simpler.

This is the format we use: https://keepachangelog.com/en/0.3.0/

diondokter / device-driver