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

Pin.Configuration: a way to specify pin setup for constructors that take pins #9845

Open
dhalbert opened this issue Nov 29, 2024 · 3 comments

Comments

@dhalbert
Copy link
Collaborator

(This issue is an offshoot of #1270.)

A number of open issues (#1270, #6009, #7206, #8315, #8941, #8945, #9373, #9821, #9823) have asked for the ability to set pin drive mode, drive strength, or a pull for various *io and other objects. Currently these classes provide limited access to some of these settings, and sometimes don't provide them at all. For instance, drive strength is not available at all, and specific pull settings are not available in classes that could use them.

In more recent posts in #1270, there has been some discussion of how to to do this in general way, maybe by introducing a Pad class or making Pin objects mutable.

I have an idea for an upward-compatible addition which takes a different approach. In most or all API's which currently take a Pin, I propose the ability to additionally take a Pin.Configuration object, which specifies both the Pin and the desired Pin.DriveMode, Pin.Pull, and drive strength settings. A Pin.Configuration object would not change the pin: it's instead just some settings that are desired when the pin is set up. Some of the settings could be left unspecified for the particular class to choose.

DriveMode and Pull would be moved from digitalio. For drive strength, I propose passing an integer, which must be one of the integers in Pin.drive_strengths, as described #1270 (comment). So there is no Pin.DriveStrength enum.
Here is a rough pseudo-definition in Python:

class Pin:
   ...
   class Configuration:
       def __init__(self, pin: Pin, *, drive_mode: Optional[Pin.DriveMode] = None, drive_strength: Optional[int[ = None, pull: Optional[Pin.Pull] = None):
       # Args should be validated
       self._pin = pin
       self._drive_mode = drive_mode
       self._drive_strength = drive_strength
       self._pull = pull

       @property
       def pin(self):
           return self._pin

       # ... similar read-only properties for the other values

    # An existing pin can create a Configuration for itself:
    def configuration(self, pin: Pin, *, drive_mode: Optional[Pin.DriveMode] = None, drive_strength: Optional[int[ = None, pull: Optional[Pin.Pull] = None):
        return Configuration(pin, drive_mode=drive_mode, drive_strengh=drive_strength, pull=pull)

Right now, PWMOut can take a pin:

pwm = pwmio.PWMOut(board.D5)

But it could also take a Pin.Configuration, say to set the drive strength:

pwm = pwmio.PWMOut(board.D5.configuration(drive_strength=4))

Internally, in the shared-bindings constructors that take Pins as arguments, any bare Pin would be converted to a Pin.Configuration, and the Configuration objects would be what are passed to all the common-hal/ constructors. The shared-binding/ and common-hal/ constructors could reject invalid configurations if they made no sense for that kind of object or that particular port (e.g., the chip doesn't support the configuration on the peripheral). The constructors would call some shared setup routines to do the GPIO configuration as needed on pins or pass the appropriate arguments to the port-specific HAL layer

I am not wedded to Configuration as the name of the class. I also thought of:

Pin.Details
Pin.Config
Pin.Setup
Pin.Settings

It is important to emphasize that a Pin.Configuration does not make any changes itself to the pin settings. It's just a carrier of requested settings. This is because for some HAL layers, the pin is not alway set up by GPIO operations. For instance, in ESP-IDF, pulls are sometimes specified at the driver level for a particular peripheral, in a driver-specific configuration structure.

Pin.Configuration does not replace existing dynamic mode setting needs. For instance, DigitalInOut would still provide dynamic setting of Pull and DriveMode, since that is often necessary for matrix, scanning, bidirectional buses, etc.

The generalized mechanism provided by Pin.Configuration can make some current pull= and similar arguments unneeded. For instance, keypad.Keys takes a pull argument. Instead, the pull would be specified in the pins argument:

# current
keys = keypad.Keys((board.D1, board.D2), value_when_pressed=False, pull=True)
# new
keys = keypad.Keys([pin.configuration(pull=Pull.UP) for pin in (board.D1, board.D2), value_when_pressed=False)

This proposal is the result of some 3am thinking, so maybe I am missing something obvious. But I like the idea.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Nov 29, 2024

This is because for some HAL layers, the pin is not alway set up by GPIO operations. For instance, in ESP-IDF, pulls are sometimes specified at the driver level for a particular peripheral, in a driver-specific configuration structure.

EDIT to number proposal:

Proposal 2

I said that above, but it's possible that perhaps it's not really necessary, and the parameters could be changed later. In that case, here's a simpler alternative:

Add properties .drive_strength, .pull, .drive_mode to Pin, and/or add a .configuire() method. The settings can be made at any time, whether or not the pin is in use. Since using a pin for a *io object may reset the pin in various ways, the property settings can be made after the object is constructed.

The values for the properties should be available by reading the hardware, so that the Pin object can continue to be immutable.

@dhalbert dhalbert added this to the 10.0.0 milestone Dec 2, 2024
@tannewt
Copy link
Member

tannewt commented Dec 2, 2024

Overall, I like the PinConfiguration object idea. I wouldn't do board.D5.configuration but PinConfiguration(board.D5) to make it clear it is a shared settings container. I also like that the object given the configuration is still responsible for actually setting the values (and maybe changing them over time.)

Do we want to accept Pin or PinConfiguration in one argument or keep Pin separate and replace pull etc with PinConfiguration?

Add properties .drive_strength, .pull, .drive_mode to Pin, and/or add a .configuire() method.

I think the risk of this approach is that other things can change these settings too. There isn't some implicit ownership like the PinConfiguration idea maintains.

The values for the properties should be available by reading the hardware, so that the Pin object can continue to be immutable.

This isn't always true. I think there are some DigitalInOuts that have to cache these properties.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Dec 3, 2024

... I wouldn't do board.D5.configuration but PinConfiguration(board.D5) to make it clear it is a shared settings container. I also like that the object given the configuration is still responsible for actually setting the values (and maybe changing them over time.)

The use of such "factory" functions is pretty rare in Python, so that's OK with me.

Do we want to accept Pin or PinConfiguration in one argument or keep Pin separate and replace pull etc with PinConfiguration?

A number of the constructors take multiple pins, so passing a parallel sequence of PinConfigurations is a nuisance. The PinConfiguration above contains a reference to the pin, so it's self-identifying.


The comments below are re Proposal 2 above. I gave that as a bit of a strawman, but it's lower level and you have pointed out the disadvantages.

Add properties .drive_strength, .pull, .drive_mode to Pin, and/or add a .configuire() method.

I think the risk of this approach is that other things can change these settings too. There isn't some implicit ownership like the PinConfiguration idea maintains.

The values for the properties should be available by reading the hardware, so that the Pin object can continue to be immutable.

This isn't always true. I think there are some DigitalInOuts that have to cache these properties.

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

2 participants