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

machine/pin: add pin.ConfigureAsInput() and pin.ConfigureAsOutput() helper functions #1537

Closed
wants to merge 1 commit into from

Conversation

deadprogram
Copy link
Member

@deadprogram deadprogram commented Dec 20, 2020

This PR adds the PinConfigInput() and PinConfigOut() helper functions to avoid so much repetition when just using the default values for config:

Before:

	led.Configure(machine.PinConfig{Mode: machine.PinOutput})

After:

	led.Configure(machine.PinConfigOutput())

You can still use the longer form, this is just a shortened form since the defaults are so often used.

@deadprogram deadprogram changed the title machine/pin: add PinConfigInput() and PinConfigOut() helper functions machine/pin: add PinConfigInput() and PinConfigOutput() helper functions Dec 21, 2020
@deadprogram
Copy link
Member Author

Any opinions on this PR @aykevl @conejoninja @sago35 anyone? 😸

@conejoninja
Copy link
Member

I like the idea, it's not only shorter (not much) but easier/simpler. Could the code be placed inside machine.go only once? It smells a little that it needs to be copied/pasted in every machine_xxx.go file, we'll need to make sure new devices add those functions too.

To make it even simpler, what about :

led.ConfigureAsOutput()
led.ConfigureAsInput()

@aykevl
Copy link
Member

aykevl commented Jan 7, 2021

What about pull up and down? It seems to me that those are rather important and also supported on most chips (I believe AVR is an exception and it only supports pull up, not pull down).

@aykevl
Copy link
Member

aykevl commented Jan 7, 2021

Also, what problem are you trying to solve exactly? Adding PinConfigInput and PinConfigOutput (returning a machine.PinConfig) do not remove a dependency on the machine package.

@deadprogram
Copy link
Member Author

This change is not intended to remove dependency on machine. It is only to simplify the syntax for default values. One benefit is it removes the need to explain "what is an empty struct" for simple examples. The purpose is to streamline the experience for new programmers, as well as people new to the Go language.

In fact, I much prefer @conejoninja suggestion, and I think I will change this PR to incorporate it.

Regarding pull-up vs. pull-down, the proposed change to pin.ConfigureAsInput() should probably do the most likely thing on that hardware. For devs who require more specific control, the current pin.Configure() is still intended to be used. The simplified syntax is only intended as a means for shortcut to the most common default.

@aykevl
Copy link
Member

aykevl commented Jan 8, 2021

Ah okay, thank you for the explanation.

I'm not entirely convinced that adding more API surface will simplify things, to be honest. However, the current API is a bit more complicated than necessary so maybe we can simplify something? This might break existing code depending on what we do.

For example:

pin.Configure(machine.PinOutput)
pin.Configure(machine.PinInput)
pin.Configure(machine.PinInputPullup)

And if we later need more options (which so far don't seem to be necessary), they can be implemented with extra methods, such as:

pin.SetDriveStrength(10) // 10mA drive strength

@aykevl
Copy link
Member

aykevl commented Jan 8, 2021

Although, maybe the suggestion from @conejoninja is better as it also allows using the pins in an interface. For example, a hypothetical button driver could use this interface:

type Pin interface {
    ConfigureAsInputPullup()
    Get() bool
}

It still looks a bit ugly to me to have four methods for essentially one thing (ConfigureAsInput, ConfigureAsOutput, ConfigureAsInputPullup, ConfigureAsInputPulldown) but it would work neatly in an interface. And it doesn't break all the TinyGo code that already exists.

@deadprogram
Copy link
Member Author

Although, maybe the suggestion from @conejoninja is better as it also allows using the pins in an interface.

That right there does achieve both the simplification I am seeking, and also gives us an interface for testing.

So do we want to go with this:

type Pinner interface {
    ConfigureAsInputPullup() error
    ConfigureAsInputPulldown() error
    ConfigureAsOutput() error
    Get() bool
    Set(bool) error
}

or this:

type Pinner interface {
    ConfigureAsInput() error
    ConfigureAsOutput() error
    Get() bool
    Set(bool) error
}

Where ConfigureAsInput() will be pulldown or pullup depending on what is the default for that hardware.

What do you think?

@conejoninja
Copy link
Member

I'm more inclined to the simplest ConfigureAsInput() if possible. I didn't see any "Arduino" tutorial that makes the difference between pull-up / pull-down, at least in entry level.

@deadprogram
Copy link
Member Author

Seems like pullup is more common then pulldown. We could default to that for ConfigureAsInput() and document to use the longer syntax if you need a pulldown?

@aykevl
Copy link
Member

aykevl commented Jan 8, 2021

What do you mean, default?
There are three possible ways to configure an input: with a pulldown, a pullup, and floating. machine.PinInput configures the input as a floating input and is what people probably expect (a pulldown or pullup can have side effects you don't want). So, there is no "correct" or "default" pull, except that AVR does not usually support pulldown.
The pull mode is simply an extra added resistor to VCC or GND, floating means that there is no such register and something else needs to pull the line up or down (such as an external resistor or an output of another chip).

Regarding the interface: I would avoid making one shared interface type. Instead, I think every driver should define the subset that the driver needs as this can vary greatly. One driver might only need ConfigureAsOutput and Set, while another might also need to configure a pin as an input.

@deadprogram
Copy link
Member Author

The interface will be defined in the consumer aka the drivers repo, not here.

I just pushed a commit to this branch with the ConfigureAsOutput() and ConfigureAsInput() functions. Based on what you were saying, perhaps they should default to floating inputs instead of pullup?

@deadprogram deadprogram changed the title machine/pin: add PinConfigInput() and PinConfigOutput() helper functions machine/pin: add pin.ConfigureAsInput() and pin.ConfigureAsOutput() helper functions Jan 8, 2021
@deadprogram
Copy link
Member Author

OK I have rebased and squashed commits. I think this is now ready to go, unless there is additional feedback.

@deadprogram
Copy link
Member Author

@aykevl any other feedback on this PR?

@deadprogram
Copy link
Member Author

Closing, since we decided not to do this.

@deadprogram deadprogram deleted the gpio-config-helpers branch March 31, 2023 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants