Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider validating
usageagainst the allowed values ("grid", "pv", "battery") inNewEcoFlowStreamFromConfig(and/orNewEcoFlowStream) so invalid values fail fast instead of propagating togetDataandCurrentPower. - In
getData, it might be safer to return an error for unsupportedusagevalues instead of callingGetDeviceParameterswith an emptyparamsslice, which would make issues easier to diagnose. - The string parameter names used in
paramsandCurrentPower(e.g. "powGetSysGrid", "powGetPvSum", "powGetBpCms", "cmsBattSoc") could be defined as shared constants to avoid typos and keep meter and battery logic in sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating `usage` against the allowed values ("grid", "pv", "battery") in `NewEcoFlowStreamFromConfig` (and/or `NewEcoFlowStream`) so invalid values fail fast instead of propagating to `getData` and `CurrentPower`.
- In `getData`, it might be safer to return an error for unsupported `usage` values instead of calling `GetDeviceParameters` with an empty `params` slice, which would make issues easier to diagnose.
- The string parameter names used in `params` and `CurrentPower` (e.g. "powGetSysGrid", "powGetPvSum", "powGetBpCms", "cmsBattSoc") could be defined as shared constants to avoid typos and keep meter and battery logic in sync.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/ecoflow-stream.yaml" line_range="45" />
<code_context>
+ generic: Serial Number
+ help:
+ en: Serial number of your main EcoFlow Stream device (e.g., BK51ZE1B2H4H0029)
+ de: Seriennummer eines Ihres EcoFlow Stream Hauptgerätes (z.B. BK51ZE1B2H4H0029)
+ - name: cache
+ default: 30s
</code_context>
<issue_to_address>
**suggestion (typo):** Fix minor German wording redundancy in the serial number help text.
`Seriennummer eines Ihres` is redundant German. Please change to `Seriennummer Ihres EcoFlow Stream Hauptgerätes` for natural wording.
```suggestion
de: Seriennummer Ihres EcoFlow Stream Hauptgerätes (z.B. BK51ZE1B2H4H0029)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@andig I attempted fixes, please review again. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new Stream meter reuses
extractFloatfrom the PowerOcean implementation implicitly; consider extracting this helper into a shared EcoFlow-level utility to avoid hidden inter-meter dependencies and make reuse explicit. - Both PowerOcean and Stream now store a
context.ContextfromNew...FromConfigand pass it directly toGetDeviceParameters; if that context is long-lived, you may want to introduce per-call timeouts (like the previousWithTimeout) to avoid hanging API calls. - In
templates/definition/meter/ecoflow-powerocean.yamlthe English help text forserialcontains a typo ("Seriel number"); aligning this with the terminology used in the code and other texts would avoid user confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Stream meter reuses `extractFloat` from the PowerOcean implementation implicitly; consider extracting this helper into a shared EcoFlow-level utility to avoid hidden inter-meter dependencies and make reuse explicit.
- Both PowerOcean and Stream now store a `context.Context` from `New...FromConfig` and pass it directly to `GetDeviceParameters`; if that context is long-lived, you may want to introduce per-call timeouts (like the previous `WithTimeout`) to avoid hanging API calls.
- In `templates/definition/meter/ecoflow-powerocean.yaml` the English help text for `serial` contains a typo ("Seriel number"); aligning this with the terminology used in the code and other texts would avoid user confusion.
## Individual Comments
### Comment 1
<location path="meter/ecoflow-powerocean.go" line_range="84-87" />
<code_context>
// getData retrieves device parameters from EcoFlow API
func (m *EcoFlowPowerOcean) getData() (*ecoflow.GetCmdResponse, error) {
- ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
- defer cancel()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Removing the per-call timeout may cause long-hanging API requests.
`getData` previously wrapped calls in `context.WithTimeout(..., 30*time.Second)`, but now uses `m.ctx` directly. If `m.ctx` isn’t time-bounded, `GetDeviceParameters` can block indefinitely on network issues. Please reintroduce a per-call timeout (e.g., wrapping `m.ctx` with `WithTimeout`) to preserve the previous behavior.
</issue_to_address>
### Comment 2
<location path="templates/definition/meter/ecoflow-powerocean.yaml" line_range="43-45" />
<code_context>
help:
- en: Device ID of your EcoFlow PowerOcean system (e.g., HJ31ZD1AZH5G0342)
- de: Geräte-ID Ihres EcoFlow PowerOcean Systems (z.B. HJ31ZD1AZH5G0342)
+ en: Seriel number of your EcoFlow PowerOcean system (e.g., HJ31ZD1AZH5G0342)
+ de: Seriennummer Ihres EcoFlow PowerOcean Systems (z.B. HJ31ZD1AZH5G0342)
- name: cache
</code_context>
<issue_to_address>
**nitpick (typo):** Fix typo in the English help text for the serial number.
Please change "Seriel" to "Serial" in the English help text.
```suggestion
help:
en: Serial number of your EcoFlow PowerOcean system (e.g., HJ31ZD1AZH5G0342)
de: Seriennummer Ihres EcoFlow PowerOcean Systems (z.B. HJ31ZD1AZH5G0342)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Since
extractFloatis now used by both the PowerOcean and Stream meters, consider moving it into a small shared helper in this package (e.g.ecoflow_helpers.go) to make the cross‑meter dependency explicit and avoid future circular or accidental coupling. - For consistency and to reduce duplication,
EcoFlowStreamBattery.Soccould reuseextractFloatinstead of manually accessing and castingres.Data["cmsBattSoc"]. - To make interface conformance explicit and catch breakages at compile time, consider adding a
var _ api.Battery = (*EcoFlowStreamBattery)(nil)assertion similar to the existingapi.Meterassertions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `extractFloat` is now used by both the PowerOcean and Stream meters, consider moving it into a small shared helper in this package (e.g. `ecoflow_helpers.go`) to make the cross‑meter dependency explicit and avoid future circular or accidental coupling.
- For consistency and to reduce duplication, `EcoFlowStreamBattery.Soc` could reuse `extractFloat` instead of manually accessing and casting `res.Data["cmsBattSoc"]`.
- To make interface conformance explicit and catch breakages at compile time, consider adding a `var _ api.Battery = (*EcoFlowStreamBattery)(nil)` assertion similar to the existing `api.Meter` assertions.
## Individual Comments
### Comment 1
<location path="meter/ecoflow-stream.go" line_range="53" />
<code_context>
+ if cc.Serial == "" {
+ return nil, errors.New("missing serial number")
}
if cc.Usage == "" {
return nil, errors.New("missing usage")
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Validate `usage` against the supported values instead of only checking for non-empty strings.
`NewEcoFlowStreamFromConfig` only checks that `Usage` is non-empty, but `getData`/`CurrentPower` assume it is one of `grid`, `pv`, or `battery`. Any other value leads to a `GetDeviceParameters` call with `nil` params and only later returns "invalid usage". Please validate `Usage` against the allowed set at construction (or add a `default` case in `getData` that returns an error) so invalid values fail fast and avoid undefined API calls.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@andig can you review again? |
Co-authored-by: andig <[email protected]>
|
Looking at this for the umpteenth time: these devices are absolutely identical, only difference is the actual measurements. Why not just combine the implementations and pass a list or map of values to use for specific purposes? There's really no reason to duplicate all this code. Measurements could even come from the template... |
|
Hey @andig, you're absolutely right about the duplication. Collapsing the two meters was pretty easy. Variable count exploded a little bit. Have a look at the solution. Did I get what you meant by "coming from the template"? |
Head branch was pushed to by a user without write access
|
@andig The linter failed and blocked the merge. I fixed the issue. Can you approve again? |
This PR implements a EcoFlow Stream meter and template. It was heavily derived from the EcoFlow PowerOcean implementation so I didn't need to use any AI code generators.
I encountered the following problems/questions:
extractFloatfunction of the PowerOcean meter, since copying it with the rest of the meter file resulted in a duplicate function issue. Is this dependency a problem? Should the function be either duplicated under a different name or extracted into a shared space?I did test it locally with the simulator and a configured meter for
usage: grid,pvas well asbattery. I can provide my credentials for testing over a private channel if needed.