GPIO refactor (Simplify macro, reduce type states, and support interrupt) #189

Piroro-hs · 2020-12-24T16:11:44Z

Simplify macro
gpio.rs uses more generics than macro now
Collapse Input type arguments
An attempt to address Collapse Input<OpenDrain / PullUp / PullDown) into Input #125
Support interrupt
Close External interrupts (example, question) #83 and Interrupt support #112

Sh3Rm4n · 2020-12-25T19:26:36Z

Super interesting attempt. I'll take a closer look in the next few days. But this approach seems promising!

Sh3Rm4n

Some thoughts which came into my mind.

I've only read a small part of src.gpio.rs so far. Will look into this in more detail later.

Sh3Rm4n · 2021-01-03T21:33:52Z

src/gpio.rs

-/// Input mode (type state)
-pub struct Input<MODE> {
-    _mode: PhantomData<MODE>,
+/// GPIO Register interface traits private to this module


Also known as sealed traits :)

src/gpio.rs

David-OConnor · 2021-01-06T21:43:37Z

Could you please add support for GPIO interrupts? I'm modding this now to do so. I made a standalone module a while back for this (In my PR), but am switching it to GPIO to be consistent with L4 and F4 HALs.

API:

let btn = gpioa
    .pa15
    .into_floating_input(&mut gpioa.moder, &mut gpioa.pupdr);

btn.make_interrupt_source(&mut dp.SYSCFG, &mut rcc.apb2)
btn.enable_interrupt(&mut dp.EXTI)
btn.trigger_on_edge(&mut dp.EXTI, Edge::FALLING);

https://github.com/stm32-rs/stm32l4xx-hal/blob/master/src/gpio.rs#L129

In general, keep in mind that GPIO is very similar (identical?) across the STM32 line, so there's a lot to be learned from the other crates. Take the best from each.

Piroro-hs · 2021-02-13T13:53:15Z

GPIO interrupts support added.
Referred to f1xx-hal, f4xx-hal, and #200.

Piroro-hs · 2021-02-13T15:21:24Z

Clippy warnings should be fixed by #202

examples/gpio_interrupts.rs

seanybaggins · 2021-02-18T18:23:52Z

src/gpio.rs

+/// GPIO Register interface traits private to this module
+mod private {
+    pub trait GpioRegExt {
+        fn is_low(&self, i: u8) -> bool;


perhaps pin_num would be a more informative variable name than i?

Though i is / was pretty common to refer the pin number in this file, pin_num does not hurt.

src/gpio.rs

Sh3Rm4n · 2021-03-02T21:27:45Z

I'll take a closer look in the next few days. But this approach seems promising!

Next few months rather. I have this on my TODO list. I'm hopeful that I find time this week :)

Sh3Rm4n

Wow, this PR is huge. 👀 Both in code change, but also implementation wise.

Props to you, to unclutter the macro and leveraging the rust type-system and static polymorphism techniques, to not rely too heavily on macros to hold it all together.

Your extensive use of traits not only move most of the dispatch logic out of the macro, but actually makes it easier to read through the gpio implementation itself.

The interrupt implementation is a very welcomed additions as well.

Props to you and many thanks for this implementation. 🙏

I've added mostly nits as review comments, but design wise, this looks perfect. I'm not able to find any critiques, as I couldn't have done it any better than you. As if I could even do this myself 😆

Sh3Rm4n · 2021-03-03T20:56:45Z

examples/gpio_interrupts.rs

+    // Moving ownership to the global BUTTON so we can clear the interrupt pending bit.
+    cortex_m::interrupt::free(|cs| *BUTTON.borrow(cs).borrow_mut() = Some(user_button));
+
+    unsafe { NVIC::unmask(Interrupt::EXTI0) };


Is there an easy way, that the user button can give us the according EXTI interrupt line, so a manual lookup is not necessary, or is it too complicated?

This could be done in another PR though. It's a nice to have, not necessary.

Note to self: Lookup documentation for interrupt methods, if the user gets a hint, that he has to unmask the interrupts before utilizing them.

nvic(&self) -> pac::Interrupt method is added.

Sh3Rm4n · 2021-03-03T21:11:35Z

src/gpio.rs

+/// GPIO Register interface traits private to this module
+mod private {
+    pub trait GpioRegExt {
+        fn is_low(&self, i: u8) -> bool;


Though i is / was pretty common to refer the pin number in this file, pin_num does not hurt.

src/gpio.rs

src/serial.rs

src/gpio.rs

Sh3Rm4n

Minor tiny nit.

Overall this looks fantastic. Let me know if you want me to press the merge button 😀

Sh3Rm4n · 2021-03-25T20:39:32Z

src/gpio.rs

-                    pub struct AFRH {
-                        _0: (),
-                    }
+macro_rules! r_trait {


I see how this is macro is useful, but a comment would help to understand how it works / what its for.

Piroro-hs · 2021-03-26T13:30:52Z

Updated (add comments and move IntoAf.. traits to marker module).
This is ready to merge :)

Sh3Rm4n · 2021-03-27T14:49:35Z

Nice. Let's merge it 🚂

Thanks for your hard work, again! :)

Piroro-hs changed the title ~~Collapse Input<Floating / PullUp / PullDown> into Input~~ GPIO refactor (Simplify macro and reduce type states) Dec 25, 2020

Sh3Rm4n self-requested a review December 25, 2020 19:26

Sh3Rm4n reviewed Jan 3, 2021

View reviewed changes

Piroro-hs force-pushed the gpio-collapse branch 2 times, most recently from 103e950 to 584f11d Compare February 13, 2021 13:52

Piroro-hs changed the title ~~GPIO refactor (Simplify macro and reduce type states)~~ GPIO refactor (Simplify macro, reduce type states, and support interrupt) Feb 13, 2021

Piroro-hs added a commit to Piroro-hs/stm32f3xx-hal that referenced this pull request Feb 13, 2021

Update changelog for stm32-rs#189

10b0a21

Piroro-hs marked this pull request as ready for review February 13, 2021 16:11

seanybaggins reviewed Feb 18, 2021

View reviewed changes

seanybaggins mentioned this pull request Feb 23, 2021

Gpio exti declaritive #200

Closed

Sh3Rm4n self-requested a review March 2, 2021 21:27

Sh3Rm4n requested changes Mar 7, 2021

View reviewed changes

Piroro-hs and others added 12 commits March 14, 2021 13:01

Simplify gpio.rs macro

81d17bf

Implement into_xxx() methods for partially erased pins

831d6a6

codegen: Fix fmt and clippy

8e77d06

Add alternate function open-drain pins

a2393df

Collapse Input type arguments

b1a3451

Add SYSCFG wrapper, based on f4xx-hal

817252b

about to run code gen

1ffac2e

Add support for GPIO interrupts

907e553

added interrupt example

a6f3a91

Update docs

7ab776b

Update changelog for stm32-rs#189

3158004

Allow some peripherals to work with OD pins

bd36c7b

Piroro-hs force-pushed the gpio-collapse branch from fa42567 to 76d8213 Compare March 14, 2021 11:34

Piroro-hs force-pushed the gpio-collapse branch 3 times, most recently from cf00e5b to d5358fa Compare March 15, 2021 06:53

Piroro-hs mentioned this pull request Mar 15, 2021

Seal "private" traits 🦭 #169

Closed

Review comments and various fixes

b2f2fb3

Piroro-hs force-pushed the gpio-collapse branch from d5358fa to b2f2fb3 Compare March 20, 2021 02:54

Sh3Rm4n approved these changes Mar 25, 2021

View reviewed changes

More fixes

3dc5239

Sh3Rm4n merged commit 231c959 into stm32-rs:master Mar 27, 2021

Sh3Rm4n mentioned this pull request Mar 28, 2021

Collapse Input<OpenDrain / PullUp / PullDown) into Input #125

Closed

Piroro-hs deleted the gpio-collapse branch March 30, 2021 04:55

Sh3Rm4n mentioned this pull request May 19, 2021

MVP: Make use of const generics #224

Closed

Sh3Rm4n mentioned this pull request Jun 8, 2021

Tracking issue for refactoring #122

Open

12 tasks

Sh3Rm4n mentioned this pull request Aug 16, 2021

Interrupt support #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPIO refactor (Simplify macro, reduce type states, and support interrupt) #189

GPIO refactor (Simplify macro, reduce type states, and support interrupt) #189

Piroro-hs commented Dec 24, 2020 •

edited

Loading

Sh3Rm4n commented Dec 25, 2020

Sh3Rm4n left a comment

Sh3Rm4n Jan 3, 2021

David-OConnor commented Jan 6, 2021 •

edited

Loading

Piroro-hs commented Feb 13, 2021

Piroro-hs commented Feb 13, 2021

seanybaggins Feb 18, 2021

Sh3Rm4n Mar 3, 2021

Sh3Rm4n commented Mar 2, 2021

Sh3Rm4n left a comment •

edited

Loading

Sh3Rm4n Mar 3, 2021

Piroro-hs Mar 19, 2021

Sh3Rm4n Mar 3, 2021

Sh3Rm4n left a comment

Sh3Rm4n Mar 25, 2021

Piroro-hs commented Mar 26, 2021

Sh3Rm4n commented Mar 27, 2021

GPIO refactor (Simplify macro, reduce type states, and support interrupt) #189

GPIO refactor (Simplify macro, reduce type states, and support interrupt) #189

Conversation

Piroro-hs commented Dec 24, 2020 • edited Loading

Sh3Rm4n commented Dec 25, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

Sh3Rm4n Jan 3, 2021

Choose a reason for hiding this comment

David-OConnor commented Jan 6, 2021 • edited Loading

Piroro-hs commented Feb 13, 2021

Piroro-hs commented Feb 13, 2021

seanybaggins Feb 18, 2021

Choose a reason for hiding this comment

Sh3Rm4n Mar 3, 2021

Choose a reason for hiding this comment

Sh3Rm4n commented Mar 2, 2021

Sh3Rm4n left a comment • edited Loading

Choose a reason for hiding this comment

Sh3Rm4n Mar 3, 2021

Choose a reason for hiding this comment

Piroro-hs Mar 19, 2021

Choose a reason for hiding this comment

Sh3Rm4n Mar 3, 2021

Choose a reason for hiding this comment

Sh3Rm4n left a comment

Choose a reason for hiding this comment

Sh3Rm4n Mar 25, 2021

Choose a reason for hiding this comment

Piroro-hs commented Mar 26, 2021

Sh3Rm4n commented Mar 27, 2021

Piroro-hs commented Dec 24, 2020 •

edited

Loading

David-OConnor commented Jan 6, 2021 •

edited

Loading

Sh3Rm4n left a comment •

edited

Loading