f4xx-hal like gpio #316
f4xx-hal like gpio #316
Conversation
burrbull
force-pushed
the
f4gpio
branch
9 times, most recently
from
01c1dad to
893fb64
Compare
| @@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| No changes. | |||
| - Use const generics for pins. The MSRV was bumped to 1.59 ([#316]) | |||
There was a problem hiding this comment.
As this pull request changes many things in the gpio API I'd like to have a short list of things which changed like:
into_af_push_pull->into_alternatedowngrade().downgrade()->erase()
Kind of like a short upgrade guide. You don't have to be too detailed. I'm fine to do it myself as well. You can also wait with that change until I finished my thorough review :)
Also, this should be listed under ### Breaking Changes
There was a problem hiding this comment.
I can add functions with old names and mark them deprecated.
There was a problem hiding this comment.
Probably it is not really worth it to add deprecation notices as the resulting code would be really messy I guess.
src/gpio.rs
Outdated
| //! To make a pin dynamic, use the `into_dynamic` function, and then use the `make_<mode>` functions to | ||
| //! change the mode | ||
|
|
||
| #![allow(missing_docs)] |
There was a problem hiding this comment.
Please remove this when finished. If it does make sense to not comment, some parts either add this attributes to local functions or types or use #[doc(hidden)] in case of traits with the private method convention e.g. the functions or prefixed with fn _, which (I like by the way).
| pub use erased::{EPin, ErasedPin}; | ||
| mod dynamic; | ||
| pub use dynamic::{Dynamic, DynamicPin}; | ||
| mod hal_02; |
There was a problem hiding this comment.
Hm, hal_02 seem a little cryptic for a module name. Maybe use the conventional abbreviation of embeded_hal like eh
Or as it is not typed very often I'm fine with a long name like embedded_hal_v0_2 or similar.
src/gpio.rs
Outdated
|
|
||
| mod sealed { |
There was a problem hiding this comment.
This could probably still be named marker though I'm fine with that change.
| } | ||
|
|
||
| /// GPIO interrupt trigger edge selection |
There was a problem hiding this comment.
Why are these doc comments removed?
| } | ||
| } | ||
| #[inline(always)] | ||
| fn _set_high(&mut self) { |
There was a problem hiding this comment.
If this function is private, adding #[doc(hidden)] might make sense.
| } | ||
| } | ||
| #[cfg(feature = "gpio-f302")] | ||
| gpio!(GPIOA, gpioa, PA, 'A', PAn, [ |
There was a problem hiding this comment.
The macros here are different to the previous macros, which were auto generated by the codegen tools inside this repo.
Maybe you can adjust that tool to get your desired output. But I'm fine if you don't like to bother. codegen needs a cleanup / overhaul anyway.
But did you autogenerate this or did you adjust these invocations by hand, basing on the previous implementation?
src/gpio/erased.rs
Outdated
| @@ -0,0 +1,147 @@ | |||
| use super::*; | |||
|
|
|||
| pub type EPin<MODE> = ErasedPin<MODE>; | |||
There was a problem hiding this comment.
I don't think this type alias is needed. I think ErasedPin is short enough. At least this type alias should not be used in this code base to benefit readability.
src/gpio/partially_erased.rs
Outdated
| @@ -0,0 +1,129 @@ | |||
| use super::*; | |||
|
|
|||
| pub type PEPin<const P: char, MODE> = PartiallyErasedPin<P, MODE>; | |||
There was a problem hiding this comment.
As the other type alias, I'm not a fan of this one. Though here the case is stronger for an alias as the original type name is long.
But I don't think this is a problem. A user might just use a type alias themself via use hal::gpio::PartiallyErasedPin as PEPin.
| @@ -16,9 +16,3 @@ pub use crate::rcc::RccExt as _stm32f3xx_hal_rcc_RccExt; | |||
| pub use crate::syscfg::SysCfgExt as _stm32f3xx_hal_syscfg_SysCfgExt; | |||
| pub use crate::time::duration::Extensions as _stm32f3xx_hal_time_time_Extensions; | |||
| pub use crate::time::rate::Extensions as _stm32f3xx_hal_time_rate_Extensions; | |||
| pub use crate::{ | |||
There was a problem hiding this comment.
This is an important breaking change, which should be commented especially in the Changelog.
Might also be a good idea to put this in a single commit :)
|
Hey, sorry for the delay, I'm pretty busy right now. I hope I'll make progress on this review this week. |
|
cc @Sh3Rm4n |
No description provided.