Const generic GPIOs #343

burrbull · 2021-07-09T15:17:11Z

No description provided.

therealprof · 2021-07-10T16:32:53Z

@TheZoq2 This is similar to the change landed in the stm32f4xx-hal and to me it marks a huge improvement. What's your opinion?

TheZoq2

Sorry for the delay, as you can probably tell I've been distracted by other things.

Looks like a nice change overall, I left some review comments for the main gpio.rs file. I don't really have the energy to review the erased and partially erased files right now since it's 10:30 pm, I'll see if I can get that done tomorrow

TheZoq2 · 2021-07-13T20:04:37Z

src/gpio.rs

+        let i = self.pin_id();
+        match edge {
+            Edge::RISING => {
+                exti.rtsr
+                    .modify(|r, w| unsafe { w.bits(r.bits() | (1 << i)) });
+                exti.ftsr
+                    .modify(|r, w| unsafe { w.bits(r.bits() & !(1 << i)) });
+            }
+            Edge::FALLING => {
+                exti.ftsr
+                    .modify(|r, w| unsafe { w.bits(r.bits() | (1 << i)) });
+                exti.rtsr
+                    .modify(|r, w| unsafe { w.bits(r.bits() & !(1 << i)) });
+            }
+            Edge::RISING_FALLING => {
+                exti.rtsr
+                    .modify(|r, w| unsafe { w.bits(r.bits() | (1 << i)) });
+                exti.ftsr
+                    .modify(|r, w| unsafe { w.bits(r.bits() | (1 << i)) });
+            }


Looks like we now use a runtime value determine the constant to write, is that constant folded away? I.e. is the compiler smart enough to figure out that self.pin_id is constant?

Yes, compiler optimize out this match to same code as before in release mode.

TheZoq2 · 2021-07-13T20:06:32Z

src/gpio.rs

-                    Ok(unsafe { (*$GPIOX::ptr()).odr.read().bits() & (1 << self.i) == 0 })
+            impl<MODE> PEPin<MODE, $port_id> {
+                pub fn erase(self) -> EPin<MODE> {
+                    EPin::$PXx(self)


I don't really like calling this EPin, to me, it's not obvious that that stands for erased pin, should we just call it ErasedPin instead?

TheZoq2 · 2021-07-13T20:08:47Z

src/gpio.rs

-
-            impl<MODE> Mode<MODE> for Generic<MODE> {}
+/// Pin
+pub struct Pin<MODE, CR, const P: char, const N: u8> {


Another naming question: Should we stick with P and N here, or just write out Port and Number. The latter is clearly more verbose, but also makes it less ambiguous

It will be too long to use it everywhere. I will add it to docs better.

TheZoq2 · 2021-07-13T20:09:35Z

src/gpio.rs

+    }
+    #[inline(always)]
+    fn port_id(&self) -> u8 {
+        P as u8 - 0x41


Where does this 0x41 come from?

This is A to 0 conversion.

TheZoq2 · 2021-07-13T20:13:31Z

src/gpio.rs

-                    }
+    #[inline(always)]
+    fn _is_set_low(&self) -> bool {
+        unsafe { (*Gpio::<P>::ptr()).odr.read().bits() & (1 << N) == 0 }


Missing //NOTE(unsafe) comment

TheZoq2 · 2021-07-13T20:14:59Z

src/gpio.rs

-                            $PXi::<Output<PushPull>>::set_mode(cr)
-                        }
-                    }
+// embedded_hal impls


This comment looks wrong, we are not impling embedded_hal in this block, right?

TheZoq2 · 2021-07-13T20:24:49Z

src/gpio.rs

-                        }
-                    }
+// embedded_hal impls
+impl<MODE, CR, const P: char, const N: u8> Pin<Output<MODE>, CR, P, N> {


I haven't tested this, but what happens if you use embedded_hal::digital::v2::OutputPin, will the compiler not throw an "ambiguous function call error" since we have both the local set_high and the OutputPin set_high in scope.

Is this a problem? Not having to unwrap certainly has its advantages, but at the same time, it would make code like

use embedded_hal::digital::v2::OutputPin; fn generic_fn(pin: impl OutputPin) { // ... } fn main() { let pin = // get a pin pin.set_high(); // Now ambiguous? }

break, which might be confusing if the generic stuff is added later in a project.

Ok, I tested it in the playground and it seems like this is a non-issue. I guess I'm not entirely sure how method resolution works in rust.. I'll leave this comment here in case it is an issue in this case, but feel free to ignore it.

The inherent methods on type are always preferred than that from traits.
This trick was discussed on matrix where we were decided to use only fallible traits in embedded-hal.

TheZoq2 · 2021-07-13T20:25:50Z

src/gpio.rs

+        impl<const P: char> Cr<$CR, P> {
+            // NOTE(allow) we get a warning on GPIOC because it only has 3 high pins
+            #[allow(dead_code)]
+            pub(crate) fn cr(&mut self) -> &pac::gpioa::$CR {


Maybe I'm misunderstanding things, but should this really be gpioa, seems like this is more generic?

Sorry. Didn't notice.

TheZoq2 · 2021-07-13T20:26:57Z

src/gpio.rs

+        {
+            /// Configures the pin to operate as an alternate function push-pull output
+            /// pin.
+            #[inline]
+            pub fn into_alternate_push_pull(
+                self,
+                cr: &mut Cr<$CR, P>,
+            ) -> Pin<Alternate<PushPull>, $CR, P, N> {
+                // Alternate function output push pull
+                const CNF: u32 = 0b10;
+                // Output mode, max speed 50 MHz
+                const MODE: u32 = 0b11;
+                const BITS: u32 = (CNF << 2) | MODE;
+
+                // input mode
+                cr.cr().modify(|r, w| unsafe {
+                    w.bits((r.bits() & !(0b1111 << Self::OFFSET)) | (BITS << Self::OFFSET))
+                });
+
+                Pin::new(Alternate::_new())
+            }

-                        cr.cr().modify(|r, w| unsafe {
-                            w.bits((r.bits() & !(0b11 << OFFSET)) | ((speed as u32) << OFFSET))
-                        });
-                    }
-                }
+            /// Configures the pin to operate as an alternate function open-drain output
+            /// pin.
+            #[inline]
+            pub fn into_alternate_open_drain(
+                self,
+                cr: &mut Cr<$CR, P>,
+            ) -> Pin<Alternate<OpenDrain>, $CR, P, N> {
+                // Alternate function output open drain
+                const CNF: u32 = 0b11;
+                // Output mode, max speed 50 MHz
+                const MODE: u32 = 0b11;
+                const BITS: u32 = (CNF << 2) | MODE;
+
+                // input mode
+                cr.cr().modify(|r, w| unsafe {
+                    w.bits((r.bits() & !(0b1111 << Self::OFFSET)) | (BITS << Self::OFFSET))
+                });
+
+                Pin::new(Alternate::_new())
+            }


Why can these not use the set_mode functions?

burrbull · 2021-07-13T23:20:27Z

Talking about erased pin we have alternative implementation for it:
https://github.com/stm32-rs/stm32f4xx-hal/blob/master/src/gpio/erased.rs
It is not same beautiful but should be more optimized.

burrbull · 2021-07-25T11:59:45Z

@TheZoq2 Can we merge this now?

TheZoq2

I don't think we should add external crates for dependencies, apart from that I think this looks good

TheZoq2 · 2021-07-25T18:44:35Z

Cargo.toml

@@ -52,6 +52,7 @@ mfrc522 = "0.2.0"
 serde_derive = "1.0.90"
 usb-device = "0.2.3"
 usbd-serial = "0.1.0"
+hd44780-driver = "0.4.0"


I don't think we should depend on crates just for examples, compile times etc. get way worse that way

I think we have had some discussions on this before, perhaps a stm32f1xx_examples repo would be in order?

Ok. I've removed last commit.

talhaHavadar · 2021-09-26T00:35:30Z

Is there an alternative for downgrade function?

Found it already 👍 (ErasedPin)

burrbull mentioned this pull request Jul 9, 2021

Const generic GPIO's #324

Closed

burrbull force-pushed the cg-gpio2 branch 8 times, most recently from d0956bb to 95ebb02 Compare July 10, 2021 13:42

burrbull mentioned this pull request Jul 10, 2021

Infallible Pin #341

Closed

TheZoq2 reviewed Jul 13, 2021

View reviewed changes

burrbull force-pushed the cg-gpio2 branch 2 times, most recently from 65b6fd2 to 0a376b5 Compare July 14, 2021 08:31

burrbull mentioned this pull request Jul 14, 2021

Add GPIOF/GPIOG support #327

Closed

burrbull added 11 commits July 14, 2021 12:06

PinExt

b63d870

const generic pins

a913ea7

associated CR in PinMode + OFFSET

f76d292

rm Mode trait

04e6dce

CR

888fbd0

make PinMode sealed instead of unsafe

c21c2f8

Add GPIOF, GPIOG support

5b45175

rename Pxx to EPin, Generic to PEPin

f208591

split gpio on modules

639bc9f

review fixes

8a7beb9

fix example

a7d9580

burrbull force-pushed the cg-gpio2 branch from 0a376b5 to a7d9580 Compare July 14, 2021 09:10

exti_rtic

9214d0b

TheZoq2 reviewed Jul 26, 2021

View reviewed changes

burrbull force-pushed the cg-gpio2 branch from 3fd490c to 9214d0b Compare July 26, 2021 15:35

TheZoq2 merged commit afe280c into stm32-rs:master Jul 26, 2021

TheZoq2 mentioned this pull request Jul 27, 2021

Examples using drivers #347

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Const generic GPIOs #343

Const generic GPIOs #343

burrbull commented Jul 9, 2021

therealprof commented Jul 10, 2021

TheZoq2 left a comment

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021 •

edited

Loading

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

TheZoq2 Jul 13, 2021

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

TheZoq2 Jul 13, 2021

burrbull Jul 13, 2021

burrbull commented Jul 13, 2021

burrbull commented Jul 25, 2021

TheZoq2 left a comment

TheZoq2 Jul 25, 2021

burrbull Jul 26, 2021

talhaHavadar commented Sep 26, 2021 •

edited

Loading

Const generic GPIOs #343

Const generic GPIOs #343

Conversation

burrbull commented Jul 9, 2021

therealprof commented Jul 10, 2021

TheZoq2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burrbull Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burrbull commented Jul 13, 2021

burrbull commented Jul 25, 2021

TheZoq2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talhaHavadar commented Sep 26, 2021 • edited Loading

burrbull Jul 13, 2021 •

edited

Loading

talhaHavadar commented Sep 26, 2021 •

edited

Loading