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

Add an API for temporary GPIO reconfiguration #74

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Add an API for temporary GPIO reconfiguration #74

merged 2 commits into from
Feb 25, 2020

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Jan 16, 2020

This eliminates a frequent annoying pattern in embedded Rust when having to dynamically change the configuration of a pin. Where you previously would have to dance the Option dance:

static mut PIN: Option<PB2<Input<Floating>>> = ...;

let mut out = PIN.take().unwrap().into_push_pull_output();
// do stuff with `pin`
PIN = Some(out.into_floating_input());  // put it back

With this API you can instead do this:

static mut PIN: PB2<Input<Floating>> = ...;

PIN.with_push_pull_output(|pin| {
    // do stuff with `pin`
});

Of course, this doesn't fix cases where the configuration changes globally and would need to be stored back into the PIN resource, but it should still be helpful.

Please carefully review the bit manipulation as I have not yet tested this (I have lightly tested this now and it seems to work)

cc @lthiery

@jonas-schievink
Copy link
Contributor Author

@arkorobotics friendly ping about this :) Is there anything I should address to get this merged?

@arkorobotics
Copy link
Member

Sorry for the delay. Just to confirm, users can still use let mut out = PIN.take().unwrap().into_push_pull_output();, correct? I haven't had a chance to dive into the code, but if this feature is backwards compatible, I'll merge it. Otherwise I'd like to wait for v1.0.0.

@jonas-schievink
Copy link
Contributor Author

Yeah you can still do that, but it's not entirely backwards-compatible due to the new PinMode bounds.

Did you release a new version recently? I can't find one. If #72 is part of the next release then that's a breaking change anyways.

@arkorobotics
Copy link
Member

Copy that. I haven't yet, but I'm planning to review all outstanding PRs and issues this weekend to draw the line what will or will not make it into v1.0.0. Ideally, we'd group all breaking changes into it. That said, before making a decision I'd like to get @hannobraun input.

@dbrgn
Copy link
Contributor

dbrgn commented Feb 20, 2020

Wouldn't it be good to fix the pin function mappings before the 1.0.0 release? There are a wide range of STM32L0 devices that will not work with this HAL (as in "it will compile but not work, depending on the peripheral used"). To me, 1.0.0 would mean "it's not feature-complete but it should work reliably".

(See #76 and #77)

@dbrgn
Copy link
Contributor

dbrgn commented Feb 20, 2020

For the record, here are the different "gpio version" groups (WIP, generated with https://github.com/dbrgn/cube-parse). Our grouping into stm32l0x1, stm32l0x2 and stm32l0x3 seems to be wrong.

More details can be found here: stm32-rs/stm32f0xx-hal#29 (comment) I plan to continue working on this matter, with the goal of ideally having consistent pin mappings across all STM32 MCU HALs.

(By the way, is there a place to discuss stm32-wide changes that would affect multiple HALs, besides the rust-embedded Matrix channel? Maybe a hal-meta repo could be created to discuss relevant issues in a more permanent way than chat?)

@hannobraun
Copy link
Contributor

hannobraun commented Feb 21, 2020

I've taken a look, although I didn't do the close review that @jonas-schievink requested (lack of time). I really like this API, and the way it's implemented looks solid (like everything I see from Jonas). Better testing and closer review would be desirable, of course, but I'd merge this, if it were up to me.

@arkorobotics

if this feature is backwards compatible, I'll merge it. Otherwise I'd like to wait for v1.0.0.

@jonas-schievink

Did you release a new version recently? I can't find one. If #72 is part of the next release then that's a breaking change anyways.

I can definitely say that the current master is not backwards compatible with the current released version, due to the EXTI changes.

It's not my decision to make (nor do I want it to be my decision), but I'd be careful about releasing a 1.0 version at this point. I'm not so sure that more breaking changes aren't desirable (there have been a steady stream of them until recently, after all). And personally, I don't see the point of releasing 1.0, if that's going to result in 2.0, 3.0, 4.0 in short succession (as according to semantic versioning, every breaking change would require a major version increase once the major version is > 0).

@arkorobotics
Copy link
Member

Great points @hannobraun. I'll proceed with the merge.

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.

4 participants