Robbepop / modular-bitfield

Macro to generate bitfields for structs that allow for modular use of enums.
Apache License 2.0
159 stars 41 forks source link

Automatically implement generic bitfield trait for #[bitfield] structs #40

Open henrikssn opened 3 years ago

henrikssn commented 3 years ago

Would it be possible to create a trait where the Bitfield methods are defined? Or maybe this already exists?

Thinking about the following:

I want to create a generic function that takes bitfields and writes them to a device, but currently I don't have anything to enforce type bounds on.

Robbepop commented 3 years ago

Hi! Can you please make an example of your use case? I wonder, would it be possible to just define this trait on your side and simply implement it for all your bitfields manually? This could even be done using a macro_rules! macro if there are too many bitfields.

henrikssn commented 3 years ago

Say I have the following structs:


pub trait Register {
  const ADDRESS: u8;
}

// A lot (>60) of these:
#[bitfield]
#[derive(Copy, Clone, PartialEq, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct MyRegister {
  pub field_one: B2,
  pub field_two: B4,
  pub field_three: B2,
}

impl Register for MyRegister {
  const ADDRESS: u8 = 0x01;
}

and these two methods (note: this is the code which isn't working):


pub fn read_register<R>(&mut self) -> Result<R, Error<MyError>>
where
    R: Register + Bitfield<Bytes = u8>,
{
    let data = self.read_reg(R::ADDRESS)?;
    let reg = R::from_bytes(data).unwrap();
    Ok(reg)
}

pub fn write_register<R>(&mut self, reg: R) -> Result<(), Error<MyError>>
where
    R: Reg + Bitfield<Bytes = u8>,
{
    self.write_regs(R::ADDRESS, reg.as_bytes())?;
    Ok(())
}

The goal is to have the Register classes capture the address and the data, so that I can generalize the read/write methods over all registers.

Robbepop commented 3 years ago

Okay I got your use case. But couldn't you simply write your own Bitfield trait and simply implement this Bitfield trait for all your concrete bitfield types? I should be pretty straight forward:

pub trait Bitfield {
    type Bytes;

    fn new() -> Self;
    fn from_bytes(bytes: Self::Bytes) -> Self;
    fn into_bytes(self) -> Self::Bytes;
}

and then for MyRegister:

impl Bitfield for MyRegister {
    type Bytes = [u8; 2]; // Or whatever is the actual underlying type.

    fn new() -> Self {
        MyRegister::new()
    }

    fn from_bytes(bytes: Self::Bytes) -> Self {
        Self::from_bytes(bytes)
    }

    fn into_bytes(self) -> Self::Bytes {
        self.into_bytes()
    }
}
henrikssn commented 3 years ago

Thanks, that would work but brings a lot of boilerplate - remember I have quite a few of those register bitfields to implement.

Actually, I found Specifier which is very close to what I need. This works:

#[bitfield(specifier = true)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct MyRegister {
  pub field_one: B2,
  pub field_two: B4,
}

But if I complement the type to be 8 bits long, it crashes in compilation:

error: this arithmetic operation will overflow
  --> src/register.rs:79:1
   |
79 | #[derive(Copy, Clone, PartialEq, Debug)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to shift left by 8_usize which would overflow
   |
   = note: `#[deny(arithmetic_overflow)]` on by default
Robbepop commented 3 years ago

Yes, you need to add filled = false since you do not fill all the available bits in your #[bitfield].

#[bitfield(specifier = true, filled = false)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct MyRegister {
  pub field_one: B2,
  pub field_two: B4,
}
henrikssn commented 3 years ago

Sorry for not being clear, this is the example which does not compile:

#[bitfield(specifier = true)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct MyRegister {
  pub field_one: B2,
  pub field_two: B4,
  pub field_three: B2,
}
error: this arithmetic operation will overflow
  --> src/register.rs:79:1
   |
79 | #[derive(Copy, Clone, PartialEq, Debug)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to shift left by 8_usize which would overflow
   |
   = note: `#[deny(arithmetic_overflow)]` on by default
Robbepop commented 3 years ago

Well I don't know why but it compiles completely fine on my machine. :upside_down_face:

use modular_bitfield::prelude::*;

#[bitfield(specifier = true)]
#[derive(Copy, Clone, PartialEq, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct MyRegister {
  pub field_one: B2,
  pub field_two: B4,
  pub field_three: B2,
}

fn main() {}

I tried with and without --release and with stable as well as current nightly. Always succeeds.

Robbepop commented 3 years ago

I toyed some more and now also stubled upon your error. It can be triggered with cargo clippy which is why cargo build and cargo run worked for me. Seems like I fixed it with the most recent PR.

Robbepop commented 3 years ago

With the ability to use #[derive(BitfieldSpecifier)] on a #[bitfield] struct which implements the Specifier trait which is probably what this issue asks for: Is there still a need for this feature or can this be closed?

TrueBers commented 3 years ago

Seems like I have the same:

#[bitfield(bits = 64, filled = false)]
#[derive(Debug, BitfieldSpecifier)]
pub struct Cr0Flags {
    pub protection_enable: bool,
    pub monitor_coprocessor: bool,
    pub emulation: bool,
    pub task_switched: bool,
    pub extension_type: bool,
    pub numeric_error: bool,
    _reserved0: B10,
    pub write_protect: bool,
    _reserved1: B1,
    pub alignment_mask: bool,
    _reserved2: B10,
    pub non_writethrough: bool,
    pub cache_disable: bool,
    pub paging: bool,
}

got an error:

46 | / #[derive(Debug, BitfieldSpecifier)]
47 | | pub struct Cr0Flags {
48 | |     pub protection_enable: bool,
49 | |     pub monitor_coprocessor: bool,
...  |
62 | |     pub paging: bool,
63 | | }
   | |_^ attempt to shift left by `8_usize`, which would overflow
   |
   = note: `#[deny(arithmetic_overflow)]` on by default

in both release and dev profiles on the latest nightly. Didn't try stable one.

Robbepop commented 3 years ago

Seems like I have the same:

#[bitfield(bits = 64, filled = false)]
#[derive(Debug, BitfieldSpecifier)]
pub struct Cr0Flags {
    pub protection_enable: bool,
    pub monitor_coprocessor: bool,
    pub emulation: bool,
    pub task_switched: bool,
    pub extension_type: bool,
    pub numeric_error: bool,
    _reserved0: B10,
    pub write_protect: bool,
    _reserved1: B1,
    pub alignment_mask: bool,
    _reserved2: B10,
    pub non_writethrough: bool,
    pub cache_disable: bool,
    pub paging: bool,
}

got an error:

46 | / #[derive(Debug, BitfieldSpecifier)]
47 | | pub struct Cr0Flags {
48 | |     pub protection_enable: bool,
49 | |     pub monitor_coprocessor: bool,
...  |
62 | |     pub paging: bool,
63 | | }
   | |_^ attempt to shift left by `8_usize`, which would overflow
   |
   = note: `#[deny(arithmetic_overflow)]` on by default

in both release and dev profiles on the latest nightly. Didn't try stable one.

Thank you for the failing example. Does it also occur with the most recent version of modular_bitfield? Which version are you using?

TrueBers commented 3 years ago
[dependencies.modular-bitfield]
version = "0.11"
xgroleau commented 2 years ago

I ran in a similar need to have a generic bitfield trait. I tried using the Specifier trait, but it would require const generic expression, which are not available at the moment.