hecatia-elegua / bilge

Use bitsized types as if they were a feature of rust.
Apache License 2.0
173 stars 18 forks source link

Add optional/don't care fields and builder pattern #82

Closed xgroleau closed 1 year ago

xgroleau commented 1 year ago

We have multiple registers with "don't care" fields, it's a bit annoying to need to explicit every fields. Example:

/// Some register
#[bitsize(16)]
#[derive(Copy, Clone, PartialEq, Eq, DebugBits, TryFromBits, RWRegister, BilgeExt)]
#[register(ty = "RegisterAddress", addr = "RegisterAddress(0x01)")]
pub struct ADCControl {
    pub clock: Clock,
    pub mode: Mode,
    pub duty_cycle: DutyCycleRatio,
     /// Don't care field
    __reserved0: u1,
    pub internal_ref: bool,
    pub chip_select: bool,
    pub data_status: bool,
    pub continuous_read: bool,
    pub delay: DisableDelay,
    pub internal_ref_val: InternalReference,
    pub polarity: Polarity,
    /// Don't care  field
    __reserved1: u1, 
}

/// It's creation
        let cfg = ADCControl::new(
            Clock::External,
            Mode::Continuous,
            DutyCycleRatio::_1_16,
            u1::new(0), // Verbose and annoying to set
            true, // Not really clear what the field is here
            true,
            true,
            true,
            DisableDelay::_100ns,
            InternalReference::V1_25,
            Polarity::Bipolar,
            u1::new(0), // Verbose and annoying to  set
        );

It would be more explicit to have a builder pattern to know what are the boolean flags and it would also allow handling the "don't care" bits

        let cfg = ADCControl::builder()
            .with_clock(Clock::External)
            .with_mode(Mode::Continuous)
            .with_duty_cycle(DutyCycleRatio::_1_16)
            .with_internal_ref(true) // Clear on what those flags are
            .with_chip_select(true)
            .with_data_status(true)
            .with_continuous_read(true)
            .with_with_delay(DisableDelay::_100ns)
            .with_internal_ref_val(InternalReference::V1_25)
            .with_polarity(Polarity::Bipolar)
            // No need to specify the reserved bits
            .build(); // Maybe build would return a result in case all the required fields are not set

And we could add default to those fields to avoid to always needing to explicitly set them.

xgroleau commented 1 year ago

I've actually managed to extend Bilge using a derive macro. It shows how flexible the lib is. Now either we could want to add this by default or let the user extend bilge for the builder pattern.

Though maybe add to the documentation how ending the derive macro with *Bits allows to get the original fields.

hecatia-elegua commented 1 year ago

You're very right about the docs. About "don't care" fields, we don't handle __reserved0, but we do handle reserved and then you don't have to pass these to the new constructor (see the README for more info).

A derive for a type based builder pattern could be its own crate, you could even publish it yourself. On the other hand, I know bitbybit has some experimental builder shenanigans which I liked and it is annoying to set bitflags when you just do true, true, false, true in a constructor.....

xgroleau commented 1 year ago

Oh I haven't realized reserved bits where supported! I'll close this issue since make your own derive macro actually fix the builder issue.

And yes if I have some time I'll write a crate for the builder pattern, the current code is far from ready for general usage.