Basic GPIO support #24

JennToo · 2021-04-17T23:33:03Z

I don't have a lot of experience with embedded Rust, so I'm definitely looking for feedback. This design is mostly inspired by other HALs like the STM32 ones.

The example code I've included seems to be enough to get the on-board Pico LED to turn on or off. I have no idea if it's legit to bring GPIO out of reset before doing clock stuff, but the clock stuff is WIP in that other PR and I don't feel anywhere near smart enough to help with that 😅

Things that are still TODO for this PR, that I plan to tackle if people are happy with what I've got so far:

Input
Docs

Things I'm considering out of scope for this PR:

QSPI IO config
Any GPIO functions other than SIO
Bundling multiple GPIO pin updates into a single write (is this even a thing the embedded crates support? it would be handy, but seems like it could be difficult)
Interrupts

Related to #4 and #19

tdittr

Hi,

thanks for picking this up. Your implementation looks good to me. The only major thing I would change is to move the ownership of the SIO and pads peripherals also into the driver. What do you think about that?

I might have overlooked something with the macro as I am not yet really used to their syntax. 😄

Cargo.lock

tdittr · 2021-04-24T15:33:47Z

rp2040-hal/Cargo.toml

@@ -13,4 +13,4 @@ license = "MIT OR Apache-2.0"
 cortex-m = "0.7.1"
 embedded-hal = "0.2.4"
 nb = "1.0.0"
-rp2040-pac = "0.1.1"
+rp2040-pac = { git = "https://github.com/rp-rs/rp2040-pac", branch="main" }


I think this might cause problems when we add this to crates.io. Do you need something specific from the main branch? Maybe we should ask them for a new release in that case.

Yes we'll want a release of that crate again before this one.
Between the crates.io version of the PAC and the main branch, GPIOs seem to have been rearranged quite a bit.

Indeed, a new version would be needed for the PAC; commit rp-rs/rp2040-pac@56198b7 was a breaking change to how GPIOs are handled but it never made it into a release. That commit also breaks the pico-blink-rs example based on the PAC, but I see a PR has been submitted to fix that.

rp2040-hal/src/gpio.rs

tdittr · 2021-04-24T15:36:10Z

rp2040-hal/src/gpio.rs

+pub trait GpioExt {
+    type Parts;
+
+    // TODO: Do we need a marker to check that clocks are up?


Yes I think a marker or token would be good. But I would keep it this way until the clock modules are implemented.

rp2040-hal/src/gpio.rs

Sorry this is a large commit :( This adds support for input pins, including pulling them high or low. It also adds two examples: the start of a classic blinky LED example, and an example for reading input.

tdittr

Great to see so much progress. The implementation looks quite clean and complete to me. I only added some more minor points I noticed.

The only major point would be to use const generics instead of a macro. But I can't find a good example of another HAL already doing that. So if you want to keep the macro feel free to do so we can always come back to this in another iteration.

rp2040-hal/src/gpio.rs

tdittr · 2021-04-25T06:06:31Z

rp2040-hal/src/gpio.rs

+// const FUNCTION_PIO0: u8 = 6;
+// const FUNCTION_PIO1: u8 = 7;
+// const FUNCTION_CLOCK: u8 = 8;
+// const FUNCTION_USB: u8 = 9;

 macro_rules! gpio {


After a good nights sleep I think we can solve this without a macro by using const generics.

Instead of types for Gpio0, Gpio1 etc. there would be just one type Gpio<const N: u8>. This could make the docs easier and less redundant. And might also make it easier for some editors because the macro does not need to be expanded.

But we can also change it in a future iteration. I actually did not yet try const generics in the wild so I do not know what problems might show up.

Yes I think for this first round it might be good to stick with what the other HALs are doing, though eliminating the macro would be ideal. The macro makes it way harder to understand what is going on.

rp2040-hal/src/gpio.rs

tdittr · 2021-04-25T06:15:17Z

rp2040-hal/src/gpio.rs

+                        self,
+                    ) -> $PXi<Output> {
+                        unsafe {
+                            setup_pad_io(&*pac::$PADSX::ptr(), $i);


I would move the dereferences into their own helper methods. That would move all the unsafe to one area. In turn all the functions using them would become easier to read.

So I'm relatively new to unsafe. To me the reason I didn't do that was because I basically didn't want the de-reference to escape the unsafe block, since it would be unsafe if we were to use it outside of this context.

Oh I see, yes we need to make sure that the reference can't escape. We can use lifetimes to achieve that.

fn pads<'a>(&'a self) -> &'a pac::pads_bank0::RegisterBlock { unsafe {&*pac::$PADSX::ptr()} }

Which ensures that the reference to the register block can at most live as long as the Gpio$i.

To make it simpler we can use lifetime elision which happens whenever you take one reference as parameter and return one reference from the function:

fn pads(&self) -> &pac::pads_bank0::RegisterBlock { unsafe {&*pac::$PADSX::ptr()} }

Now that I think about it, it might be better to use &mut since we modify the register.

Strangely the registers themselves don't seem to require mut to write to them. I wonder if that means we should follow the same pattern.

Oh yes, that is weird. I would tend to take &mut but I'm not sure now. The OutputPin trait for example takes &mut self.

rp2040-hal/src/gpio.rs

rp2040-hal/examples/blinky.rs

Co-authored-by: tdittr <tdittr@users.noreply.github.com>

We're technically supposed to wait for these resets to finish before poking at registers. This seems to fix the instability I was seeing on the input example especially (TBH I have no idea how it ever worked)

rp2040-hal/src/gpio.rs

This moves almost all of the unsafe stuff together. The last remaining bit is going to need a PAC fix.

Co-authored-by: tdittr <tdittr@users.noreply.github.com>

Szpadel · 2021-04-26T14:47:09Z

.cargo/config

@@ -1,3 +1,11 @@
 [build]
 # Instruction set of Cortex-M0+
 target = "thumbv6m-none-eabi"
+
+[target.'cfg(all(target_arch = "arm", target_os = "none"))']


Why we need those flags?

They seem to be the magic incantations needed to get the embedded Rust stuff to build. I copied them from the sample project.

link-arg=--nmagic is needed if flash or ram is not aligned to 0x10000

link-arg=-Tlink.x" is LLD linker (I think needed if you need to link C dependencies)

but i'm not sure why we need the last 2

A quick check on my machine showed that just `cargo check --workspace` didn't verify the examples. Quite silly.

tdittr

I think we should merge this as it is now and keep on iterating on the function over time :)

9names · 2021-05-04T06:24:31Z

I think we should merge this as it is now and keep on iterating on the function over time :)

Agreed. I'll fix up the simple merge conflict and bring it in.

9names · 2021-05-04T09:53:38Z

CI failure here is because "cargo test" fails due to missing embedded-hal traits on the CI host.
Will merge this after #32 which disables running cargo test in CI

Add basic output support for GPIOs

45580ec

tdittr reviewed Apr 24, 2021

View reviewed changes

JennToo added 5 commits April 24, 2021 16:18

Add input support, examples, SIO/PADS ownership

2c3a095

Sorry this is a large commit :( This adds support for input pins, including pulling them high or low. It also adds two examples: the start of a classic blinky LED example, and an example for reading input.

Cleanup wacky imports in examples

2e5c1fc

Block input reads on Unknown state

3536604

Finish configuration options for IOs

94f67f7

Add documentation

422a45f

JennToo marked this pull request as ready for review April 24, 2021 23:41

tdittr suggested changes Apr 25, 2021

View reviewed changes

9names reviewed Apr 25, 2021

View reviewed changes

rp2040-hal/examples/blinky.rs Outdated Show resolved Hide resolved

JennToo and others added 4 commits April 25, 2021 10:15

Apply suggestions from code review

037fc66

Co-authored-by: tdittr <tdittr@users.noreply.github.com>

More review comments

b3b7677

Do reset in a good way instead of a wacky way

7e45c96

Pull pads out of reset and wait

8290368

We're technically supposed to wait for these resets to finish before poking at registers. This seems to fix the instability I was seeing on the input example especially (TBH I have no idea how it ever worked)

tdittr reviewed Apr 25, 2021

View reviewed changes

rp2040-hal/src/gpio.rs Show resolved Hide resolved

JennToo and others added 2 commits April 25, 2021 12:13

Concentrate the unsafe register constructs

eaea9ae

This moves almost all of the unsafe stuff together. The last remaining bit is going to need a PAC fix.

Update rp2040-hal/src/gpio.rs

c4ae152

Co-authored-by: tdittr <tdittr@users.noreply.github.com>

Szpadel reviewed Apr 26, 2021

View reviewed changes

JennToo added 5 commits April 26, 2021 11:57

Merge branch 'main' of github.com:rp-rs/rp-hal into basic-gpio

0ff5152

Also check examples

d738626

A quick check on my machine showed that just `cargo check --workspace` didn't verify the examples. Quite silly.

Install arm-none-eabi-gcc in CI

0badff4

apt-get requires sudo

09d9247

Fix ARM toolchain apt package name

a4b15a3

tdittr approved these changes May 3, 2021

View reviewed changes

Merge branch 'main' into basic-gpio

7588f76

9names mentioned this pull request May 4, 2021

Running tests on CI #33

Closed

Merge branch 'main' into basic-gpio

d26e13c

9names merged commit 19cb392 into rp-rs:main May 4, 2021

JennToo deleted the basic-gpio branch May 4, 2021 22:29

Lichtso mentioned this pull request Oct 1, 2021

GPIO interrupt #147

Merged

afaber999 mentioned this pull request Jul 21, 2022

Reading/writing to flash #257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic GPIO support #24

Basic GPIO support #24

JennToo commented Apr 17, 2021 •

edited

Loading

tdittr left a comment

tdittr Apr 24, 2021

JennToo Apr 24, 2021

Nic0w Apr 25, 2021

tdittr Apr 24, 2021

tdittr left a comment

tdittr Apr 25, 2021

JennToo Apr 25, 2021

tdittr Apr 25, 2021

JennToo Apr 25, 2021

tdittr Apr 25, 2021 •

edited

Loading

JennToo Apr 25, 2021

tdittr Apr 26, 2021

Szpadel Apr 26, 2021

JennToo Apr 26, 2021

Szpadel Apr 26, 2021

tdittr left a comment

9names commented May 4, 2021 •

edited

Loading

9names commented May 4, 2021

Basic GPIO support #24

Basic GPIO support #24

Conversation

JennToo commented Apr 17, 2021 • edited Loading

tdittr 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

tdittr 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

tdittr Apr 25, 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

tdittr left a comment

Choose a reason for hiding this comment

9names commented May 4, 2021 • edited Loading

9names commented May 4, 2021

JennToo commented Apr 17, 2021 •

edited

Loading

tdittr Apr 25, 2021 •

edited

Loading

9names commented May 4, 2021 •

edited

Loading