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

Conversion between InputPin and OutputPin #82

Open
peckpeck opened this issue Apr 13, 2021 · 26 comments
Open

Conversion between InputPin and OutputPin #82

peckpeck opened this issue Apr 13, 2021 · 26 comments

Comments

@peckpeck
Copy link

We should be able ton convert InputPin to OutputPin and vice versa.
This should be done by consuming the xxPin object and creating a new one.

This would make the compiler automatically detect error based on the pin type while still allowing pin mode change. And it would remove the need for an IoPin that is a cause of bugs that can only be detected at runtime.

@peckpeck
Copy link
Author

Note that removing IoPin would help removing some duplicate code and many macros in the gpio code.

@golemparts
Copy link
Owner

IoPin was added to reduce overhead when pins constantly switch between input and output, such as charlieplexing and bitbanging I2C, where every bit of performance increase counts. I do like your suggestion of being able to convert between the various pin types, which should help to remove some implementation clutter.

@peckpeck
Copy link
Author

peckpeck commented Apr 14, 2021

Another usage of IoPin is putting a ping into alt mode. You either need to implement that within Pin or have an AltPin type (that would have no specific method i think).

@peckpeck
Copy link
Author

I could try to implement this, but would you accept a PR that would remove IoPin ?

@golemparts
Copy link
Owner

I appreciate the offer, but I would hold off on submitting a PR, as I would like to keep IoPin as is (for now) since it serves a purpose, and many of the existing structs might change based on the article you shared about zero sized pins. Once 0.12.0 is out the door and I've thought a bit more about any sweeping changes, I'll definitely let you know where I could use some help.

@peckpeck
Copy link
Author

One thing that is not present in the zero sized pin article, is that it is impossible to give those pin a generic type, so you cannot put them as is in an array or a vec.
The solution is either to create an enum with all those pin, or to create a Pin trait and store boxed traits.

@peckpeck
Copy link
Author

I've got a question on this subject, what is the usage of does reset on drop ?

  • is it to leave the pin on the same state after the program ends
  • or is it to use a pin, dropping it (without reset) let a subfunction acquire a pin, drop it (with reset) and reuse the pin ?
  • something else ?

@golemparts
Copy link
Owner

I've got a question on this subject, what is the usage of does reset on drop ?

  • is it to leave the pin on the same state after the program ends

Correct. By default, GPIO pins (and other peripherals where applicable) are reset to their previous state when they're dropped (although this won't work if a program is terminated abnormally). To prevent this behavior in specific cases, reset_on_drop was added. For instance, a program could be used to switch a connected device on or off, where the state of the pin needs to be preserved after the program exits.

@peckpeck
Copy link
Author

I understand, in this case,wouldn't it be better to implement it once in the Pin structure instead of in each pin type

@golemparts
Copy link
Owner

wouldn't it be better to implement it once in the Pin structure instead of in each pin type

I agree that's something to consider, however I could argue for both implementations. On the one hand it's a good idea to keep the code in one place, even though it technically already is, and just copied using a macro. On the other hand, the mode isn't changed until a Pin is consumed, and it makes sense to balance the mode change in the constructor of InputPin/OutputPin/IoPin with a reset in the destructor.

@peckpeck
Copy link
Author

Indeed

@peckpeck
Copy link
Author

but it prevents me from implementing into_output in InputPin.

By the way, I would ague that it should be implemented in the Gpio struct 😁 since it is the struct that starts with the program and goes out of scope when it finishes, ie when the reset should happen

@peckpeck
Copy link
Author

And pullupdwn mode is not reset properly since it doesn't seem possible to read it.

@golemparts
Copy link
Owner

For pull-ups/downs we do the best we can - disable any that were set when the InputPin was configured.

Apparently the BCM2711 on the 4B and 400 does allow the state to be read, but that would create too much of an inconsistency with all previous Raspberry Pi models where that isn't possible.

@peckpeck
Copy link
Author

Doing this ins't consistent either

  • Output -> reset toPullOff
  • IoPin -> reset to PullOff
  • InputPin -> reset to the pull mode passed as a parameter (ie the new one)

I think it would be more consistent to always pull off

@golemparts
Copy link
Owner

I think it would be more consistent to always pull off

As far as I'm aware it already does that. The macro that adds a Drop impl for all three sets it to PullUpDown::Off if it was set to something other than that.

@golemparts
Copy link
Owner

Actually, now that I'm looking at the code, it looks like it actually doesn't do that, so this would be a bug. I'll add it to the list to fix.

@golemparts
Copy link
Owner

Scratch that, it is implemented correctly so it's working as intended and always turns it off. Still early here, so took a bit of puzzling.

@peckpeck
Copy link
Author

Oh my bad

@peckpeck
Copy link
Author

I keep dropping my thoughts here, but I think that (Unconfigured)Pin should not be public, you should directly get an InputPin ou an OutputPin from gpio, the user should not get a pin in an unknown state.

@golemparts
Copy link
Owner

Pin is public so a GPIO pin's mode and logic level can be read without explicitly changing the mode. examples/gpio_status.rs is a good example of where this is useful, since it needs that information to print the status of all pins without modifying the mode.

@peckpeck
Copy link
Author

Is the acquire semantic useful here ? Because a read_mode method on the gpio object could be sufficient.

@golemparts
Copy link
Owner

Theoretically everything could be a method on Gpio including mode changes, logic level changes, etc, but that was changed on purpose to add ownership to pins by having to acquire a specific GPIO pin before you can interact with it, and reduce mistakes/remove checks by having an explicit InputPin and OutputPin.

There are multiple ways to implement GPIO pins. The current implementation is what made sense at the time to stay true to Rust's design principles. I appreciate any suggestions, but that is unlikely to change.

@peckpeck
Copy link
Author

Another question, why do pin methods require a "&mut self" while the mutable property is not required ?

@golemparts
Copy link
Owner

why do pin methods require a "&mut self" while the mutable property is not required

mut was used in places where it technically isn't needed, because I think it's good practice to show intent to developers using the library that those methods make changes to the system, while everything else is guaranteed not to. Internally unsafe memory writes are used which Rust can't make any guarantees about, so marking those methods as mutable adds some additional precautions. And while the current implementation may not require those methods to be mutable, there might be a useable safe option in the future that does require it, and I like having them future-proofed by already being marked that way, so a major version update isn't needed after reaching 1.0.0 just because the internal implementation changed.

@Xavientois
Copy link

If conversions between InputPin and OutputPin are added, would that allow for the IoPin trait to be implemented on all of the Pin types?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Feature requests
  
Awaiting triage
Development

No branches or pull requests

3 participants