Use paste! to simplify port macros in avr-hal-generic
#451
Conversation
|
Hi, thanks a lot for this work. You have to be careful with simplifying the interface too much because in some places the chips themselves are not consistent enough to abstract them properly. I believe this might be the cause for the CI failures here. In general, my suggestion would be to do this work in one module per PR. So please open one PR for the |
Yes, I'm noticing the inconsistencies. For the port module they don't seem that bad, but the PWM one requires extra care, and probably the ones I haven't looked at yet as well. On that note, I should mention I couldn't fit Timer 1 on the Attiny85 into the module. It has a dual waveform and outputs to 2 pins simultaneously, so we probably can't reliably represent that with the current framework. I left it unimplemented with a TODO comment for later. It had an implementation prior, but it considered only one pin, which could lead to issues if the user expects to be able to use its dual normally.
Okay, that's a good idea. I'll edit this one to have only the port module as scope, and open others as I proceed to other modules. |
LuigiPiucco
changed the title
paste! more and simplify macros in avr-hal-genericpaste! to simplify port macros in avr-hal-generic
LuigiPiucco
force-pushed
the
paste-macros
branch
from
ed52304
to
20d356e
Compare
|
I'll mark this as ready for review, since we limited the scope. I rebased and fixed some issues in As a side note, and since you thanked me for this, I should say this crate is very high quality, it sure beats the regular Arduino stack by a wide margin. So thank you as well. |
| type Pin = $PinType:ident; | ||
|
|
||
| $(#[$pins_attr:meta])* | ||
| pub struct Pins from $McuPins:ty { | ||
| $($(#[$pin_attr:meta])* pub $pin:ident: $Pin:ty = $pin_orig:ident,)+ | ||
| pub struct Pins { | ||
| $($(#[$pin_attr:meta])* pub $pin_name:ident: $pin_type:ty = $pin_orig:ident,)+ | ||
| } | ||
|
|
||
| impl Pins { | ||
| type Pin = $pin_wrapper:ident; | ||
| type McuPins = $mcu_pins:ty; | ||
| } |
There was a problem hiding this comment.
Can you comment on the motivation behind this change? I don't have strong opinions either direction, but would like to hear why you prefer this macro syntax.
There was a problem hiding this comment.
This is indeed a more stylistic change. Now that I think about it, I could probably have left this bit untouched, just changing the transcriber. But in general, when writing macros I prefer to keep syntax as close to the base language as possible, and the Pins from $McuPins part both introduces a "new" keyword (from) and looks more like a syntax for inheritance (in the OOP sense). The type that the macro generates doesn't really inherit the other, in the sense of "class A is a B and therefore can do what B can and be where B would", it's more like they're alternative interfaces. I thought making it seem as an associated type would be more fitting, because 1) it still shows the relationship between the types; 2) it doesn't add any syntax (though inherent associated types is an unstable grammar, I believe, so maybe we could improve); 3) it looks more native to Rust, since the language doesn't have proper inheritance.
All that being said, it's mostly superfluous, so if you'd rather make the diff a bit smaller I can change it back.
There was a problem hiding this comment.
Fair, thanks a lot for the detailed elaboration :) Sounds good to me!
All that being said, it's mostly superfluous, so if you'd rather make the diff a bit smaller I can change it back.
I think for the future, it would be a good idea to at least put each of the changes into its own commit as they don't logically relate to one another. But let's not bother with such perfectionism here, I'll take your changes as they stand right now.
There was a problem hiding this comment.
I think for the future, it would be a good idea to at least put each of the changes into its own commit as they don't logically relate to one another.
I can apply this suggestion, just have a question of preference before: Do you value the ability to compile each commit separately, or having it working by the end of the pull request is enough? There are some instances where using a more micro approach will require introducing changes that would not need to be there otherwise, to maintain self-containment. I'm fine with either, just would rather ask to make sure.
LuigiPiucco
force-pushed
the
paste-macros
branch
from
20d356e
to
2a0c86b
Compare
Following #449, a draft of the reimplementation. Currently done is the port module and the simple_pwm module. I will be working on the others as well, and feel free to suggest changes/improvements. The intent is to deduplicate the implementations for each CPU, hoping to allow easier refactorings and additions of features.
Closes #449.