Some platforms can to this on init already and will always just set
the GPIO to LOW which can be undesireable.

That's probably a bug in that implementation. gpio_init() should not touch the currently configured level.

I created a feature branch where I changed the behaviour for EFM32 w.r.t. this issue. If that works in #12380 then it should fix this issue for EFM32.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@basilfx want to give this a review?

@benpicco I gave this PR a review and I approve it partially.

Since this extends the GPIO API, I don' feel comfortable to ACK for that part. The change does make sense, and it is only a helper function that doesn't affect the rest.

Also, I think it needs at least a test that hits these two new functions.

@benpicco I think I got you covered for a test case: basilfx@173c05f.

Feel free to cherry-pick that one.

I feel strongly that the API should include a way to set the driven level of a GPIO in the same call as setting the mode to output, so I'm in favor of this PR.

Only the GPIO peripheral driver knows how to deterministically configure a pin as an output without unintentional momentary assertions of the wrong level. Consumers of the driver do not have a guaranteed way to do that with the current API. Most of them accomplish it by calling gpio_write() prior to ever calling gpio_init() which is an undocumented feature common to most platforms except notably efm32 as referenced in OP.

In fact I even feel that the API shouldn't include a way to set the mode to output without also defining the driven level in the same call, but of course that's outside the scope of this PR.

At a glance, the only thought I had about the specifics of the API is that perhaps it's preferable to have high/low passed as a parameter rather than having two separate functions to call. Then again, perhaps it isn't. Maybe both. Both would be consistent with the fact that we have both gpio_write() and gpio_set()/gpio_clear().

I agree that from an API point of view this would be most consistent. However, we would have to touch a few GPIO implementations for that.

Is it that bad?

Most of them accomplish it by calling gpio_write() prior to ever calling gpio_init() which is an undocumented feature common to most platforms except notably efm32 as referenced in OP.

This actually goes against documentation, periph "handles" are only allowed to be used after they have been init()ed correctly. That's our shortcut to not have to check the validity of e.g., a gpio_t parameter, on every set or clear.

This actually goes against documentation, periph "handles" are only allowed to be used after they have been init()ed correctly. That's our shortcut to not have to check the validity of e.g., a gpio_t parameter, on every set or clear.

@kaspar030 should we add the contested tag to this one then?

@kaspar030 should we add the contested tag to this one then?

no, that was just for the workaround mentioned (first gpio_set(), then gpio_init()`).

I'm not happy with adding and calling these gpio_init_low/high(), as this is for configuring output. That's somewhat confusing, IMO. We'd uglify the API so we don't have to touch the implementations.

Would an alternative be wrapping the actual implementations?
Like, renaming the actual per-cpu gpio_init() to gpio_init_sth(), and then using this PR's gpio_init_high/low as a wrapper, but changed to use additional enum values?

I'm not happy with adding and calling these gpio_init_low/high(), as this is for configuring output. That's somewhat confusing, IMO.

I'm trying but I'm struggling to understand which aspect you object to. Are you saying you'd be happier with gpio_init(..., GPIO_OUT_HIGH)?

As a hardware designer I object to an API that permits telling the hardware to become an output without simultaneously telling it which level to output. It's the very definition of undefined behavior.

Perhaps we would be better off with separate functions to init GPIOs as input versus output? After all from a hardware perspective it is fundamentally different. Inputs need pull resistors specified whereas outputs need the driven level specified.

I'm trying but I'm struggling to understand which aspect you object to. Are you saying you'd be happier with gpio_init(..., GPIO_OUT_HIGH)?

yes, I mean only from an API perspective.

As a hardware designer I object to an API that permits telling the hardware to become an output without simultaneously telling it which level to output. It's the very definition of undefined behavior.

👍

GPIO_OUT could be an alias for GPIO_OUT_LOW. That shouldn't be too hard to fix / confirm in the existing implementations?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

What was missing here to get this in?

ping @maribu @benpicco @kaspar030

What was missing here to get this in?

I was objecting the API clutter:

• gpio_init(..., mode) initializing input, output not touching state
• `gpio_init_low(...) initializing output, setting to low
• `gpio_init_high(...) initializing output, setting to high

vs.

• gpio_init(..., mode) initializing input, output not touching state
• gpio_init(..., GPIO_OUT) initializing input, output not touching state
• `gpio_init(...., GPIO_OUT_LOW) initializing output, setting to low
• `gpio_init(..., GPIO_OUT_HIGH) initializing output, setting to high

gpio_init_low() kinda implies the initializiation of an output, but just kinda.

gpio_init_out_low() and gpio_init_out_high() would be a weird compromise, but why no gpio_init_in/out/in_pd/out_pd/...()?
Either the API is using a mode parameter, or distinct functions. Both are just inconsistend.
And I'd object changing towards non-overloaded init(). Too much change for too little benefit.

Note that gpio_ll would also scratch this itch and others ;-)

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.