avr-hal icon indicating copy to clipboard operation
avr-hal copied to clipboard

Add port implementations for AVR XMEGA / ATtiny404

Open agausmann opened this issue 3 years ago • 8 comments

This is my first time working with an XMEGA microcontroller. The XMEGA register maps are quite different from the ATmega / ATtiny ones I've seen, so I figured it would be best to start a separate crate for it.

This hasn't been tested on hardware yet; I'm expecting to receive the parts and prototype boards in mid/late November.

I also copied the delay re-export from attiny-hal, though I haven't checked if there are any differences in instruction timing that need to be accounted for.

agausmann avatar Oct 26 '22 02:10 agausmann

Example crate using this API

The generated machine code looks okay, all the I/O memory and SRAM accesses look correct.

(Note, it may be difficult to find a GCC toolchain that has support/specs for attiny404. I got this crate to compile successfully using the toolchain packaged with Arduino IDE 1.8.19)

agausmann avatar Oct 26 '22 03:10 agausmann

I think I noticed a logic error in the open-drain implementation.

Pin::into_opendrain_high() doesn't call PinMode::out_clear(): https://github.com/Rahix/avr-hal/blob/8e4ba8f2a097b28459e35528e9f3d89dcf2a6c39/avr-hal-generic/src/port.rs#L152-L171

... and also, Pin<OpenDrain>::set_low() only calls PinMode::make_output(), and also doesn't call PinMode::out_clear(): https://github.com/Rahix/avr-hal/blob/8e4ba8f2a097b28459e35528e9f3d89dcf2a6c39/avr-hal-generic/src/port.rs#L338-L348

Because of this, the state of the XMEGA OUT register is undefined, it could be left high if the mode is set with into_opendrain_high().

This wasn't a problem in other MCUs, because the pull-up state shares the same register as the output driver (PORTn), and the call to make_input(false) would initialize PORTn = 0 when it configures the pull-up.

An easy fix for this would be to explicitly call out_clear() in into_opendrain_high(). This would be a small regression in code size and performance for MCUs that don't require it, but I don't think the performance is a concern; setting pin modes is usually done once at the beginning of the program.

agausmann avatar Oct 26 '22 04:10 agausmann

Ooh, this looks interesting! I don't have time right now to look at it in more detail, but you may also want to check #139 to see how your work compares. I would assume yours to be more thorough, though...

Rahix avatar Oct 26 '22 10:10 Rahix

Oh, nice! They look pretty similar. The .pullupen().bit(bool) is a better way of doing that, I forgot about the bit method.

The other differences I noticed:

  • In mine, the pinXctrl register names are passed via the macro input for each pin. In yours, there is a match generated for every pin against its $pin_num to determine which pinXctrl to use.
  • In mine, pull-up state (pinXctrl.pullupen) is set before switching to input (dirclr). In yours, the pin direction is set first, then pull-up will be enabled or disabled after that, which would be the correct order for MCUs where pull-up and output drivers share the same register, but I think pull-up can be set first in this case.

agausmann avatar Oct 26 '22 14:10 agausmann

In mine, the pinXctrl register names are passed via the macro input for each pin. In yours, there is a match generated for every pin against its $pin_num to determine which pinXctrl to use.

Yeah. The trick here is that the match will be optimized out so only the single correct option will actually end up in the final program. This way I can avoid having to pass yet another identifier.

In mine, pull-up state (pinXctrl.pullupen) is set before switching to input (dirclr). In yours, the pin direction is set first, then pull-up will be enabled or disabled after that, which would be the correct order for MCUs where pull-up and output drivers share the same register, but I think pull-up can be set first in this case.

I think you're right. I didn't look into it too deeply, my main goal was making a PoC so I can see whether the general structure of the port module would work with the new-style I/O cells. So there is a good chance that I didn't look carefully at some of the things my PR does.

Rahix avatar Oct 27 '22 07:10 Rahix

I think I would err on the side of generating less code, even if the match is usually optimized out.

Having "yet another identifier" is not really that costly, especially compared to impl_port_traditional!

The ideal compromise between less code and less input/identifiers would be using something like concat_idents!(pin $pin_num ctrl), but unfortunately concat_idents! doesn't work with numeric literals.

Another option is syntax matching using macros - this one could be invoked by impl_port_xmega! using a statement like $crate::pinXctrl!($pin_num):

#[macro_export]
macro_rules! pinXctrl {
    (0) => { pin0ctrl };
    (1) => { pin1ctrl };
    (2) => { pin2ctrl };
    (3) => { pin3ctrl };
    (4) => { pin4ctrl };
    (5) => { pin5ctrl };
    (6) => { pin6ctrl };
    (7) => { pin7ctrl };
}

I don't know whether I would prefer this over simply passing the identifier as input to the impl_port macro.

agausmann avatar Nov 03 '22 16:11 agausmann

I just noticed the avr-device 0.5 release, I will rebase this PR tonight.

agausmann avatar Jan 05 '23 16:01 agausmann