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

gpio: Add basic functionality for GPIOs #17

Merged
merged 2 commits into from
Oct 8, 2024
Merged

gpio: Add basic functionality for GPIOs #17

merged 2 commits into from
Oct 8, 2024

Conversation

astapleton
Copy link
Contributor

@astapleton astapleton commented Jul 31, 2024

This adds the basic outline functionality for the GPIO module and will form the base for follow-up PRs (see #19, #20, #21) to round out the GPIO functionality.

The gpio module is largely based on/copied from the implementation in stm32h7xx-hal.

Base automatically changed from as/rcc-logic to master August 12, 2024 17:55
@astapleton astapleton force-pushed the as/gpio-base branch 2 times, most recently from 9e8d35c to 199c800 Compare August 13, 2024 18:56
@astapleton astapleton requested a review from richardeoin August 13, 2024 19:03
RisingFalling,
}

pub type AF0<Otype = PushPull> = Alternate<0, Otype>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tidy-up of the macro that was not saving any lines, although the docstrings are lost.

The docstrings looked ok in the docs, but not a big issue
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the docstrings. I added them back. It's a little less neat, but I'm still not convinced that a macro is necessary.

@richardeoin
Copy link
Member

Great that this GPIO implementation is applicable to H5 also! The H7 implementation this is based on was mostly contributed by @burrbull in stm32-rs/stm32h7xx-hal#347 and stm32-rs/stm32h7xx-hal#346

I haven't had time to do an in-depth review (limited time at the moment) but what I have seen is good :)

@astapleton
Copy link
Contributor Author

Great that this GPIO implementation is applicable to H5 also! The H7 implementation this is based on was mostly contributed by @burrbull in stm32-rs/stm32h7xx-hal#347 and stm32-rs/stm32h7xx-hal#346

I haven't had time to do an in-depth review (limited time at the moment) but what I have seen is good :)

It seems that quite a few of the peripherals are very similar between the H7 and H5 (with some notable differences like a completely new DMA engine on the H5).

@burrbull, would you be interested in reviewing these GPIO PRs, if you have the time?

@burrbull
Copy link
Member

@astapleton Good for initial stuff.

You could look at https://github.com/stm32-rs/stm32f4xx-hal/tree/master/src/gpio if you want to develop it further.

@astapleton
Copy link
Contributor Author

@astapleton Good for initial stuff.

You could look at https://github.com/stm32-rs/stm32f4xx-hal/tree/master/src/gpio if you want to develop it further.

Wow, thanks for the quick response! Would you mind if I added you as reviewer to the other GPIO PRs I have? They include some of the other functionality you mentioned. I haven't included some of the smarter GPIO pin allocation stuff that the M4 library does because it seems like a big change that would have impacts on the downstream work that I've already done and want to push upstream before I start making additional changes like that.

@burrbull
Copy link
Member

It seems that quite a few of the peripherals are very similar between the H7 and H5 (with some notable differences like a completely new DMA engine on the H5).

Was it really needed to create separate repo/crate from stm32h7xx-hal?
Maybe just add H5 support there?

@astapleton
Copy link
Contributor Author

It seems that quite a few of the peripherals are very similar between the H7 and H5 (with some notable differences like a completely new DMA engine on the H5).

Was it really needed to create separate repo/crate from stm32h7xx-hal? Maybe just add H5 support there?

While a number of the peripherals are quite similar, some are also quite different: the H5 has a completely new DMA engine (GPDMA). In other ways the families are also quite different. The H5 family is all Cortex-M33 single core MCUs, vs the very complex multi-core H7s with mixed Cortex-M7/M4 cores. It introduces a slew of new security peripherals/modules, which can gate access to peripherals and other functionality, as well as support for some sort of Trusted Execution Environment implementation on a subset of the cores. It also uses a completely new system control block (SBS).

All that said, I think there's enough difference between the families to justify a different HAL to avoid adding significant complexity to the stm32h7 crate. I think if I was looking for a way of sharing code between the various ST modules, I think moving more of the peripheral control into separate crates would probably be my preferred solution. The SPI/I2C/GPIO/UART peripherals are quite similar across a number of families, not just the H7 and H5 families.

@astapleton astapleton force-pushed the as/gpio-base branch 2 times, most recently from 83e274f to 6c7580a Compare September 5, 2024 20:31
@astapleton astapleton merged commit 526ceba into master Oct 8, 2024
15 checks passed
@astapleton astapleton deleted the as/gpio-base branch October 8, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants