Skip to content

Factor common GPIO code out of stm32{g0,h7}#416

Merged
cbiffle merged 1 commit intomasterfrom
stm32-gpio-refactor
Feb 4, 2022
Merged

Factor common GPIO code out of stm32{g0,h7}#416
cbiffle merged 1 commit intomasterfrom
stm32-gpio-refactor

Conversation

@cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Feb 1, 2022

This is a step toward reducing the overall high level of code
duplication between stm32 targets. It exploits some traits that the PACs
started generating circa 0.14, which are unfortunately not put into a
common crate, but, hey, at least they exist now.

Knocks some bytes (100+) off GPIO servers by reducing macro-driven code
duplication and giving the compiler an easier task.

This is sort of a HAL, but unlike most HALs it's very careful to allow
static monomorphization on properties you know at compile time (like
your SoC model target) and only use dynamic dispatch for things you vary
at runtime (like which gpio port you're poking).

Scary bits are mostly hidden in the server.rs file.

@cbiffle cbiffle force-pushed the stm32-gpio-refactor branch 3 times, most recently from 93f9496 to 2c0b6b8 Compare February 2, 2022 20:53
@cbiffle cbiffle requested a review from mkeeter February 2, 2022 21:51
@cbiffle cbiffle force-pushed the stm32-gpio-refactor branch from 2c0b6b8 to 753727b Compare February 2, 2022 21:52
do_configure!(gpio, pins, packed_attributes);
}
}
unsafe { get_gpio_regs(port) }.configure(pins, packed_attributes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

General design question - why is getting the GPIO registers unsafe, but writing to them isn't?

This seems like the opposite of normal, e.g. getting a raw pointer is safe but dereferencing it is unsafe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

siiiiiiiiigh

So.

The family of APIs that grew up around svd2rust decided, early on, to try to manage peripheral ownership using little "tokens" that can't be duplicated. You do Peripherals::take() at boot and it gives you a struct of tokens, and you can access peripherals through those tokens. This approach is useful in some contexts.

We don't use it, though, because it assumes that all code is in the same address space and is simply unsound in unprivileged isolated mode.

This means we're sort of on the gray areas of the svd2rust-generated APIs. In particular, they decided that any access to peripherals outside of this token scheme would be unsafe. This is an abuse of unsafe to mean something other than unsafe -- technically there are peripherals one could poke that could violate memory safety, like anything capable of remapping memory or a DMA controller, but they are exceptions and the GPIO peripheral is certainly not among them.

So, a side effect of their decision is that there's no static GPIOA item defined, to prevent "ambient access" to the GPIOA peripheral bypassing the token scheme. Instead, all they'll do is give us a raw pointer from the GPIOA::ptr() call, which is the only call we can make without holding one of their tokens. We have to indirect it to get at the actual RegisterBlock, which is unsafe-in-the-Rust-sense.

Now, in this case, why did I also make get_gpio_regs unsafe, rather than hiding the unsafe within it? I could go either way on that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, that all makes sense. I don't feel strongly about this, so matching svd2rust's decisions (however misguided) seems reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(This is related to the reason why accessing raw bits is unsafe on some registers and not others. Basically the default is to make any bit manipulation unsafe; someone has to go through and edit a giant YAML file to mark operations as ok to do from safe code. People have not done that for our processors. It gets really fun when they've done it for some of our processors and not others, which gets us "unnecessary unsafe" warnings when compiling for some targets. This is part of why I'm concerned that our use case is diverging too far from what the PAC authors intend.)

@cbiffle cbiffle force-pushed the stm32-gpio-refactor branch from 753727b to f58f8ee Compare February 4, 2022 19:29
@cbiffle cbiffle requested a review from mkeeter February 4, 2022 19:30
@cbiffle cbiffle force-pushed the stm32-gpio-refactor branch from f58f8ee to a27ba32 Compare February 4, 2022 19:33
This is a step toward reducing the overall high level of code
duplication between stm32 targets. It exploits some traits that the PACs
started generating circa 0.14, which are unfortunately not put into a
common crate, but, hey, at least they exist now.

Knocks some bytes (100+) off GPIO servers by reducing macro-driven code
duplication and giving the compiler an easier task.

This is sort of a HAL, but unlike most HALs it's very careful to allow
static monomorphization on properties you know at compile time (like
your SoC model target) and only use dynamic dispatch for things you vary
at runtime (like which gpio port you're poking).

Scary bits are mostly hidden in the server.rs file.
@cbiffle cbiffle force-pushed the stm32-gpio-refactor branch from a27ba32 to 2e7f3dd Compare February 4, 2022 19:36
@cbiffle cbiffle merged commit 2935ba2 into master Feb 4, 2022
@cbiffle cbiffle deleted the stm32-gpio-refactor branch February 4, 2022 19:52
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.

2 participants