diondokter / device-driver Goto Github PK

A way to say to the macro that I do not want a fallible interface for register decoding (enums).
So if it can detect that an fallible interface would be generated it should scream at me, or maybe a trait error. :)

The infallible interface would also be nice if it could be related to the bit size, e.g. a 2 bit field that is linked to a 4 variant enum (or less + FromPrimitive implemented).
Then it is known that the entire value span is well defined.

I think this is possible with a newer bitvec version. Hopefully the compiler will then be smart enough to always inline as much as possible

The links on https://crates.io/crates/device-driver/0.5.0 for the various examples go 404 -- they contain twice the /device-driver/ path component.

I'd file this as a PR, but I have no clue as to which magic makes the relative references in there work on crates.io in the first place – is there some regeneration of the README at release time involved? (Might be related to the tags not being set).

If there is no file mangling magic, please let me know and close this issue; then, this must be some crates.io mixup, maybe as part of processing the readme.workspace attribute.

I'm writing a driver for the MAX17302 that uses 16 bit words over an I2C interface. Page 140 of the datasheet states:

With 2-Wire communication, a byte of data consists of 8 bits ordered most significant bit (MSb) first. The least significant bit (LSb) of each byte is followed by the Acknowledge bit. IC registers composed of multibyte values are ordered least significant byte (LSB) first.

The load_be used in the implement_register_field macro seems to be loading the values returned over the wire incorrectly. I'm using full-width fields like so:

// Create a register set for the device
implement_registers!(
    /// The global register set
    Max17302.registers<u16> = {
        avgcurrent(RW, 0x01D, 2) = { avgcurrent: u16 = RW 0..16 },
    }
);

I forked device-driver and changed the two occurrences of load_be to load_le in implement_register_field and now the values are returned as expected.

Should it be possible to specify the endianness of device-driver, or is load_be a bug?

Hi,

For example when I read a register it would be really nice in development to be able to add debug prints of the register.
Eg, lets say I have a register with 2 flags:

implement_registers!(
    MyChip.registers<u8> = {
        status(RO, 0, 1) = {
            flag1: u8 as Bit = RO 0..=0,
            flag2: u8 as Bit = RO 1..=1,
        },
        // ...
    }
}

Debug-printing with {:?} I'd expect something like status::R { flag1: Set, flag2: Cleared }, and with {:#?}

status::R { 
    flag1: Set, 
    flag2: Cleared 
}

It would also be preferable if one use custom enums that there are also debug printed with correct variants.

I have noticed that generating dual definitions within a register works fine, for example:

        // #[generate(Debug)]
        tuning_r(RW, 0x1d, 1) = {
            dl_tune: u8 = RW 4..=7,
            lead_lag: u8 = RO 3..=3,
            cagainov: u8 = WO 3..=3,
            integlen: u8 = RW 2..=2,
            integgain: u8 = RW 0..=1,
        },

The bit which has different meaning when writing and reading is generated correctly in the R and W.
However when I add the #[generate(Debug)] the debug tuple is not correctly constructed with the debug info for the WO bit is in the R debug print impl:

            impl core::fmt::Debug for R {
                fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> {
                    f.debug_struct("tuning_r::R")
                        .field(
                            "raw",
                            &device_driver::utils::SliceHexFormatter::new(&self.0),
                        )
                        .field("dl_tune", &self.dl_tune())
                        .field("lead_lag", &self.lead_lag())
                        .field("cagainov", &self.cagainov())
                        .field("integlen", &self.integlen())
                        .field("integgain", &self.integgain())
                        .finish()
                }
            }

So to me it seems that the #[generate(Debug)] does not handle WO bits correctly as they should not be in the debug print.
What do you think @diondokter ?

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.

Syntax should be left the same, but this would support more complex macros more easily

I'm looking into implementing driver for mcp2515, and I'm trying to create nice SPI registers abstraction.

The issue is that for many operations (eg. marking irq as acknowledged) require use of "Bit Modify" operations (to prevent race conditions, eg when device receive message between read and write operation to registers).

This require executing bit modify operation with bit mask (what bits we want to set) and new value (bits outside bit mask are ignored).

The issue that I'm having is how to create register, set some bit fields there, and then get mask for modified fields.

If you do this, the correct value is written.

regs.mode_ctrl().write(|w| w.mode_ctrl(Mode::Lowpower).sleep_durn(SleepDuration::_1S))

However, when doing this, it writes 0.

regs.mode_ctrl().write(|w| {
    w.mode_ctrl(Mode::Lowpower);
    w.sleep_durn(SleepDuration::_1S);
    w
});

This was quite surprising to me, since the latter works correctly with svd2rust, and the API generated by this crate is very similar. I always write in the 2nd style, I find it more readable to have one field per line.