avr-hal

avr-hal copied to clipboard

Some newer generations of AVR MCUs use a different IO-cell: There are 10 registers instead of 3 and one additional reg for each pin. E.g.

• DIR, DIRSET, DIRCLR, DIRTGL
• OUT, OUTSET, OUTCLR, OUTTGL
• IN
• INTFLAGS
• PIN0CTRL, PIN1CTRL, PIN2CTRL, PIN3CTRL, PIN4CTRL, PIN5CTRL, PIN6CTRL, PIN7CTRL

We need a common abstraction that allows both the old and the new IO-cell to be used with the exact same API. While integrating that, we can also rethink the entire IO HAL design. My proposal is (in general) to move away from the all macro based approach and do as much directly in avr-hal-generic with only the PAC-specific parts being implemented by a macro. For the IO HAL in particular, this could mean:

// in avr-hal-generic
pub struct Pin<PIN: PinOps, MODE: PinMode> {
    p: PIN,
    _mode: core::marker::PhantomData<MODE>,
}

pub trait PinOps {
    unsafe fn out_set(&mut self, value: bool);
    unsafe fn out_toggle(&mut self);
    unsafe fn set_direction(&mut self, output: bool);
    unsafe fn in(&self) -> bool;
}

// the macro then implements `PinOps` for concrete (zero-sized) types for each pin:
    pub struct PA0;

    impl PinOps for PA0 {
        #[inline(always)]
        unsafe fn out_set(&mut self, value: bool) {
            if value {
                (*<$PORTX>::ptr()).OUTSET.write(|w| 1 << $i);
            } else {
                (*<$PORTX>::ptr()).OUTCLR.write(|w| 1 << $i);
            }
        }

        // ...
    }

    // and similarly for the "old" IO-cell ...

Cc: @chris-ricketts

Cc: @chris-ricketts

@Rahix Thanks for linking those issues. I'll take those into consideration.

I'm still getting my head around the current ports API/macros and experimenting.

I think a single macro in a form similar to what @fneddy suggested would be possible for defining a chip's ports.

// Something like the following could be done, maybe
// if we use features to select pin impl macro
port!(PortA, [PA0, PA1, PA2, PA3]) 

// The could expand to 
pub struct PortA {
  pa0: Pin<PA0, Input<Floating>>
  ...
}

impl PortExt for PORTA {
  type Parts = PortA;

 fn split(self) -> Self::Parts {
    PortA {
      pa0: Pin::new(self.pa0)
      ...
  }
}


// avr_hal_generic/Cargo.rs
[features]
traditional = [] // needs better name
nextgen = [] // needs better name
avr_arch_avr51 = [ "traditional" ]
avr_arch_avr25 = [ "traditional" ]
avr_arch_avrxmega3 = [ "nextgen" ]

// avr_hal_generic/src/port.rs
pub struct RawPin<PIN>(PIN);

pub trait UnsafePinOps {
 unsafe fn set_direction(&mut self);
 ...
}

pub struct Pin<PIN, MODE> {
  pin: RawPin<PIN>,
  mode: MODE
}

impl<PIN> Pin<PIN, Input<Floating>> {
  fn new(pin: PIN) -> Self {
    let pin = RawPin(pin);
    ...
  }
}

macro_rules! port($PortX:ident, [$($PXi:ident,)+]) => { 
   // use impl_unsafe_pin_ops on each $PXi  
}

// different helper macros depending on  avr arch
#[cfg(feature = "traditional")] 
#[doc(hidden)]  
macro_rules! impl_unsafe_pin_ops($PortX:ident, $PXi:ident) => { 
  impl UnsafePinOps for RawPin<$PXi> {
    ...
  }
 }

#[cfg(feature = "nextgen")]   
#[doc(hidden)]     
macro_rules! impl_unsafe_pin_ops($PortX:ident, $PXi:ident) => { 
  impl UnsafePinOps for RawPin<$PXi> {
    ...
  }
}

I'm not sure how well the above would work yet or if you'd want to go down the features route but that's where my thinking is at atm.

I would not like to have architecture feature-flags on the avr-hal-generic crate. Instead, I would argue it should contain multiple macros for the different peripheral revisions which the HAL crate can then call based on which revision is built into the respective MCU. E.g.

macro_rules! impl_3regs_port {
    // ...
}

macro_rules! impl_10regs_port {
    // ...
}

I am also a bit unsure what the RawPin in your example is meant for. Can't we just use define a struct for each pin and use that? E.g.

pub struct PA0 {
    pub(crate) _private: (),
}

impl PinOps for PA0 {
    // ...
}

Or did I miss something?

I went ahead and built a rough implementation in #129 which mimicks what we discussed in this issue. You can take a look at the code right here on the PR branch.

One big difference is that I dropped the concept of indiviudal ports. Everything goes into a single Pins struct now which IMO is much easier to deal with for downstream code.

I think it would be very good to get support for the new AVR IO-cell very early on in this big refactor so we can prove that the new design is capable of supporting it.

@chris-ricketts: Would you be interested in helping out with that?

See commit 49fc191a5501 ("generic: port: Add support for new-style IO cells")... Sadly not yet usable because the overall support for the ATmega4809 turns out to be a bit more difficult.