-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Tasmota: Eable SML-IR reader phase readings #26920
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- In
Connection.Currents, the SML branch returnsVoltage_L1/2/3instead ofCurrent_L1/2/3, which looks like a copy-paste bug and will surface as incorrect current readings when SML is available. - The
decorateTasmotaswitch has become quite complex with many condition combinations for currents/voltages/powers; consider refactoring to build up the embedded interfaces incrementally to avoid a combinatorial explosion of cases and make future extensions safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Connection.Currents`, the SML branch returns `Voltage_L1/2/3` instead of `Current_L1/2/3`, which looks like a copy-paste bug and will surface as incorrect current readings when SML is available.
- The `decorateTasmota` switch has become quite complex with many condition combinations for currents/voltages/powers; consider refactoring to build up the embedded interfaces incrementally to avoid a combinatorial explosion of cases and make future extensions safer.
## Individual Comments
### Comment 1
<location> `meter/tasmota/connection.go:220-226` </location>
<code_context>
return res.StatusSNS.Energy.Total + res.StatusSNS.SML.TotalIn, err
}
-// Currents implements the api.PhaseCurrents interface
-func (c *Connection) Currents() (float64, float64, float64, error) {
- return c.getPhaseValues(func(s StatusSNSResponse) Channels {
- return s.StatusSNS.Energy.Current
- })
+// Powers implements the api.PhasePowers interface
+func (c *Connection) Powers() (float64, float64, float64, error) {
+ s, err := c.statusSnsG.Get()
+ if err != nil {
+ return 0, 0, 0, err
+ }
+ if s.StatusSNS.SML.TotalIn != 0 {
+ // SML powers available
+ return float64(s.StatusSNS.SML.Power_L1), float64(s.StatusSNS.SML.Power_L2), float64(s.StatusSNS.SML.Power_L3), nil
</code_context>
<issue_to_address>
**issue (bug_risk):** SML branch of Currents returns voltage fields instead of current fields
In the SML branch, the code currently returns `Voltage_L1/L2/L3` instead of `Current_L1/L2/L3`, which will produce wrong current values whenever SML data is used. This appears to be a copy/paste mistake and should be updated to:
```go
if s.StatusSNS.SML.TotalIn != 0 {
// SML currents available
return s.StatusSNS.SML.Current_L1, s.StatusSNS.SML.Current_L2, s.StatusSNS.SML.Current_L3, nil
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
meter/tasmota.go
Outdated
| if usage != "grid" && len(channels) == 3 { | ||
| currents = c.currents | ||
| var currents, voltages, powers func() (float64, float64, float64, error) | ||
| if usage != "grid" && len(channels) == 3 || usage == "grid" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is usage necessary here? shouldn't this only check if the measurements are available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are different device configurations possible with tasmota software. In case a tasmota device was configured to only measure a single phase / one channel for pv, battery, charger usage, I wanted to skip the phase api decoration.
Only in case the meter is configured to meter 3 channels/phases the phase apis should provide information.
|
@andig imho the PR should be ready for merge. |
Fix #26857
Eable SML-IR reader phase readings.
Refer to #26857.