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

[Proposal] Add System.Device.Gpio.Toggle() #107

Closed
josesimoes opened this issue Dec 17, 2018 · 12 comments
Closed

[Proposal] Add System.Device.Gpio.Toggle() #107

josesimoes opened this issue Dec 17, 2018 · 12 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins

Comments

@josesimoes
Copy link

Toggling a Gpio pin is a very, very common operation. To accomplish that, with what's available today, one has to either:

  1. Keep a variable to hold the pin state and then update the pin and the variable.
  2. Perform a read pin, toggle state and perform a write pin.

Most of modern MCUs provide a low level way of doing this with one or maybe a couple of instructions. Even if it requires implementing 2. in native code it would still be far more efficient that using 1. or 2. in managed code.

Considering the above, I'm proposing that a System.Device.Gpio.Toggle(int pinNumber) method should exist.

PS: We already have this on nanoFramework Windows.Devices.Gpio API, see here.

@grahamehorner
Copy link

a FlipFlop operation is also very common aka alternate between states (on/off) for x cycles.

@shaggygi
Copy link
Contributor

shaggygi commented Dec 17, 2018

@grahamehorner FlipFlop seems to be more of a type of circuit (https://en.wikipedia.org/wiki/Flip-flop_(electronics)). I could eventually see a bindings for a 7474-type component, though.

@shaggygi
Copy link
Contributor

I like option 1, but understand more logic is needed to hold state.

By having this feature, it would also fix the issue when trying to read an input's value

See following GpioController method.

public PinValue Read(int pinNumber)

@joperezr
Copy link
Member

The part I don't like about Toggle is that it means that we have to keep some sort of state around, which makes it dangerous for multi-threaded scenarios. If you wanted to implement this thread-safe, then you would be pretty much having to do a read and then a write inside the Toggle method, which in my mind won't add much value given that as you said you can already do this yourself by writing a small helper function.

@joperezr joperezr added enhancement New feature or request api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins and removed enhancement New feature or request labels Dec 20, 2018
@shaggygi
Copy link
Contributor

shaggygi commented Dec 20, 2018

Just throwing this out as an option for conversation. Not saying it is the best (actually causes more overhead) and probably simpler to continue using Write methods for this.

public PinValue Toggle(int pinNumber, PinValue currentValue)
{
    PinValue toggledValue;

    if(currentValue == PinValue.Low)
    {
        toggledValue = PinValue.High;
    }
    else
    {
        toggledValue = PinValue.Low;        
    }

    _controller.Write(pinNumber, toggledValue);
    return toggledValue;
}

// So used in example....
int pinNumber = 2;
PinValue currentValue = PinValue.Low;
_controller.OpenPin(clkPin, PinMode.Output);  // Setup clock pin.
_controller.Write(pinNumber, currentValue); // Set to initial low state.

// Do work...

// And toggle...
currentValue = _controller.Toggle(pinNumber, currentValue);  // Will toggle high.
currentValue = _controller.Toggle(pinNumber, currentValue);  // Will toggle low.

@josesimoes
Copy link
Author

@joperezr

it means that we have to keep some sort of state around

that won't be necessary, because the state is kept at the CPU level in the Gpio register.

The descriptions of "pseudo-code" I added to the issue are the usual way of dealing with this at the application. I don't have an extension for this nor anything similar.

My suggestion is for the toggle operation to be performed at the PAL level, right were the Gpio CPU registers are written. This won't require any state keeping and it's as thread safe as the existing Read or Write operations.

For CPUs that have a specific operation to toggle a Gpio pin, that would be called. For the others it would have to be mimicked with a simple 1) read Gpio register for current state 2) toggle value 3) write CPU Gpio register to update with the toggled value.

Is this more clear now? 😃

@joperezr
Copy link
Member

Right, what I meant is that if you would want to not keep state then you have to perform the two operations (read first, then write). Which CPUs have the functionality for toggling the value? AFAIK, the only one that "has" the functionality is the Windows IoT driver which if I'm not mistaken does a read and write internally anyways. All of our other existing drivers (UnixDriver, RaspberryPi driver and HummingBoard Driver) would have to do a read and write, unless we wanted to keep state.

@shaggygi
Copy link
Contributor

shaggygi commented Dec 27, 2018

Another example...

public void Toggle(int pinNumber, ref PinValue currentValue)
{
  if (currentValue == PinValue.Low)
  {
    currentValue = PinValue.High;
  }
  else
  {
    currentValue = PinValue.Low;
  }
  
  _controller.Write(pinNumber, currentValue);
}

@joperezr
Copy link
Member

I still don't think that this method would provide enough value for the potential cost (in some drivers this would require either a read/write operation, or it might also require some state to be preserved). Also, this could be easily implemented on a consumer's app in case they really need this functionality. I would like to see more feedback regarding this in order to consider it has enough value for more scenarios.

@shaggygi
Copy link
Contributor

I like this one better than Clock() proposal. However, I can only recall using it in with the GpioSpiDevice proposal (#122). I'd go ahead and close for now.

@krwq
Copy link
Member

krwq commented Feb 22, 2019

toggle can be usually done with:
foo ^= 0b0001000; if we are able to do something like that directly then this should be atomic if not then I got mixed feelings if we should have it

@krwq krwq added this to the Future milestone Feb 22, 2019
@joperezr
Copy link
Member

I agree with @krwq. I'll go ahead and close it since there doesn't seem to be something that would add a lot of value. Feel free to re-open if there are any new cases that would make this enhancement more valuable.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins
Projects
None yet
Development

No branches or pull requests

5 participants