GPIO: Add support for active low#28187
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The change from always using
WithPullUpon input lines to conditionally usingWithPullUp/WithPullDownfor all lines (including outputs) alters the previous default behavior; consider preserving the old default for existing configs (e.g., only changing bias whenActiveLowis explicitly set) to avoid unexpected behavior changes. - Coupling the pull-up/pull-down configuration directly to the
ActiveLowflag makes it hard to support cases where active-low lines should still be left floating or use a different bias; consider separatingActiveLowfrom a distinct optional bias/pull configuration to keep these concerns independent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change from always using `WithPullUp` on input lines to conditionally using `WithPullUp`/`WithPullDown` for all lines (including outputs) alters the previous default behavior; consider preserving the old default for existing configs (e.g., only changing bias when `ActiveLow` is explicitly set) to avoid unexpected behavior changes.
- Coupling the pull-up/pull-down configuration directly to the `ActiveLow` flag makes it hard to support cases where active-low lines should still be left floating or use a different bias; consider separating `ActiveLow` from a distinct optional bias/pull configuration to keep these concerns independent.
## Individual Comments
### Comment 1
<location path="plugin/gpio_linux.go" line_range="42-43" />
<code_context>
var opts []gpiocdev.LineReqOption
switch cc.Function {
case GpioTypeRead:
- opts = append(opts, gpiocdev.AsInput, gpiocdev.WithPullUp)
+ opts = append(opts, gpiocdev.AsInput)
case GpioTypeWrite:
opts = append(opts, gpiocdev.AsOutput(0))
</code_context>
<issue_to_address>
**issue (bug_risk):** Re-evaluate change from default pull-up to pull-down for input lines.
This change effectively flips the default from pull-up to pull-down because `ActiveLow` defaults to false and the later `if cc.ActiveLow { ... } else { ... }` block will apply `WithPullDown`. That silently alters existing configurations’ electrical behavior. If you only intend to change active-low handling, consider separating pull configuration from `ActiveLow` (e.g., explicit pull option or preserving the old pull-up default when `ActiveLow` is unset).
</issue_to_address>
### Comment 2
<location path="plugin/gpio_linux.go" line_range="50-51" />
<code_context>
return nil, fmt.Errorf("invalid type: %s", cc.Function)
}
+ if cc.ActiveLow {
+ opts = append(opts, gpiocdev.WithPullUp, gpiocdev.AsActiveLow)
+ } else {
+ opts = append(opts, gpiocdev.WithPullDown)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid applying pull-up/pull-down options to output lines unless explicitly intended.
This block runs for both read and write modes, so `WithPullUp`/`WithPullDown` will also be applied when the line is configured as `AsOutput(0)`. On many platforms, pull configuration is only valid for inputs and can lead to undefined behaviour on outputs. Consider only appending pull options when `cc.Function == GpioTypeRead`, and gate `AsActiveLow` for writes on a confirmed, driver-specific expectation.
Suggested implementation:
```golang
switch cc.Function {
case GpioTypeRead:
opts = append(opts, gpiocdev.AsInput)
case GpioTypeWrite:
opts = append(opts, gpiocdev.AsOutput(0))
default:
return nil, fmt.Errorf("invalid type: %s", cc.Function)
}
// Only configure pulls (and active-low) for input lines. Many platforms
// either ignore or undefinedly handle pull configuration on outputs.
if cc.Function == GpioTypeRead {
if cc.ActiveLow {
opts = append(opts, gpiocdev.WithPullUp, gpiocdev.AsActiveLow)
} else {
opts = append(opts, gpiocdev.WithPullDown)
}
}
line, err := gpiocdev.RequestLine(cc.Chip, cc.Pin, opts...)
```
If your driver or hardware requires active-low semantics for output lines as well, you can extend the `GpioTypeWrite` case to conditionally append `gpiocdev.AsActiveLow` there, behind a clearly documented, driver-specific flag (e.g. a new config field `AllowActiveLowOutput`), instead of reusing `ActiveLow` for both directions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if cc.ActiveLow { | ||
| opts = append(opts, gpiocdev.WithPullUp, gpiocdev.AsActiveLow) |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid applying pull-up/pull-down options to output lines unless explicitly intended.
This block runs for both read and write modes, so WithPullUp/WithPullDown will also be applied when the line is configured as AsOutput(0). On many platforms, pull configuration is only valid for inputs and can lead to undefined behaviour on outputs. Consider only appending pull options when cc.Function == GpioTypeRead, and gate AsActiveLow for writes on a confirmed, driver-specific expectation.
Suggested implementation:
switch cc.Function {
case GpioTypeRead:
opts = append(opts, gpiocdev.AsInput)
case GpioTypeWrite:
opts = append(opts, gpiocdev.AsOutput(0))
default:
return nil, fmt.Errorf("invalid type: %s", cc.Function)
}
// Only configure pulls (and active-low) for input lines. Many platforms
// either ignore or undefinedly handle pull configuration on outputs.
if cc.Function == GpioTypeRead {
if cc.ActiveLow {
opts = append(opts, gpiocdev.WithPullUp, gpiocdev.AsActiveLow)
} else {
opts = append(opts, gpiocdev.WithPullDown)
}
}
line, err := gpiocdev.RequestLine(cc.Chip, cc.Pin, opts...)If your driver or hardware requires active-low semantics for output lines as well, you can extend the GpioTypeWrite case to conditionally append gpiocdev.AsActiveLow there, behind a clearly documented, driver-specific flag (e.g. a new config field AllowActiveLowOutput), instead of reusing ActiveLow for both directions.
|
Added support for bias. The clanker was kinda right that the bias should have been an explicit choice from the start. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The default bias for input pins has changed from an implicit pull-up to
BiasAsIs, which may break existing configurations that relied on the previous behavior; consider preserving the old default whenbiasis omitted or explicitly documenting/migrating this change in configuration handling. - When using
ActiveLowwith output lines, double-check whether the initial output level passed togpiocdev.AsOutput(0)still matches the expected logical default (since active-low inverts semantics), and consider making the initial logical level explicit in the configuration if needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default bias for input pins has changed from an implicit pull-up to `BiasAsIs`, which may break existing configurations that relied on the previous behavior; consider preserving the old default when `bias` is omitted or explicitly documenting/migrating this change in configuration handling.
- When using `ActiveLow` with output lines, double-check whether the initial output level passed to `gpiocdev.AsOutput(0)` still matches the expected logical default (since active-low inverts semantics), and consider making the initial logical level explicit in the configuration if needed.
## Individual Comments
### Comment 1
<location path="plugin/gpio_linux.go" line_range="52-61" />
<code_context>
return nil, fmt.Errorf("invalid type: %s", cc.Function)
}
+ switch cc.Bias {
+ case GpioBiasAsIs:
+ opts = append(opts, gpiocdev.WithBiasAsIs)
+ case GpioBiasDisabled:
+ opts = append(opts, gpiocdev.WithBiasDisabled)
+ case GpioBiasPullUp:
+ opts = append(opts, gpiocdev.WithPullUp)
+ case GpioBiasPullDown:
+ opts = append(opts, gpiocdev.WithPullDown)
+ default:
+ return nil, fmt.Errorf("invalid bias: %s", cc.Bias)
+ }
+
</code_context>
<issue_to_address>
**question (bug_risk):** Consider whether bias options should be applied for output lines as well as inputs.
The shared `opts` slice applies the selected bias to both `GpioTypeRead` and `GpioTypeWrite`. Depending on `gpiocdev` and the hardware, some biases may be invalid or ignored for outputs. Please confirm whether biases are intended for outputs as well; if not, consider applying them only for `GpioTypeRead` or constraining allowed bias values per function type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| switch cc.Bias { | ||
| case GpioBiasAsIs: | ||
| opts = append(opts, gpiocdev.WithBiasAsIs) | ||
| case GpioBiasDisabled: | ||
| opts = append(opts, gpiocdev.WithBiasDisabled) | ||
| case GpioBiasPullUp: | ||
| opts = append(opts, gpiocdev.WithPullUp) | ||
| case GpioBiasPullDown: | ||
| opts = append(opts, gpiocdev.WithPullDown) | ||
| default: |
There was a problem hiding this comment.
question (bug_risk): Consider whether bias options should be applied for output lines as well as inputs.
The shared opts slice applies the selected bias to both GpioTypeRead and GpioTypeWrite. Depending on gpiocdev and the hardware, some biases may be invalid or ignored for outputs. Please confirm whether biases are intended for outputs as well; if not, consider applying them only for GpioTypeRead or constraining allowed bias values per function type.
|
Hi, any update on this? It's ready to be merged, or I can rebase the branch on top of master, if you prefer to have linear history. |
|
ActiveLow with enabled pull-up resistor is already the default? |
|
In software, in my version it is not, you'd need to set it explicitly. The default I set is „do not change”, so technically this depends on board drivers and/or devicetree, so it might be, it might not be. But if you have correctly designed board (or HAT/shield/whatever addon), you'll have hardware pull-up resistor there, so it's kinda not that important. Only if you do stuff on a breadboard, then it will matter. |
|
I've reread the question. Was it: „does It's also kinda orthogonal with whether the pin is input or output, because there are open-source or open-drain configurations where connecting internal pull-up/-down makes sense (even if generally not a good idea), contrary to what clanker says. |
|
Sorry, I still see your point here. Current GPI logic is true on short to GND with any internal pull-up enabled. Isn't it? |
There was a problem hiding this comment.
Pull request overview
Adds configurability to GPIO plugin behavior to support “active low” wiring (logical inversion) on Linux, aligning with common hardware setups.
Changes:
- Add
activeLowoption to invert GPIO line logic viagpiocdev.AsActiveLow. - Introduce configurable GPIO line bias (
as-is,disabled,pull-up,pull-down) via a newGpioBiasenum. - Update Linux GPIO line request options accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugin/gpio_linux.go | Adds activeLow and bias config handling and applies corresponding gpiocdev request options. |
| plugin/gpiobias.go | Defines the GpioBias enum and go:generate directive. |
| plugin/gpiobias_enumer.go | Generated string/text marshal/unmarshal helpers for GpioBias. |
| var opts []gpiocdev.LineReqOption | ||
| switch cc.Function { | ||
| case GpioTypeRead: | ||
| opts = append(opts, gpiocdev.AsInput, gpiocdev.WithPullUp) | ||
| opts = append(opts, gpiocdev.AsInput) | ||
| case GpioTypeWrite: |
There was a problem hiding this comment.
This changes the default input configuration: previously read always requested the line with an internal pull-up, but now the default is Bias: as-is (no pull-up unless configured). That’s a behavior/backwards-compat change and can lead to floating inputs for common “switch to GND” wiring (especially when paired with activeLow). Consider keeping the previous default (pull-up for read) and only overriding it when bias is explicitly configured (or at least documenting this breaking behavior).
No, it's not. If you short to ground, you get 0, if you pull high or leave floating, you get 1. Sometimes this is what you want, more often not (for most cases you want to at least default to 0 either way), there are good reasons to design electronics both ways, so we need configuration knobs to control both active low/high and bias, independently. I've just double-checked, since you've asked, 0.303.2 installed from APT repository, on Orange Pi PC+ (sun8i-h3, Armbian 26.2.0-trunk.626 bookworm, 6.12.76-current-sunxi). It's actually pretty poor default, because if you configure §14a/§9 energy limit without connecting anything actively pulling the pin to ground, then the restriction starts immediately after evcc starts and won't ever end. |
|
Hi, is there anything for me to do in this PR? A week has passed, and I don't want it to go stale. As far as I'm concerned, it should be ready to merge. |
|
No answer to the review comment #28187 (comment) |
|
As I've already explained in #28187 (comment), this is very poor default, because it causes the signal to be 1 by default, unless specifically driven, which is rarely what you'd want from such a signal. (Was it a mistake made by AI during last driver rewrite?). Also, I'm surprised I need to answer to a clanker which according to it's TOS is to be used for entertainment purposes only: https://www.microsoft.com/en-us/microsoft-copilot/for-individuals/termsofuse |
|
It is a breaking change. |
I understand it technically is, but I argue no-one is intentionally using this behaviour. For correctly designed electronics, it doesn't matter, because there will be hardware pull-up/down there. It will only affect people who kinda don't know what they're doing, like, they unplug the connector or daughterboard and oops, they just enabled something (like 4,2 kW restriction on main circuit, and now why this thing doesn't want to fast charge the car?). It makes for a poor UX. Can I ask on slack if there's anyone that relies on this particular behaviour? |
|
This is the intended behavior. Again: This is a breaking change that needs to be adressed. |
|
Okay, let me change this and test, will push updated version shortly. |
“Active low” pins have inverted state, i.e. they're “true” or 1 when they're pulled to the ground and “false”/0 when left floating around VCC. This is very common configuration when building interfaces with semiconductors (like optocouplers), less so with relays where the polarity mostly doesn't matter. With varying polarization it is best to leave the choice of bias to the end user. Signed-off-by: Wojciech Porczyk <wojciech.porczyk@connectpoint.pl>
|
How shall we continue here? |
It's mostly up to you guys, I believe I did what @premultiply asked for, and documetation in evcc-io/docs#1016 is also up to date, so as far as I'm concerned this PR can be merged right away, or if you prefer I can rebase on top of current master if you'd like to have linear history. Either way works for me. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Using -1 as a magic value for
GpioBiasin the config struct and switch makes the API a bit unclear; consider adding an explicitGpioBiasUnspecifiedenum value (or using a pointer/omitting the field) instead of relying on a sentinel integer. - The default pull-up behavior for
GpioTypeReadis now split between the function switch and the bias switch, which makes the effective configuration slightly harder to follow; consider consolidating the default-bias logic into a helper or a single decision point so precedence is clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using -1 as a magic value for `GpioBias` in the config struct and switch makes the API a bit unclear; consider adding an explicit `GpioBiasUnspecified` enum value (or using a pointer/omitting the field) instead of relying on a sentinel integer.
- The default pull-up behavior for `GpioTypeRead` is now split between the function switch and the bias switch, which makes the effective configuration slightly harder to follow; consider consolidating the default-bias logic into a helper or a single decision point so precedence is clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hi, guys, to quote myself from the last comment:
Do you want me to do something in this PR, or you're just waiting for something else that is blocking? |
|
Hello, this is weekly comment so the PR does not go stale. I'm sorry for the noise. As always, if you need something from my side, just ping me here. |
“Active low” pins have inverted state, i.e. they're “true” or 1 when they're pulled to the ground and “false”/0 when left floating around VCC. This is very common configuration when building interfaces with semiconductors (like optocouplers), less so with relays where the polarity mostly doesn't matter.
This is follow up to #27815. Tested on Orange Pi PC+, Armbian 26.02.