Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for the architectural refactor #130

Open
27 of 42 tasks
Rahix opened this issue Feb 7, 2021 · 28 comments
Open
27 of 42 tasks

Tracking issue for the architectural refactor #130

Rahix opened this issue Feb 7, 2021 · 28 comments
Labels
hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal help wanted Extra attention is needed mcu-support Support for a new Microcontroller
Milestone

Comments

@Rahix
Copy link
Owner

Rahix commented Feb 7, 2021

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

@Rahix Rahix added help wanted Extra attention is needed hal-impl mcu-support Support for a new Microcontroller hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal labels Feb 7, 2021
@Rahix Rahix added this to the New APIs milestone Feb 7, 2021
@Rahix Rahix pinned this issue Feb 7, 2021
@explicite
Copy link
Contributor

explicite commented Feb 8, 2021

@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
Copy link
Owner Author

Rahix commented Feb 8, 2021

@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
Copy link

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

This comment has been minimized.

@Rahix

This comment has been minimized.

@stappersg

This comment has been minimized.

@Rahix
Copy link
Owner Author

Rahix commented Feb 15, 2021

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
Copy link

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

pub trait PinOps {
type Dynamic;
fn into_dynamic(self) -> Self::Dynamic;
unsafe fn out_set(&mut self);
unsafe fn out_clear(&mut self);
unsafe fn out_toggle(&mut self);
unsafe fn out_get(&self) -> bool;
unsafe fn in_get(&self) -> bool;
unsafe fn make_output(&mut self);
unsafe fn make_input(&mut self, pull_up: bool);
}

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).

pub struct $Pin {
_private: ()
}


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
Copy link
Owner Author

Rahix commented Feb 21, 2021

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:

  • The port API is built to guarantee that you cannot use the same digial IO pin in two places at the same time. Each pin has a singleton variable binding that cannot be duplicated (e.g. type Pin<PB6, Output>). The PinOps trait cannot enforce this though, and that means calling those methods might be unsafe in some situations. By marking them as unsafe we ensure that a caller cannot do the wrong thing from safe Rust in any situation.
  • The second problem is that the PinOps trait does not concern itself with which operations are legal in which state of the pin. This is modelled by the "higher-level" API provided by the Pin struct. For example, you must not call PinOps::out_set() after configuring a pin as input with PinOps::make_input(). With the Pin API, this is not possible by design but the "raw" interface provided by PinOps cannot make such guarantees (and is not intended to).

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:

/// Internal trait for low-level USART peripherals.
///
/// This trait defines the common interface for all USART peripheral variants. It is used as an
/// intermediate abstraction ontop of which the [`Usart`] API is built. **Prefer using the
/// [`Usart`] API instead of this trait.**
pub trait UsartOps<RX, TX> {
/// Enable & initialize this USART peripheral to the given baudrate.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_init<CLOCK>(&mut self, baudrate: Baudrate<CLOCK>);
/// Disable this USART peripheral such that the pins can be used for other purposes again.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_deinit(&mut self);
/// Flush all remaining data in the TX buffer.
///
/// This operation must be non-blocking and return [`nb::Error::WouldBlock`] if not all data
/// was flushed yet.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_flush(&mut self) -> nb::Result<(), void::Void>;
/// Write a byte to the TX buffer.
///
/// This operation must be non-blocking and return [`nb::Error::WouldBlock`] until the byte is
/// enqueued. The operation should not wait for the byte to have actually been sent.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_write(&mut self, byte: u8) -> nb::Result<(), void::Void>;
/// Read a byte from the RX buffer.
///
/// This operation must be non-blocking and return [`nb::Error::WouldBlock`] if no incoming
/// byte is available.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_read(&mut self) -> nb::Result<u8, void::Void>;
/// Enable/Disable a certain interrupt.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_interrupt(&mut self, event: Event, state: bool);
}

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
Copy link

koutoftimer commented Feb 22, 2021

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.

pub fn into_floating_input(mut self) -> Pin<mode::Input<mode::Floating>, PIN> {
unsafe { self.pin.make_input(false) };
Pin {
pin: self.pin,
_mode: PhantomData,
}
}

unsafe fn make_input(&mut self, pull_up: bool) {
(*<$PinPort>::ptr()).$pin_ddr_reg.modify(|r, w| {
w.bits(r.bits() & !(1 << $pin_num))
});
if pull_up {
self.out_clear()
} else {
self.out_set()
}
}

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

@Rahix
Copy link
Owner Author

Rahix commented Feb 22, 2021

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
Copy link
Contributor

@Rahix I think ATmega1280 can be checked.

@Rahix
Copy link
Owner Author

Rahix commented Mar 2, 2021

@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 Rahix mentioned this issue Mar 3, 2021
@Rahix
Copy link
Owner Author

Rahix commented Mar 3, 2021

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
Copy link
Contributor

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

@Rahix
Copy link
Owner Author

Rahix commented Mar 3, 2021

@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
Copy link
Contributor

drmorr0 commented Mar 9, 2021

@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
Copy link
Owner Author

Rahix commented Mar 9, 2021

@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
Copy link
Owner Author

Rahix commented Mar 16, 2021

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:

  • Support for the missing boards (Arduino Diecimila, Arduino Nano, etc.). Just look at previous commits which added support for other boards; you'll essentially need to do the same.
  • Support for ADC in attiny-hal, similar to e638e68.
  • Support for tri-state pins, like it existed before.

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

@drmorr0
Copy link
Contributor

drmorr0 commented Mar 17, 2021

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

@Rahix
Copy link
Owner Author

Rahix commented Mar 17, 2021

@drmorr0, sounds good!

@drmorr0
Copy link
Contributor

drmorr0 commented Apr 8, 2021

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
Copy link
Contributor

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

@stappersg
Copy link
Contributor

stappersg commented Apr 8, 2021 via email

@Rahix
Copy link
Owner Author

Rahix commented Apr 8, 2021

@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:

  • PWM needs some more discussion, see above for what we've decided on so far. I think there are still some details left that we need to look at.
  • Tri-State: Looking back, I think the old design was not really a tri-stated pin because it didn't expose all methods necessary for full tri-state operation. It was just a subset. To get the full support, we'll have to redesign them a bit, I'll hopefully write something up regarding that soon.

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
Copy link
Contributor

drmorr0 commented Apr 9, 2021

Sure, I'll take a stab at SPI.

@Rahix
Copy link
Owner Author

Rahix commented Jul 9, 2021

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

Rahix added a commit that referenced this issue Jul 11, 2021
Finally!  This merge marks the point where the "old" version of avr-hal
is superseded by the work that's been happening on the next branch.
This merge commit drops all of the old avr-hal and replaces history with
the tree from the `next` branch entirely.

Thanks to everyone who contributed to this effort!
Ref: #130, #190
@Rahix Rahix removed the hal-impl label Jul 17, 2021
@Rahix Rahix unpinned this issue Jan 10, 2023
@ZettaScript
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal help wanted Extra attention is needed mcu-support Support for a new Microcontroller
Projects
None yet
Development

No branches or pull requests

6 participants