search

Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.3k stars 220 forks source link

Tracking issue for the architectural refactor #130

Open Rahix opened 3 years ago

Rahix commented 3 years ago

Starting with PR #129, we will refactor the crate structure of avr-hal to better fit the different requirements. This issue will serve to track the progress of the refactor up to the point where it can be merged into the avr-hal main branch. Progress will be happening on the next branch.

Goals

Tasks

explicite commented 3 years ago

@Rahix, Iwill review #129 and them, I'm happy to start with ref. for ATmega2560. Not sure if progressing on Nano Every stuff is good.

Rahix commented 3 years ago

@explicite: I would very much like to see Nano Every be one of the first supported boards because we can use this to demonstrate whether the new design is flexible enough for this. I've gotten myself a Nano Every now as well and I would start integrating it (at the most basic level) as soon as I get some more time in the next days.

If you'll take care of ATmega2560 / Arduino Mega 2560, that would be awesome! I would suggest waiting for #129 to be merged before starting, though, in case anything changes still.

koutoftimer commented 3 years ago

PwmPin is subset of Pwm. All timers from avr-hal have to implement Pwm if avr-hal intends to implement embeded_hal. Because of PwmPin is subset of Pwm, it should delegate it's calls to Pwm.

stappersg commented 3 years ago

afbeelding

The check on ATmega32U4 does that include support for USB? ( Link to #40 )

Rahix commented 3 years ago

No, USB is not dealt with at all here, that'll be a topic for another time...

stappersg commented 3 years ago

that'll be a topic for another time...

also known as patches welcome ;-)

Rahix commented 3 years ago

PwmPin is subset of Pwm. All timers from avr-hal have to implement Pwm if avr-hal intends to implement embeded_hal. Because of PwmPin is subset of Pwm, it should delegate it's calls to Pwm.

@koutoftimer, I've come to more and more view embedded-hal compatibility as a secondary goal. Primarily, avr-hal should provide a nice API for its direct users, that is people writing code for AVR microcontrollers specifically. As a secondary effort, we of course want to implement the embedded-hal traits for our HAL drivers so that users can also benefit from the existing peripheral drivers in the embedded-hal ecosystem.

As an example: With the old port implementations, users were calling the trait methods for InputPin/OutputPin for digital IO. This was super ugly because of the mandatory Result return type:

    some_pin.set_high().unwrap();

Instead, with the new API, we have a set_high() method on the Pin type directly and just implement the OutputPin trait in parallel:

struct Pin { ... }

impl Pin {
    pub fn set_high(&mut self) { ... }
}

impl OutputPin for Pin {
    fn set_high(&mut self) -> Result<(), ()> { ... }
}

With this, users who use avr-hal directly can just write

    pin.set_high();

While the pin still implements the embedded-hal traits so it can be passed to a generic driver crate.

For PWM/timers I also think we shouldn't let our design be constrained by the embedded-hal traits. Sure, it would be good to implement them where possible, but the primary incentive should be building an API that abstracts all common AVR-specific usecases well.

That's my thoughts on this at least...

Now, in more practical terms, my idea for a the new design would be to follow a similar pattern to the new port/usart: We define a "low-level" internal trait that maps the low-level timer features onto a common API:

trait TimerOps {
    type Tcnt;

    unsafe fn get_tcnt(&self) -> Self::Tcnt;
    unsafe fn set_tcnt(&mut self, tcnt: Self::Tcnt);

    // And so on...
}

We then provide macros to implement this trait for different kinds of timers. If possible, a single macro should work for e.g. the 8-bit and 16-bit timers, but I would also be okay with separate macros where it makes the code more readable.

Ontop of that we have different generic APIs for different timer usecases. E.g. the fast-PWM we have right now, a precise PWM, a delay, a clock, a systick, etc. As a rough outline:

pub struct FastPWM<T> {
    timer: T,
}

impl<T: TimerOps> FastPWM<T> {
    pub fn new(timer: T, prescaler: TimerPrescaler) -> Self { ... }
}

Check out the USART implementation on the next branch for a full example of how this might look.

koutoftimer commented 3 years ago

@Rahix while changes in the "next" is not complete I'd like to argue about needs of unsafe for PinOps

https://github.com/Rahix/avr-hal/blob/8da215ad143211eb418a42d6699564e1dad34d63/avr-hal-generic/src/port.rs#L36-L50

PinOps implemented for Dynamic and $Pin.

Dynamic has private constructor and mask field that is used for PinOps implementation, so no bypass for public API allowed in terms of safe rust code.

$Pin takes it's arguments that is used inside unsafe trait implementation and passes it to Dynamic.

At this point I'm wondering why it is impossible to say that PinOps is safe? Unsafe in general means cases where compiler is dumb to verify correctness. Inside unsafe block only two parts are really needs to be unsafe:

  1. dereferencing raw pointer
  2. unsafe .bits() method of register's writer

Can we ensure that dereferencing raw pointer at this point is ok? - If avr-device generated code that is consistent with specs from Atmel, then yes.

Can we ensure that using .bits() method is safe? The only reason why it is marked unsafe is because, in general, there are registers, where not all bits are writeable and you may mess up with them easily. But, the only way from where .bits() receives its data is formed based on avr-device data, that is auto generated from Atmel's spec... which stands for "Yes, we can".

I, personally, do not like an idea to mark method unsafe just because of unsafe operations inside. If you can ensure that they are safe, then they are. Public API, PinOps provides, does not allow you to mess up with unsafe internal parts even from inside of the crate (using safe rust code).

You can say that by marking PinOps methods with unsafe you are telling that there are something going on and, if one wants to find out what, he should take a look. But, on the other hand, more unsafe code arrives and it is harder to specify which part is really unsafe.

Rust does not allow to create empty generic struct, but do allows to create regular empty structs, that is why no _private field needed (playground).

https://github.com/Rahix/avr-hal/blob/8da215ad143211eb418a42d6699564e1dad34d63/avr-hal-generic/src/port.rs#L349-L351

About docs: for me, it is easy to thinks about avr-hal as 4 independent layers, where each new one depends on previous and adds tiny bit of new features:

  1. avr-device - "binding" for MCU, you can start writing with just this;
  2. avr-hal-generic - set of common utils used for later layers;
  3. mcu - adds useful abstractions for specific MCU, writing code is more reliable and pleasant;
  4. boards - adds sugar for working with concrete brand's board based on specific MCU.

I wish I knew this beforehand. Maybe this info requires separate section on the main doc page. I'm not sure where to put it though.. avr-device is completely separate crate with it's own docs. I suppose, MCUs and boards will have its own too. Maybe some common section like "here is set of our products" in each of them will be helpful.

Rahix commented 3 years ago

Hi @koutoftimer,

At this point I'm wondering why it is impossible to say that PinOps is safe? Unsafe in general means cases where compiler is dumb to verify correctness.

The safety here is less related to pure memory safety and more to the API contract of the port API. That means, unsafe here is used to mark functions which can be called in a context that would "breach the contract" we're trying to enforce. The main two points are:

Due to these things, we need to make sure the "raw" PinOps API is unreachable from safe downstream Rust code. I do see that using unsafe for this is not ideal as Rust primarily uses unsafe for concepts of memory safety. Ideally, we would have a different kind of "unsafe" for this purpose but of course that does not exist (yet?)...

As a different example, in the USART refactor, I specifically didn't go with unsafe due to the points you described above: It hides where the unsafe is used in the function implementation. See here:

https://github.com/Rahix/avr-hal/blob/8da215ad143211eb418a42d6699564e1dad34d63/avr-hal-generic/src/usart.rs#L158-L199

I named the methods raw_*() to hopefully deter people from calling them directly, but this is not ideal either... Still looking for a cleaner solution to "hide" them... If we find anything, I'd gladly apply the same to PinOps. Some more discussion about this happened back in #116, if you're interested.

Can we ensure that dereferencing raw pointer at this point is ok? - If avr-device generated code that is consistent with specs from Atmel, then yes.

No, because this could violate "thread-safety" - we could create multiple accessors to the same register in different concurrent contexts (e.g. thread-mode and an interrupt context). The only safe way to access the registers is via the singletons that are returned by Peripherals::take(). This approach ensures that there is only ever one accesor for each register and that downstream code must ensure synchronous access.

The reason why it is safe here is because we know the compiler will compile all those accesses down into a single sbi or cbi instruction and thus only touch the one bit relevant to the respective pin. And because each pin is a singleton, we can guarantee this prevents any kind of race-conditions surrounding those registers.

Can we ensure that using .bits() method is safe?

This plays into a bigger debate on whether raw register access should be considered safe, in general. There are certainly registers one could use to violate memory safety (e.g. think of DMA) but on the other hand making all register accesses unsafe also just leads to HAL code being littered with unsafe blocks... I do not think the embedded Rust community has found a clear answer to this question yet. :/

I, personally, do not like an idea to mark method unsafe just because of unsafe operations inside. If you can ensure that they are safe, then they are.

Totally with you on that. Though as I described above, these methods are not really safe - not in the sense of memory safety, but in terms of the API contract.

Rust does not allow to create empty generic struct, but do allows to create regular empty structs, that is why no _private field needed.

The _private field is used to ensure nobody can instanciate a variable of this type from outside this module. Otherwise you could create multiple accessors to the same pin from downstream code.

About docs: for me, it is easy to thinks about avr-hal as 4 independent layers, where each new one depends on previous and adds tiny bit of new features:

Very true what you are saying here. I would suggest we overhaul the documentation regarding this once most of the implementation work for the refactor is done... Ideally with some diagrams to visualize the relationships a bit better as well.

koutoftimer commented 3 years ago

13.2.3 Switching Between Input and Output

When switching between tri-state ({DDxn, PORTxn} = 0b00) and output high ({DDxn, PORTxn} = 0b11), an intermediate state with either pull-up enabled {DDxn, PORTxn} = 0b01) or output low ({DDxn, PORTxn} = 0b10) must occur. Normally, the pull-up enabled state is fully acceptable, as a high-impedance environment will not notice the difference between a strong high driver and a pull-up. If this is not the case, the PUD bit in the MCUCR register can be set to disable all pull-ups in all ports.

Switching between input with pull-up and output low generates the same problem. The user must use either the tri-state ({DDxn, PORTxn} = 0b00) or the output high state ({DDxn, PORTxn} = 0b11) as an intermediate step.

I guess, ({DDxn, PORTxn} = 0b00) represented as Input<Floating>, ({DDxn, PORTxn} = 0b11) is Output. User is forced to switch pin's mode into output with into_output method, which sets DDxn with 1 and we have ({DDxn, PORTxn} = 0b10).

{DDxn, PORTxn} = 0b01) represented as Input<PullUp>, ({DDxn, PORTxn} = 0b10) is Output. Changing DDxn brings us into ({DDxn, PORTxn} = 0b11).

I do not know was it intentional or not, but my regards anyway.

Implementation almost completely follows "Peripherals as State Machines" except that output mode isn't splitted into low/high. It feels a bit scary when you know that corner cases like above can arrive.

In order to add meaning to the text above: link to an article in the docs will help to understand what is going on under the hood.

Also I'm a bit concerned, will compiler optimize calls to unsafe fn make_input? Seeing branching is such code is not what one would expect.

https://github.com/Rahix/avr-hal/blob/8da215ad143211eb418a42d6699564e1dad34d63/avr-hal-generic/src/port.rs#L116-L122 https://github.com/Rahix/avr-hal/blob/8da215ad143211eb418a42d6699564e1dad34d63/avr-hal-generic/src/port.rs#L400-L409

UPD: ok, #[inline] does make sure no branching occurs.

Rahix commented 3 years ago

I do not know was it intentional or not, but my regards anyway.

Yeah this is a bit of a weirdness, I agree. Not sure what the best way to deal with it would be, or if we should just leave it as is. If anyone has compelling arguments for either side, please let them be known!

Also I'm a bit concerned, will compiler optimize calls to unsafe fn make_input?

Because it is branching on a constant, the dead branch should be eliminated. It wouldn't be a soundness issue if it did not, though.

explicite commented 3 years ago

@Rahix I think ATmega1280 can be checked.

Rahix commented 3 years ago

@explicite, I'm currently working on some bigger changes in the HAL-crate. If you want to continue helping out, I'd suppose it is better to wait until I've got things sorted out there. Otherwise it is going to get a bit messy, I think...

The change in question is related to the custom Peripherals structs. I do not think they will work in the long run so I'm trying to get rid of them. I still have some issues related to the orphan rule but I hope to have them sorted out soon so I can post a PR.

Rahix commented 3 years ago

Okay, changes are done, see #152 :) The USART HAL continues to serve as the design guide for what we should do with the others as well.

explicite commented 3 years ago

@Rahix do you have any idea what I can progress after ATiny mcu's?

Rahix commented 3 years ago

@explicite, I am currently wrapping up with the I2C rework. If you want, you can give the SPI HAL a shot :) Once the I2C HAL is merged, you can pretty much copy what I am doing there, the implementations will look very similar.

drmorr0 commented 3 years ago

@Rahix do you want to edit the description to mention the atmega8u2? (see #160).

I also just wanted to comment on:

For PWM/timers I also think we shouldn't let our design be constrained by the embedded-hal traits. Sure, it would be good to implement them where possible, but the primary incentive should be building an API that abstracts all common AVR-specific usecases well.

I am expecting that at some point in the future I am going to want to start doing some work with a Cortex-M ARM processor. I've been hoping that, by using a HAL that used the common embedded-hal traits, that it would be "relatively" straightforward to port my code over if and when I get to the point of using a different processor. Now that may be hopelessly naïve of me, but I'd just like to cast my vote for trying to stick to the embedded-hal traits.

Rahix commented 3 years ago

@drmorr0, code that's generic against embedded-hal will of course work with avr-hal just as well. This was more about looking beyond the provided traits, because they do not cover a lot of important usecases. It's probably the same with any HAL, you get a base-feature-set that's generic enough to work across all architectures, but for anything advanced, you need chip-specific abstractions.

Ideally, we will never not implement an embedded-hal trait. We'll just provide a "nicer" API alongside it for direct users.

Rahix commented 3 years ago

For anyone who's interested in contributing, there are still lots of tasks to be done! Some of the easier ones, to get into the project are, for example:

If here are any questions, just ping this issue or open a draft PR to discuss it.

drmorr0 commented 3 years ago

I'd pick up one of these; I could do the Arduino Nano.

Rahix commented 3 years ago

@drmorr0, sounds good!

drmorr0 commented 3 years ago

Any other tasks you want me to pick up? I'm particularly interested in SPI support (cc @explicite), not sure if there's anything there I can pick up? Or I can look at the tri-state pins or PWM.

explicite commented 3 years ago

@drmorr0, @Rahix Right now I will be not able to progress on SPI support. Please start if you can.

stappersg commented 3 years ago

Warning: Compliment ahead

On Wed, Apr 07, 2021 at 11:18:41PM -0700, Jan Paw wrote:

@drmorr0, @Rahix Right now I will be not able to progress on SPI support. Please start if you can.

Thanks for releasing the "lock". It is good to express "Do not wait for me on this".

Rahix commented 3 years ago

@explicite, that's alright, thanks for the huge help you've been so far anyway! @drmorr0, if you want to pick up the SPI task then, this would be very useful.

Regarding the other two topics:

For the big picture: I would still like to see the ATmega4809 demonstrated before going live with the new version. However the foundational work for getting there is going to take me some time which I'm not sure I can spend on this project right now. So to not block progress too much (and to stop people sending MRs against the old version all the time ^^) I think as soon as we reach a rough feature-parity with the old codebase we can switch over. Though I'd still hold off on releasing crates until the new design is "proven".

To put this into more concrete terms: I think with SPI and a basic Fast PWM implemenation for the new version we are at an acceptable level to merge next into master.

drmorr0 commented 3 years ago

Sure, I'll take a stab at SPI.

Rahix commented 3 years ago

Heads up for #190 (see PR description for details).

ZettaScript commented 10 months ago

What is this issue's status?

I started adding support for attiny402 but got stuck because avr-hal-generic is not compatible (notably port, adc, eeprom). (my wip branch: https://github.com/ZettaScript/avr-hal/tree/attiny402)