Rework USART API to reduce macro usage

[Rahix]

It is showing more and more that generating the entire API for the HAL crates from macros is a bad idea. Reasons include:

• It obscures the code. People have trouble finding where a certain type is defined, especially if they are not too well aquainted with rust macro_rules!().
• It breaks auto-completion in IDEs because they'd need to introspect the macros.
• It leaves many types with the same methods instead of a single one. This also makes code that is generic over the exact peripheral impossible to write.
• It makes it much harder to cleanly support different peripherals under a common interface.

To mitigate these problems, shift to a different design: A single Usart type provides the public API and is generic over the USART peripherals which all implement a common interface called UsartOps. Only this trait implementation will be macro generated and thus there is no user-facing macro-generated API left.

This design also makes it much easier to implement support for multiple different hardware peripherals: As long as the UsartOps are expressive enough to fit all of them, we can simply provide multiple macros for generating the implementation for each one.

As the diff in this PR is quite unreadable, please look at the new API here: https://github.com/Rahix/avr-hal/blob/refactor-usart/avr-hal-generic/src/usart.rs

Remaining work:

• Add back reader and writer halves (.split() API).
• Add back interrupts API.
• Add API documentation.
• Investigate how we can "hide" UsartOps from downstream code or make it unsafe to access.
• Verify expressiveness by implementing UsartOps for ATmega4809 (Arduino Nano Every support #97, ATmega4809 support #101).

[Rahix]

Investigate how we can "hide" UsartOps from downstream code or make it unsafe to access.

This part is still a big question mark for me. Right now, user-code could trivially do stupid things like

use avr_hal_generic::usart::{self, UsartOps};
let mut usart = dp.USART0;

usart.deinit();
usart.interrupt(usart::Event::RxComplete, true);
usart.flush();

This obviously invalidates all safety mechanisms and needs to be prevented. My original idea was to just make the functions in UsartOps unsafe to call but this has the nasty side-effect that it means all the implementations of the functions are large unchecked unsafe blocks. As the code they contain is usually mostly safe rust, this would be very unfortunate.

Ideally, we can find a way to make those functions "private" in some sense, but I am not quite sure how. The trait must be public because downstream code might want to be generic over it:

fn takes_any_usart<USART: UsartOps<RX, TX>, RX, TX, CLOCK>(any_usart: Usart<USART, RX, TX, CLOCK>) {
    // ...
}

[chris-ricketts]

@Rahix Could a possible solution be to create a type within the macro that implements UsartOps. Something like I've hashed together here: https://github.com/chris-ricketts/avr-hal/blob/adff98e9d38d2224d3a0e4e6915bc0be4c397734/avr-hal-generic/src/usart.rs#L423

This would expand to:

    // Excuse the underscore - I didn't realise the macro created a `Uart1<..>` type alias
    pub struct _Usart1 {
        peripheral: crate::pac::USART1,
    }
    impl core::ops::Deref for _Usart1 {
        type Target = crate::pac::USART1;
        fn deref(&self) -> &Self::Target {
            &self.peripheral
        }
    }
    impl core::ops::DerefMut for _Usart1 {
        fn deref_mut(&mut self) -> &mut Self::Target {
            &mut self.peripheral
        }
    }
    pub fn usart1<CLOCK>(
        peripheral: crate::pac::USART1,
        rx: portd::PD2<
            ::avr_hal_generic::port::mode::Input<::avr_hal_generic::port::mode::Floating>,
        >,
        tx: portd::PD3<::avr_hal_generic::port::mode::Output>,
        baudrate: ::avr_hal_generic::usart::Baudrate<CLOCK>,
    ) -> ::avr_hal_generic::usart::Usart<
        _Usart1,
        portd::PD2<::avr_hal_generic::port::mode::Input<::avr_hal_generic::port::mode::Floating>>,
        portd::PD3<::avr_hal_generic::port::mode::Output>,
        ::avr_hal_generic::usart::Baudrate<CLOCK>,
    > {
        let usart = _Usart1 { peripheral };
        Usart::new(usart, rx, tx, baudrate)
    }

This would meant that a downstream user doesn't have ready access to a type that implements UsartOps - they'd have to explicitly create _Uart1.

It would mean creating the uart like:

let serial = hal::usart::usart1(
       dp.USART1,
       pins.d0,
       pins.d1.into_output(&mut pins.ddr),
       57600.into_baudrate(),
   );
   // instead of
   let serial = hal::usart::Usart1::new(
       dp.USART1,
       pins.d0,
       pins.d1.into_output(&mut pins.ddr),
       57600.into_baudrate(),
   );

EDIT:
This doesn't really help with:

The trait must be public because downstream code might want to be generic over it:

fn takes_any_usart<USART: UsartOps<RX, TX>, RX, TX, CLOCK>(any_usart: Usart<USART, RX, TX, CLOCK>) {
   // ...
}

But there could be a higher level Usart Trait that users could be generic over whilst also eliding the RX, TX, CLOCK type parameters, e.g.:

pub trait Serial { ... }
fn takes_any_usart(any_usart: impl Serial) {
 ...
}

[Rahix]

I think this idea does look promising, with a few further tweaks this might work quite well. I'll have to play around with it a bit more ...

[Rahix]

OTOH: We're not gaining much by hiding this: Downstream code can already do lots of stupid things safely because the API generated by svd2rust is already mostly usable from safe rust. I think anyone doing this kind of thing is well aware of what they're getting themselves into and thus the additional "safety" from hiding it is not worth too much ...

[explicite]

Maybe we can define FSM? https://docs.rs/rust-fsm/0.4.0/rust_fsm/
Not sure if this is good way in Rust... and embedded code.

[explicite]

@Rahix is it possible to help you to push forward this?

[Rahix]

Hey @explicite,

sorry for the inactivity here ... I hope I can allocate some time next weekend to get this resolved. I did some work in other HAL layers (for other MCUs) in the meantime and I think I now got some new ideas which could be applied here as well.

[Rahix]

Okay, I've decided to just use a simple solution for now: The methods are now named raw_... to reflect that they are low-level API. In the future we can think about a more elegant design...

I'll merge this state and we'll do any future changes ontop of it later.

[koutoftimer]

https://rust-embedded.github.io/book/static-guarantees/index.html pinned here for further reading