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

[WIP/RFC] Improve GPIO API #211

Merged
merged 4 commits into from
Jun 24, 2020
Merged

[WIP/RFC] Improve GPIO API #211

merged 4 commits into from
Jun 24, 2020

Conversation

TheZoq2
Copy link
Member

@TheZoq2 TheZoq2 commented May 1, 2020

The current GPIO API does not really allow interraction with devices that
require inputs to change type at runtime. For example, ultrasonic sensors where
you send a short pulse to them, then read their reply on the same pin, or dht22
sensors which have a similar interface. This was discussed, for example, in #114

I aim to add two ways of working around this. Right now, I have implemented a
closure based approach, where the pins have a function to temporarily change
the pin mode and use that changed pin in a closure. (Currently called
as_<mode>, bikeshedding welcome)

I think this will work well in sequential applications, but might become
annoying or slow in interrupt heavy applications as it requires a mutable
reference to the control register. However, it should be very fast and does not
require any falible runtime checks for pin modes

The second mode I think would be useful would be fully dynamic. It would keep
track of the state of the pin at runtime, it would implement both OutputPin
and InputPin with a Result<_, DynamicPinError> to notify the programmer of
incorrect modes. I think it might make sense to only implement this for the
Generic pin since you already have to sacrifice some performance to use it.

Standing issues:

  • Document the new modes. Specifically, which ones to use when and where

@TheZoq2
Copy link
Member Author

TheZoq2 commented May 1, 2020

And now I have added dynamic pins as well. Docs are still on my todo list

@TheZoq2
Copy link
Member Author

TheZoq2 commented May 1, 2020

And now docs have been added 🎉

@TheZoq2
Copy link
Member Author

TheZoq2 commented May 24, 2020

@therealprof Would you mind having a look at this?

src/gpio.rs Outdated

// These impls are needed because a macro can not brace initialise a ty token
impl<MODE> Input<MODE> {
fn _new() -> Self {
Copy link
Contributor

@burrbull burrbull May 25, 2020

Choose a reason for hiding this comment

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

const fn where it is possible, #inline other

Copy link
Member Author

Choose a reason for hiding this comment

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

I made all of these const, but I'm not sure how often we should use inline. My general feeling is that compilers are usually better at deciding when to inline things than we are, but maybe that's wrong. Would you like any more consts added?

Copy link
Member

Choose a reason for hiding this comment

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

#[inline] is just an indication that this might be useful to inline, it just shifts the bias a bit and will be ignored in dev mode altogether.

@therealprof
Copy link
Member

Spending 10 minutes with it it looks okay to me. The blank lines are a bit off and I'm not a big fan of the macro_rules! blocks bang in the middle of it.

@TheZoq2
Copy link
Member Author

TheZoq2 commented May 30, 2020

I'll take care of the blank lines and the const fn/inline stuff.

I also don't really like the macro_rules! in the middle of things. My reasoning for it is that I want to access a bunch of variables defined in the scope where the macros are defined.

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 20, 2020

I have now added a bunch of #[inline] declarations to most one-liner functions. Would you mind having another look? @therealprof @burrbull

@burrbull
Copy link
Contributor

@TheZoq2 have you seen rust-embedded/embedded-hal#29 ?

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 20, 2020

I had not, I'll have a look through it

@therealprof
Copy link
Member

Looks good to me. I don't care to deeply about this functionality so I don't really want to test the usability and functionality aspects myself but I trust you've done due dilligence. ;)

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jun 20, 2020

:D I don't think I did any more testing apart from writing and running the examples, but I'm hoping that will be enough

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.

None yet

3 participants