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

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

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 myfork. 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.

I've gotten my fork to a point I'm reasonably happy with, and have done a little testing with it - yourgpio-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.

@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.

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

@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/toRequests. 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.

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 iflater 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 theembedded-hal traits even make sense for this?

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. theInput marker toInput<Floating>/Input<PullUp>/Input<PullDown> if needed.

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

What does that mean exactly? Would theembedded-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. theInput marker toInput<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.

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.

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.

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 aRequest.

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 aRequest.

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.

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. callinginto_pullup on aCdevPin<Input<PullUp>>.

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.

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 ofgpiocdev, 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 aCdevPin that is usable without ever touching agpiocdev::Request directly.

• Your goal seems to be to have a thin wrapper aroundgpiocdev::Request that only implements theembedded-hal traits.

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 isgpiocdev_embedded_hal.