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

DigitalInOut: support for inverting output #4230

Open
dhalbert opened this issue Feb 20, 2021 · 11 comments
Open

DigitalInOut: support for inverting output #4230

dhalbert opened this issue Feb 20, 2021 · 11 comments

Comments

@dhalbert
Copy link
Collaborator

@ZodiusInfuser asked in #4228 whether DigitalInOut has functionality to invert the sense of .value, so that True represents a low signal level , and False represents high. The motivation is that Pimoroni has boards where the RGB LEDs are common anode. But this functionality could also be used for buttons that ground when on, etc.

MicroPython has a Signal class that wraps Pin and has an optional invert argument: classmachine.Signal(pin_obj, invert=False).

DigitalInOut could take an optional invert argument, which would invert the sense of .value. That might be all that is necessary.

We could also leave .value alone, and add, say, an .on property that depends on invert. Or we could add a .level property that always corresponds to high and low, and let .value be influenced by invert. But these extra properties may not be necessary.

One still needs to know whether a pin is active-high or active-low. Right now the board-specific pin names do not indicate that. We could start adding _INV or some other indicator to their names to make that clearer.

@gamblor21
Copy link
Member

I like the idea, though it may be good alongside a guide (or reference) on buttons and why they are "False" when pressed and how this can invert it. At least as a newbie that got me for a while, maybe I missed something that exists too. But I think it would make code flow a lot better to set some invert type property and the rest of the code can say if pressed is True.

In terms of CP would potentially inverting .value though cause issues with pre-existing libraries that expect it a certain way? Maybe a new property would be better?

@jfurcean
Copy link

It would be helpful to have an easier way to indicate that a button is pressed is True instead of having to use negation in user code. An additional property like .on or .active would probably be a better solution than an invert argument that modified .value.

button = DigitalInOut(board.GP0)
button.direction = Direction.INPUT
button.pull = Pull.UP

# this is counterintuitive for a newbie to electronics
if button.value:
    print('button is NOT pressed')
if not button.value:
    print('button IS pressed')

Using a new property would have the benefit of being more intuitive when using DigitalInOut for buttons, while also requiring less code changes if someone decides to invert their wiring on a project.

button = DigitalInOut(board.GP0)
button.direction = Direction.INPUT
button.pull = Pull.UP

if button.on:
    print('button IS pressed')
if not button.on:
    print('button is NOT pressed')

@deshipu
Copy link

deshipu commented Feb 20, 2021

I see no reason why you couldn't write a Signal class in Python (and add it, say, to simpleio) for this purpose, without having to add it to digitalio and making the space on the samd21 boards even tighter.

@deshipu
Copy link

deshipu commented Feb 20, 2021

You could even have a Button class that also does debouncing for you.

@jfurcean
Copy link

I see no reason why you couldn't write a Signal class in Python (and add it, say, to simpleio) for this purpose, without having to add it to digitalio and making the space on the samd21 boards even tighter.

You could even have a Button class that also does debouncing for you.

I agree with both of these points, but was offering support that the solution to the original issue could have a benefit with using buttons as well.

@jerryneedell
Copy link
Collaborator

I am a bit concerned about adding it to Digitalio -- at some point, we do want people to understand the hardware. Messing with .value in Digitalio really hides things.... I agree with @deshipu that this should be implemented at a higher level like simpleio.

@jepler
Copy link
Member

jepler commented Feb 20, 2021

I feel like this is related to the discussion we had In The Weeds recently about whether to have a ".voltage" property of AnalogIn objects, or have code need to scale ".value * .reference_voltage / 65535". I think we concluded that while the decision to have a 0..65535 value was also made very early when it was unclear whether CircuitPython would support floating-point numbers at all, it was better to leave the core as-is and put added functionality in Python code.

That said, because of some hardware & software I used to use, I miss the "invert" property in digitalio. And it's worth mentioning that at least the RP2040 has a hardware invert. This is used, for instance, to implement the two different clock polarity options of a PIO implementation of SPI (separate programs are needed for the two clock phase options, but in this way only 2 programs are needed rather than 4). But adding an invert to rp2pio is different than adding it to digitalio.

@Neradoc
Copy link

Neradoc commented Feb 20, 2021

I've kind of wanted a .on property that would be value != pull basically. In fact I use a Button class to encapsulate a DigitalInOut for that purpose in some of my code and give it a .on. For outputs I kind of think of it as an equivalent to pull in the way it is a default value: decide what that value is, treat .on as not being that.

That kind of abstraction is already done in board-specific libraries like magtag.button_a_pressed (return not self.buttons[0].value). In Pimoroni's case, they could have such a library for their board to encapsulate those things rather than change the behavior of the base pin, though I can see the appeal to have the board.LED_xx take care of it from the board definition.
It could even be in the Community bundle with upcoming support in circup.

@dhalbert
Copy link
Collaborator Author

We could add .inverted property to specific board. pins, which would make it automatic. However, that doesn't solve the problem of external devices which are not known in advance.

A simple Signal-like class would help. However, you still have to know to specify the inverted behavior. So I am still interested in naming pins with active-low behavior in a way that helps the reader, such as a _INV suffix. On schematics one typically puts an overbar over the signal name or adds a N suffix.

The naming convention be a cue to set .inverted in DigitalInOut or whatever wrapper class is created or just to take care to use active-low values.

Another idea would be to add a .inv_value property to DigitalInOut, which would be just the inverse of value. If you read code that uses a _INV pin and you're not using .inv_value, that would be a flag.

@deshipu
Copy link

deshipu commented Feb 22, 2021

I just want to note that after the whole discussion several years ago about adding a Signal class and renaming value to ``on`, breaking dozens of libraries in the process, to date I am not aware of a single piece of public code that would use that class. Many of the libraries, though, were never updated and are still broken.

@tannewt
Copy link
Member

tannewt commented Feb 22, 2021

I don't really like the idea of changing DigitalInOut. value is meant to represent the gpio's binary value, not really "on-ness" of a button or LED.

My gut aligns with @jerryneedell's point to do it in Python. However, having native support would be very nice so that board could have objects that work out of the box.

What if we added two new native classes LED and Button (module TBD)? They'd be similar to Signal except they'd lean into what they are. LED could be explicitly documented as being on the cathode or anode of the LED and never pulled. Button could always be pulled and perhaps even include "background" functionality similar to gamepad. New boards could have LED-only pins only provide LED objects then. (The raw pin is always available in microcontroller.pin, maybe that means it's "in-use" though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants