search

Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.33k stars 225 forks source link

Hot take: the way avr-hal uses features is frustrating #602

Open innermatrix opened 1 week ago

innermatrix commented 1 week ago

As I am sure you know, the recommended way to use features is to make them additive, and to avoid mutually exclusive features. However, avr-hal relies heavily on non-additive mutually exclusive features. I find this to be a pain.

This setup works great as long as you are building a crate that generates one executable for one board. It doesn't work great if you are

The core issue — as I see it — is not that avr-hal is using features, it's the way it's using them. Specifically, avr-hal has some symbols whose implementation is selected mutually-exclusively using a feature — for example, Pins in atmega-hal. The fact that atmega-hal supports multiple MCUs, but can only ever export one Pins symbol, is at the root of the conflict.

IMO a better way to manage this would be to use features as they are intended: a feature flag selects whether a piece of functionality is included in the library, but allows multiple features to be selected. For example, I should be able to select both atmega32u4 and atmega328 features — and if I do, instead of them fighting over who gets to control the global Pins symbol, I should get both atmega32u4::Pins and atmega328::Pins symbols.

Taking this all the way up to the board level, what this would mean is that I would be able to use arduino-hal.features = ["atmega32u4", "atmega328"] when importing arduino-hal— which would indicate that I want arduino-hal to include support for both of those MCUs (using features additively, as they were intended) — and then in my main.rs I would write

#[cfg(any(feature = "atmega328"))]
use arduino_hal::atmega328 as board;

#[cfg(any(feature = "atmega32u4"))]
use arduino_hal::atmega32u4 as board;

I think this would completely eliminate all the shenanigans with mutually-exclusive features, and simplify the experience for anyone trying to use avr-hal in a non-trivial way.

Thoughts?

stappersg commented 1 week ago

On Sun, Nov 17, 2024 at 02:02:09PM -0800, Iris Artin wrote:

Thoughts?

Patches welcome

innermatrix commented 5 days ago

@stappersg I have started this work in https://github.com/Rahix/avr-hal/pull/603

That PR only changes the feature-specific pins! macros in attiny-hal and atmega-hal. I have follow-on PRs that I already started working on for refactoring other aspects — you can see the current PR stack at https://github.com/innermatrix/avr-hal/pull/1#issuecomment-2489875246.

My current overall plan is:

After I am done with that we can figure out what the plan is for if/when to fully remove the deprecated-globals functionality; that would be a breaking change, and I would leave decisions about that up to the maintainers. Up until that point I plan to maintain backwards compatibility.

I am structuring this as a PR stack, of which the PR above is the first. The full PR stack is linked in https://github.com/Rahix/avr-hal/pull/603 if you want to peek ahead to where I am going with this and offer early feedback, but you can also ignore it for now if you prefer 🙂 If there is some other structure you'd prefer for this work, let me know.

stappersg commented 5 days ago

On Wed, Nov 20, 2024 at 05:53:21PM -0800, Iris Artin wrote:

@stappersg I have started this work in https://github.com/Rahix/avr-hal/pull/603

Yep. Due "Notify all" I saw it already.

Please don't wait for me and above all: Explore what you want to explore.

Groeten Geert Stappers -- Silence is hard to parse

innermatrix commented 1 day ago

Alrighty, I have a coherent plan and will soon be sending along some PRs that put this into place.