Shelly: fix PV energy for devices with reverse power measurement enabled - #32550
Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Fetching the reverse flag during
newGen2currently fails the entire constructor if*GetConfigis unsupported or returns an error; consider treating this as an optional capability (e.g., log and default tofalse) so older or slightly incompatible devices don’t prevent meter initialization. - The
Gen2Configstruct relies on default JSON field naming forReverse, but the Shelly API uses a lowercasereversefield per the description; it would be safer to add an explicit struct tag (e.g.,Reverse booljson:"reverse"``) to ensure the flag is actually populated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Fetching the reverse flag during `newGen2` currently fails the entire constructor if `*GetConfig` is unsupported or returns an error; consider treating this as an optional capability (e.g., log and default to `false`) so older or slightly incompatible devices don’t prevent meter initialization.
- The `Gen2Config` struct relies on default JSON field naming for `Reverse`, but the Shelly API uses a lowercase `reverse` field per the description; it would be safer to add an explicit struct tag (e.g., `Reverse bool `json:"reverse"``) to ensure the flag is actually populated.
## Individual Comments
### Comment 1
<location path="meter/shelly/gen2.go" line_range="160-162" />
<code_context>
+ cfgChannel = c.switchchannel
+ }
+
+ if cfgMethod != "" {
+ var cfg Gen2Config
+ if err := c.execCmd(cfgChannel, cfgMethod, &cfg); err != nil {
+ return nil, err
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider not failing construction if the config endpoint is unavailable
Any error from `execCmd` when fetching config (missing `*GetConfig` endpoint, transient network issues, permission issues) currently causes `newGen2` to fail, making config access a hard requirement. If the reverse-measurement flag is only an enhancement, consider logging the error and defaulting to `reversed = false` so meters without config support (or with temporary failures) still work, preserving backward compatibility while enabling the new behavior when available.
</issue_to_address>
### Comment 2
<location path="meter/shelly/gen2.go" line_range="153-154" />
<code_context>
+ switch {
+ case c.hasEM1Endpoint():
+ cfgMethod = "EM1.GetConfig"
+ case c.hasMethod("PM1.GetStatus"):
+ cfgMethod = "PM1.GetConfig"
+ case c.hasMethod("Switch.GetStatus"):
+ cfgMethod = "Switch.GetConfig"
</code_context>
<issue_to_address>
**question (bug_risk):** Align PM1 capability detection with the actual config method
Here you probe for `PM1.GetStatus` but then call `PM1.GetConfig`. This only works if `PM1.GetStatus` always guarantees `PM1.GetConfig` exists. If some firmwares expose status but not config (or under a different name), `newGen2` will fail when reading config. Consider either checking for `PM1.GetConfig` directly (if possible) or explicitly handling a missing config method as non-fatal (e.g., by catching and interpreting the specific error).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This comment has been minimized.
This comment has been minimized.
Contributor
|
✅ Build finished.
|
andig
force-pushed
the
fix/shelly-reverse-measurement
branch
from
August 7, 2026 16:40
d4037c8 to
6154fcf
Compare
This comment has been minimized.
This comment has been minimized.
Member
Author
|
@Hofyyy das klappt aus Sicherheitsgründen nur für maintainer. |
Contributor
|
Die Änderung sieht für mich auf jeden Fall sauber aus. Flucht nach vorne, in dem wir mehr Features der Shellys abfragen. würde mich wundern, wenn da nicht einiges mit erschagen wird. |
Contributor
|
✅ Build finished.
|
This comment has been minimized.
This comment has been minimized.
Contributor
|
✅ Opened backport pull request on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #32213
#31957 unconditionally swaps the energy registers for
usage: pv, assuming production is measured in the return direction. When the device's own "Reverse power measurement" setting is enabled, the device already swaps the direction itself, so evcc's swap pointsTotalEnergyat the frozenret_aenergyregister and PV energy stops recording.Read the device-side
reverseflag once at startup (EM1.GetConfig/PM1.GetConfig/Switch.GetConfig, matching the status endpoint priority — changing it requires a device restart, so it is static) and only swap the registers when it is off. Same for signed (gen3+) PV power, which is only negated when the device does not already reverse it. Gen1 devices have no such setting.🤖 Generated with Claude Code