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 ReadBool and WriteBool to GpioController #121

Closed
shaggygi opened this issue Dec 27, 2018 · 5 comments
Closed

[Proposal] Add ReadBool and WriteBool to GpioController #121

shaggygi opened this issue Dec 27, 2018 · 5 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

@shaggygi
Copy link
Contributor

shaggygi commented Dec 27, 2018

This proposal is an addition to the GpioController class that include helper methods for working with the different pin values and their respective boolean value.

Rationale and Usage

There are many times you need to determine the bit value and must perform conversions of setting/clearing the bit. Pins sometimes represent bits within a value. The proposal would help from having to write redundant code in apps and device bindings. For example, reading pin values while clocking data to/from a device.

Example

The following writes the different bits to a pin.

BitArray bitArray = new BitArray(new byte[] { 0x1A });

foreach (bool bitValue in bitArray)
{
  if (bitValue)
  {
    _controller.Write(1, PinValue.High);
  }
  else
  {
    _controller.Write(1, PinValue.Low);
  }
  
  // Other logic...
}

The proposed changes would allow the code to be written as:

BitArray bitArray = new BitArray(new byte[] { 0x1A });

foreach (bool bitValue in bitArray)
{
  _controller.WriteBool(1, bitValue);

  // Other logic...
}

Proposed Changes

The proposed changes are to add the following helper methods within GpioController class.

/// <summary>
/// Reads the current boolean value of a pin.
/// </summary>
/// <param name="pinNumber">The pin number in the controller's numbering scheme.</param>
/// <returns>The boolean value of the pin.</returns>
public bool ReadBool(int pinNumber)
{
    return PinValueToBool(_controller.Read(pinNumber));
}

/// <summary>
/// Writes a boolean value to a pin.
/// </summary>
/// <param name="pinNumber">The pin number in the controller's numbering scheme.</param>
/// <param name="value">The boolean value to be written.</param>
public void WriteBool(int pinNumber, bool value)
{
    _controller.Write(pinNumber, BoolToPinValue(value));
}

/// <summary>
/// Converts a pin value to a boolean value.
/// </summary>
/// <returns>The boolean value of the pin value.</returns>
public static bool PinValueToBool(PinValue value)
{
    return value == PinValue.High;
}

/// <summary>
/// Converts a boolean value to a pin value.
/// </summary>
/// <returns>The pin value of the boolean value.</returns>
public static PinValue BoolToPinValue(bool value)
{
    return value ? PinValue.High : PinValue.Low;
}

Open Questions

  1. I'm not sure the proposed method names are the best. I used WriteBool/ReadBool as you can't overload Read to return PinValue or bool. What would be the best names?

  2. Should there be an option as a bool? where null represents closed pin or high-impedance state? I don't think so as that makes things a little more complex. Low (false) and High (true) should be good enough.

@joperezr
Copy link
Member

I'm not sure how valuable these methods would be, since in reality you could still have the above code written in one line like:

BitArray bitArray = new BitArray(new byte[] { 0x1A });

foreach (bool bitValue in bitArray)
{
  _controller.Write(1, (bitValue) ? PinValue.High : PinValue.Low);
  
  // Other logic...
}

Granted it is not as readable as an overload, but I'm just not convinced that we should add extra methods for this. Also, if we were to add them, I would much rather just have an overload of Write that takes in a bool instead of a PinValue than having different method names.

@joperezr joperezr added 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 labels Jan 10, 2019
@shaggygi
Copy link
Contributor Author

Yeah, I originally wanted the Write/Read, but since you couldn't have a Read for returning both PinValue or a bool is why I added the Bool at end. I agree, just use the logic above will be fine. Closing.

@krwq
Copy link
Member

krwq commented Feb 4, 2019

@shaggygi you can have a ReadBool or ReadAsBool and Write with different overload. Reopening as I personally find the enum cumbersome and prefer bool and keep on noticing the pattern @joperezr mentioned more and more in the PRs - I think we should keep things as simple as possible

@krwq krwq reopened this Feb 4, 2019
@joperezr
Copy link
Member

I believe that this experience should be better now that PinValue is a struct right? You should now be able to use either int or bool on the write method by just casting. @krwq do you agree that after this change we don't need these extra Apis any longer?

@krwq
Copy link
Member

krwq commented Feb 12, 2019

I think I'm fine with this as us after @JeremyKuhne's changes. @shaggygi? (closing this assuming you agree)

@krwq krwq closed this as completed Feb 12, 2019
@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

3 participants