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 getting GPIO state #4

Closed
wants to merge 1 commit into from

Conversation

RandomInsano
Copy link

Tested on real hardware. Are there other adjustments or tests I can set up?

@posborne
Copy link
Member

@RandomInsano although it may work on some hardware it will not work on others. For example, I have worked quite a lot with the Freescale/NXP imx series processors which do not support reading the output state of GPIO output pins. Caching the most frequent write value is not guaranteed to be correct either as there could be an external condition which is resulting in the actual voltage of the net not matching what we are attempting to set it to in software.

From https://elixir.bootlin.com/linux/v4.14.28/source/drivers/gpio/gpiolib-sysfs.c#L43:

 *   /value
 *      * always readable, subject to hardware behavior
 *      * may be writable, as zero/nonzero

The "subject to hardware behavior" is dealing with what the value means when it is an output mostly and it's pretty much all bets off.

@japaric Any thoughts on how to handle this? For some boards, the hardware provides no way to definitely know whether the value is high or low and, in general, there is no way to know for Linux as a whole (without knowing specifics of a given gpiochip in the kernel and hardware).

@RandomInsano
Copy link
Author

RandomInsano commented Mar 25, 2018

I've done some thinking on this, and it probably makes sense to throw an exception for those platforms where it's not possible. Maybe an unreachable!() or compile_error!() on those platforms where trying to pull that data doesn't make sense.

Possibly even a new hal-only macro.

I suppose that it would cause some compatibility problems for libraries...

@posborne
Copy link
Member

I've done some thinking on this, and it probably makes sense to throw an exception for those platforms where it's not possible. Maybe an unreachable!() or compile_error!() on those platforms where trying to pull that data doesn't make sense.

This problem isn't really specific to Linux so I can see two sane ways to handle:

  1. We keep the functions to read the state as part of the OutputPin trait and better define the fact that based on the support in the gpiochip (borrowing Linux terminology for the IP chunk through which the pin is accessed) the behavior of getting the state may borrow.
  2. Consider removing it from the core trait and creating a separate trait that would guarantee specific semantics on only be implemented on targets that can actually provide a guaranteed implementation of those semantics.

With either option, if an author decides that on Linux they know they are working with a board/gpiochip that supports reading GPIO state on an output pin (in keeping with whatever semantics are documented) then they could go ahead and implement the trait differently.

I'm currently working on a new GPIO library for linux based on the GPIO character device (the sysfs approach is deprecated and will be removed from the kernel in 2020). I'll try to think about how to handle cases like this a bit more.

@japaric
Copy link
Member

japaric commented Mar 27, 2018

Any thoughts on how to handle this?

I think we should rework the GPIO traits to be more flexible. If some devices don't support reading the state of their output then is_low / is_high should be in a separate trait. There's quite a bit of scattered discussion about this in the embedded-hal repo; there are some special considerations when using OutputPin with open drain pins as well, etc.

It's probably best to wait until we reach a conclusion there before deciding on this PR.

@RandomInsano
Copy link
Author

Better ideas have come up over the in embedded-hal project.

rust-embedded/embedded-hal#29

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.

3 participants