Skip to content

Allow keyword arguments for digital pin setup #5124

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

Closed
Andon-A opened this issue Aug 11, 2021 · 9 comments
Closed

Allow keyword arguments for digital pin setup #5124

Andon-A opened this issue Aug 11, 2021 · 9 comments

Comments

@Andon-A
Copy link

Andon-A commented Aug 11, 2021

As it sits right now, I can set up nearly any type of pin input and output in one line. I can, for example, use:

led=pwmio.PWMOut(board.D13, frequency=5000, duty_cycle = 32767)

And this will start a PWM output that, on most boards, will turn the on-board LED on at half power. Similarly, AnalogIn and AnalogOut don't require any setup at all.

DigitalInOut, on the other hand, requires this, to largely accomplish the same thing:

led = digitalio.DigitalInOut(board.D13)
led.switch_to_output(True)
led.value = True

It would be nice if we could do something like this:

led = digitalio.DigitalInOut(board.D13, value=True)

Or, if you're doing an input:

led = digitalio.DigitalInOut(board.D13, pull=digitalio.Pull.UP )

It would definitely be nice to be able to start every pin in a single line, and importing another library just to do that seems a little excessive.

@dhalbert dhalbert added this to the Long term milestone Aug 11, 2021
@anecdata
Copy link
Member

anecdata commented Aug 11, 2021

Would also presumably obviate the need for the alternative 2nd step in the current flow: led.direction = Direction.OUTPUT. This sequence actually does require 3 steps currently.

@Neradoc
Copy link

Neradoc commented Aug 11, 2021

The most concise way to define a digital pin is currently 2 steps, not counting the imports.

led = digitalio.DigitalInOut(board.LED)
led.switch_to_output(True)  # LED is on
button = digitalio.DigitalInOut(board.BUTTON)
button.switch_to_input(digitalio.Pull.UP)

I have seen a couple of people on discord mention that they find the setup for pins in Circuitpython complicated (though they might think more of the 3-step version seen in many guides), and I tend to find it a little too verbose myself. In addition to require installing a library, simpleio's DigitalIn and DigitalOut don't seem to be used in learn guides and therefore nobody knows they exist.

However, maybe it's too ambiguous to have the "input" or "output" direction of the pin be implicit in the use of keyword arguments (whereas PWMOut,AnalogIn,AnalogOut are explicit). And there is the possibility of setting value and pull at the same time, or neither (though that can be managed with exceptions and good defaults).

It might be better to have something like DigitalIn/DigitalOut in the core, or in the form of helper functions that return a DigitalInOut like say digitalio.digital_out(pin, value).

@dhalbert
Copy link
Collaborator

dhalbert commented Aug 11, 2021

I think this is not too hard:

def  __init__(
    self, 
    pin: microcontroller.Pin,
    *,
    value: Optional[bool] = None,
    pull: Optional[Pull] = None,
    drive_mode: Optional[DriveMode] = None) -> None:

If value is None, the pin is set to be an input. If pull is None, it is set to be PULL_NONE. If drive_mode is not None, raise a ValueError.

If value is not None, the pin is set to be an output, with the given initial value. If drive_mode is not None, it is set to be DRIVE_PUSH_PULL. If pull is not None, raise a ValueError.

I personally would rather not add DigitalIn and DigitalOut, because it complicates things from a beginner's point of view, and raises the question of whether these can be set to the opposite direction or whether they are stuck that way. If they are in a Python library, then supplying that library is a nuisance, or it has to be frozen in.

@anecdata
Copy link
Member

Worth keeping mind that that we can "read" the value of an output:

led.value = not led.value

@tannewt
Copy link
Member

tannewt commented Aug 11, 2021

The reason we don't allow it from the constructor is that it's weird to have keyword arguments that are only valid depending on another (direction). Value and drive mode are for output and pull is for input. The class does default to input so we could add a pull kwarg. That'd simplify the input case. I'd push back against combining them.

The other examples don't have this weird dependency between kwargs.

@Andon-A
Copy link
Author

Andon-A commented Aug 11, 2021

Perhaps instead of setting pull or value directly, an independent arg could be used? I'll call it startvalue because it's the value the pin starts with. Unless I'm missing something, you could use four arguments and, since things are exclusive anyway, there'd be no cases of keywords depending on other keywords.

My thought process says you'd have the following:

pin = digitalio.DigitalInOut(board.D13, startvalue=True) # This would start with an output with the pin high
pin = digitalio.DigitalInOut(board.D13, startvalue=False) # This would start with an output with the pin low
pin = digitalio.DigitalInOut(board.D13, startvalue=digitalio.Pull.UP) # Start with a pullup enabled input
pin = digitalio.DigitalInOut(board.D13, startvalue=digitalio.Pull.DOWN) # Start with a pulldown enabled input
pin = digitalio.DigitalInOut(board.D13) # Start a non-pull input, as per current function.

So the argument would accept a boolean (For output values) or a pull direction (For input values), or default to how it works now.

@dhalbert
Copy link
Collaborator

startvalue

You still want to be able specify the drive_mode, too, on construction.

I personally don't mind the keyword arguments depending on each other, because there are easy idioms for both the input and output cases. It's a deficiency of Python that it's painful to have multiple constructors: you can use class methods, but it's rare.

@Andon-A
Copy link
Author

Andon-A commented Aug 11, 2021

drive_mode being a separate argument might work.

My thoughts on it:
If used in conjunction with startvalue being True or False, it works as expected.
It could be ignored with Pull.UP or Pull.DOWN, instead of having exceptions
If used with no startvalue then it could default to having a value of False (As is current) and then set the pin to the appropriate output mode.

@dhalbert
Copy link
Collaborator

I'll say this is subsumed by #9845.

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

5 participants