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

Implement Wait for CdevPin with tokio runtime. #110

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Jan 27, 2024

Depends on #92, i.e. we either need an enum error type, or completely switch to gpiocdev. Both are breaking changes, so a complete switch seems preferable.

Currently, all gpiocdev::Errors are unwrapped.

Replaced CdevPin implementation to be based on gpiocdev::Request.

Closes #92.

@reitermarkus reitermarkus requested a review from a team as a code owner January 27, 2024 18:55
@reitermarkus reitermarkus changed the title WIP: Implement Wait for CdevPin with tokio runtime. Implement Wait for CdevPin with tokio runtime. Jan 28, 2024
@reitermarkus
Copy link
Member Author

@rust-embedded/embedded-linux, please have a look here.

Copy link

@warthog618 warthog618 left a comment

Choose a reason for hiding this comment

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

Wrt the second commit:

The way you drop and re-request lines looks broken to me. You should be holding the request provided in new() and never dropping it. If you need to reconfigure the request use req.reconfigure(), don't drop it and re-request it. Dropping and re-requesting can create glitches on output lines and drop buffered edge events on input lines.

You should not need to self.request()? in set_low(), set_high() and is_high() - you should already hold the request.

That goes for wait_for_edge() too - dropping and re-requesting the line will lose events.
In fact you might want to check the config there and reconfigure() if the edge detection is incorrect. Or just assume that the user set edge detection appropriately before passing the Request to new(). Either way, be aware that the kernel may have buffered events, and those may be the wrong type so you always need to filter.

And line 41 looks redundant - see line 32.

@reitermarkus
Copy link
Member Author

@warthog618, thanks for the review, req.reconfigure() seems to be what I was missing.

@reitermarkus reitermarkus force-pushed the cdev-wait branch 4 times, most recently from b6f76e3 to 8d94dec Compare February 15, 2024 00:17
@warthog618
Copy link

warthog618 commented Feb 15, 2024

FWIW, I forked your patch to take a closer look and see what it would take to fix it. Of course one thing lead to another and what I've ended up with might be considered a complete re-write. I haven't done any testing with it yet, but it solves all the issues I've found, including ones I didn't list above, like panics - don't panic, and adds StatefulOutputPin support. I'll look at getting that pushed to github so you can take a look and compare it with where you are at.

This is my fork. That needs to be built against gpiocdev master, to get the derive Debug on the async wrappers, until that makes its way into a release.

@warthog618
Copy link

I've gotten my fork to a point I'm reasonably happy with, and have done a little testing with it - your gpio-wait example works for me on a Pi (though I use pins 22-23 there as they are easily jumpered).

I would like to tidy that up into a PR, if that is ok with you, and add a test suite for it as well.
Though before doing that I would like some feedback to ensure I'm not on the wrong track, ideally from the Embedded Linux team themselves.

I'm probably trampling a bunch of norms, so sorry about that, but I'm terrible at reviewing on github and I figured this is the quickest way to get to a working solution.

@reitermarkus
Copy link
Member Author

reitermarkus commented Feb 16, 2024

@warthog618, I took a look at your changes and made some improvements in here accordingly. Instead of two different types for input/output pins, I used marker types, which is a bit more common for HALs I think.

I'm terrible at reviewing on github

No worries. I think suggesting changes in the “Files changed” tab would be a bit easier to manage than comparing a whole rewrite and avoid duplicating effort, though.

@reitermarkus
Copy link
Member Author

For simplicity, I also left out any conversions from/to Requests. These can be added later if needed. It's probably best to not expose too many (or any) gpiocdev types for now.

@warthog618
Copy link

@warthog618, I took a look at your changes and made some improvements in here accordingly. Instead of two different types for input/output pins, I used marker types, which is a bit more common for HALs I think.

Not familiar with markers, and don't see the advantage so far.

I'm terrible at reviewing on github

No worries. I think suggesting changes in the “Files changed” tab would be a bit easier to manage than comparing a whole rewrite and avoid duplicating effort, though.

That is what I thought as well - but the changes rapidly became extensive and it was quicker to re-code than describe what I wanted to change.

For simplicity, I also left out any conversions from/to Requests. These can be added later if needed. It's probably best to not expose too many (or any) gpiocdev types for now.

I disagree - that allows for complex use cases that basic input/output doesn't, without wrapping gpiocdev functions. If the user has a complex use case, or even just want to set bias, they can construct the request themselves and then wrap that in a CdevPin and still get all the benefits of the embedded-hal traits.

Speaking of which, do you support requesting a line without setting direction? You would be surprised how often that gets requested.

@reitermarkus
Copy link
Member Author

If the user has a complex use case, or even just want to set bias

Which is why I said

These can be added later if needed.

even if later means another PR immediately after this, but I think it's easier to review this PR without these additional constructors.

Speaking of which, do you support requesting a line without setting direction?

What does that mean exactly? Would the embedded-hal traits even make sense for this?

@reitermarkus
Copy link
Member Author

Not familiar with markers, and don't see the advantage so far.

For the implementation, you don't need an “inner” struct to share code between input/output pins, and the API user only has to import one type. It's also possible to then extend e.g. the Input marker to Input<Floating>/Input<PullUp>/Input<PullDown> if needed.

@warthog618
Copy link

Speaking of which, do you support requesting a line without setting direction?

What does that mean exactly? Would the embedded-hal traits even make sense for this?

It means requesting the pin in whatever state it currently is in.
Frequently that is used to read an output line without setting the output value, whereas explicitly requesting as an output always sets the value. I never use it that way, but some people do, and the kernel supports it.

It still makes sense for InputPin.

Not familiar with markers, and don't see the advantage so far.

For the implementation, you don't need an “inner” struct to share code between input/output pins, and the API user only has to import one type. It's also possible to then extend e.g. the Input marker to Input<Floating>/Input<PullUp>/Input<PullDown> if needed.

So it is syntactic sugar. That approach looks like exposing all the internal variables that you want to hide and are not relevant for the HAL traits.

@eldruin
Copy link
Member

eldruin commented Feb 16, 2024

Thank you both for your work. It seems like there is a fruitful collaboration happening here.
I would be happy to go with the approach that is most comprehensive and fits best.
We are quite time-constrained so it is indeed best to submit smaller PRs if the changes can be separated and so speed up the review process.

@reitermarkus
Copy link
Member Author

So it is syntactic sugar. That approach looks like exposing all the internal variables that you want to hide and are not relevant for the HAL traits.

That's how the STM32 HALs do it. Of course it depends on how much state you want to encode in types, i.e. whether you prefer compile-time or runtime checks for changing states.

@reitermarkus
Copy link
Member Author

Frequently that is used to read an output line without setting the output value

So e.g. is_set_high would need to first read the value instead of checking the stored one? If so, that seems simple enough to add in a follow-up PR that also adds the conversion from such a Request.

@warthog618
Copy link

Frequently that is used to read an output line without setting the output value

So e.g. is_set_high would need to first read the value instead of checking the stored one? If so, that seems simple enough to add in a follow-up PR that also adds the conversion from such a Request.

It could be thought of as a StatefulOutputPin where the initial value is read from the current line state rather than being provided by the user. Some users use that to store state, e.g. did I leave the led on??, despite the kernel explicitly not guaranteeing that the line will remain as set after you release it. To do that with the kernel uAPI you have to request the line without any direction set, read the value, then reconfigure the line to explicitly be an output to allow you to modify the value. (that aspect of the uAPI is intentionally awkward - you must explicitly request the line as an output to change its value to ensure you aren't flipping the line to an output by accident.)

So it is syntactic sugar. That approach looks like exposing all the internal variables that you want to hide and are not relevant for the HAL traits.

That's how the STM32 HALs do it. Of course it depends on how much state you want to encode in types, i.e. whether you prefer compile-time or runtime checks for changing states.

I prefer compile time if at all possible, and after a quick look at the STM32 HALs my impression is that they feel the same and chose to expose their pin state to get the Rust compiler to do the access control on their peripheral registers for them at compile time. Nice.

But that doesn't apply here - the kernel does that for you at run-time, and that can't be avoided. And you don't need to check or care what, for example, the bias setting is to read or write the line value, so I don't see the value in it being exposed in the type. Those attributes are orthogonal to the HAL traits. The only relevant state here is direction.

@reitermarkus
Copy link
Member Author

I prefer compile time if at all possible

But that doesn't apply here

The only relevant state here is direction.

So in this case you prefer runtime checks, except for direction, which is the current state of this PR, correct?

I don't see the value in it being exposed in the type.

It allows users to ensure a certain bias by specifying the corresponding type, and it allows implementing the API in a way that forbids useless operations at compile time, e.g. calling into_pullup on a CdevPin<Input<PullUp>>.

@warthog618
Copy link

No, I do not prefer runtime checks, if you check my fork I explicitly refactored it into separate input/output classes to avoid runtime checks.

I don't see how checks for bias etc are at all relevant to these HAL traits, so I don't see the need for any checks. But you do whatever works for you.

@reitermarkus
Copy link
Member Author

I don't see how checks for bias etc are at all relevant to these HAL traits

I see what you mean. They are not relevant for the traits, they are only relevant to avoid exposing any implementation detail, i.e. usage of gpiocdev, to users of this crate.

Correct me if I am wrong, I think we have different design goals in mind:

  • My goal is to have a CdevPin that is usable without ever touching a gpiocdev::Request directly.

  • Your goal seems to be to have a thin wrapper around gpiocdev::Request that only implements the embedded-hal traits.

@warthog618
Copy link

Implementing traits should just be implementing those traits. If you go beyond that you need to ask yourself why the methods you are adding are not part of the traits. KISS.

Correct me if I am wrong, I think we have different design goals in mind:

* My goal is to have a `CdevPin` that is usable without ever touching a `gpiocdev::Request` directly.

* Your goal seems to be to have a thin wrapper around `gpiocdev::Request` that only implements the `embedded-hal` traits.

My goal is to provide both - an implementation that does not require any knowledge of gpiocdev for basic use cases, while still allowing access to the underlying gpiocdev::Request for more complex cases. My version is gpiocdev_embedded_hal.

@mjsir911 mjsir911 mentioned this pull request Mar 12, 2024
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.

Switch to gpiocdev-rs for GPIO uAPI v2 support?
3 participants