-
Notifications
You must be signed in to change notification settings - Fork 226
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
Comments
@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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@koutoftimer, I've come to more and more view As an example: With the old port implementations, users were calling the trait methods for some_pin.set_high().unwrap(); Instead, with the new API, we have a 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 pin.set_high(); While the For PWM/timers I also think we shouldn't let our design be constrained by the 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 |
@Rahix while changes in the "next" is not complete I'd like to argue about needs of avr-hal/avr-hal-generic/src/port.rs Lines 36 to 50 in 8da215a
At this point I'm wondering why it is impossible to say that
Can we ensure that dereferencing raw pointer at this point is ok? - If Can we ensure that using 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, You can say that by marking Rust does not allow to create empty generic struct, but do allows to create regular empty structs, that is why no avr-hal/avr-hal-generic/src/port.rs Lines 349 to 351 in 8da215a
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:
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.. |
Hi @koutoftimer,
The safety here is less related to pure memory safety and more to the API contract of the port API. That means,
Due to these things, we need to make sure the "raw" As a different example, in the avr-hal/avr-hal-generic/src/usart.rs Lines 158 to 199 in 8da215a
I named the methods
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 The reason why it is safe here is because we know the compiler will compile all those accesses down into a single
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
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.
The
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. |
I guess, ({DDxn, PORTxn} = 0b00) represented as {DDxn, PORTxn} = 0b01) represented as 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 avr-hal/avr-hal-generic/src/port.rs Lines 116 to 122 in 8da215a
avr-hal/avr-hal-generic/src/port.rs Lines 400 to 409 in 8da215a
|
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!
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. |
@Rahix I think |
@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 |
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. |
@Rahix do you have any idea what I can progress after |
@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. |
@Rahix do you want to edit the description to mention the I also just wanted to comment on:
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. |
@drmorr0, code that's generic against Ideally, we will never not implement an |
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. |
I'd pick up one of these; I could do the Arduino Nano. |
@drmorr0, sounds good! |
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, 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 |
Sure, I'll take a stab at SPI. |
Heads up for #190 (see PR description for details). |
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
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) |
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 theavr-hal
main branch. Progress will be happening on thenext
branch.Goals
arduino-hal
which allows to write code that can be effortlessly used on any supported board (as long as all necessary hardware features are available). See Unified arduino-hal #117.Tasks
attiny-hal
(Pull Request: Add ATtiny167 support and ADC support for all ATtiny chips #204)ATmega1280
(Pull Request: atmega-hal: atmega1280 #143)ATmega168
(Pull Request: atmega-hal: atmega168 #145)ATmega2560
(Pull Request: atmega-hal: mcu support for atmega2560 #138)ATmega328PB
(Pull Request: atmega-hal: atmega328pb #149)ATmega328P
(Pull Request: Start work on the architectural refactor #129)ATmega32U4
ATmega48P
(Pull Request: atmega-hal: Add support for ATmega48p #150)ATtiny85
(Pull Request: atmega-hal:attiny85 support #157)ATtiny88
(Pull Request: atmega-hal ATtiny88 support #158)ATmega8U2
(Pull Request: atmega-hal: Add support for ATmega8U2 #170)ATtiny167
(Pull Request: Add ATtiny167 support and ADC support for all ATtiny chips #204)ATmega4809
as the first newer generation device.Bigavr6(Let's drop this for now)The text was updated successfully, but these errors were encountered: