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 1 issue, and left some high level feedback:
- In
publishChargeProgressyou unconditionally calllp.chargeEnergy.AddEnergy(...); consider guarding this with a nil check so that loadpoints without achargeMeter(and thus without a collector) don't panic. - Creation of the metrics collector in
NewLoadpointFromConfigcurrently aborts loadpoint initialization on error; if metrics are non-critical, consider logging the error and continuing so that a metrics failure does not prevent charger operation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `publishChargeProgress` you unconditionally call `lp.chargeEnergy.AddEnergy(...)`; consider guarding this with a nil check so that loadpoints without a `chargeMeter` (and thus without a collector) don't panic.
- Creation of the metrics collector in `NewLoadpointFromConfig` currently aborts loadpoint initialization on error; if metrics are non-critical, consider logging the error and continuing so that a metrics failure does not prevent charger operation.
## Individual Comments
### Comment 1
<location path="core/loadpoint.go" line_range="1745-1754" />
<code_context>
lp.publish(keys.ChargedEnergy, lp.GetChargedEnergy())
lp.publish(keys.ChargeDuration, lp.chargeDuration)
+
+ // update energy, prefer totals
+ var importTotal *float64
if api.HasCap[api.MeterEnergy](lp.chargeMeter) {
- lp.publish(keys.ChargeTotalImport, lp.chargeMeterTotal())
+ if f := lp.chargeMeterTotal(); f > 0 {
+ lp.publish(keys.ChargeTotalImport, f)
+ importTotal = &f
+ }
}
+
+ lp.chargeEnergy.AddEnergy(importTotal, nil, lp.chargePower)
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard `lp.chargeEnergy` before calling `AddEnergy` to avoid nil dereference when no charge meter is configured.
`lp.chargeEnergy` is only initialized when `lp.chargeMeter != nil` in `NewLoadpointFromConfig`. For loadpoints without a `chargeMeter`, `lp.chargeEnergy` remains nil, but `publishChargeProgress` still calls `lp.chargeEnergy.AddEnergy(...)`, causing a potential panic. Please add a nil check (or early return) before calling `AddEnergy`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| // add collector | ||
| if lp.chargeMeter != nil { | ||
| me, err := metrics.NewCollector(metrics.Charge, lp.GetTitle()) |
There was a problem hiding this comment.
Works for me. We have to keep in mind, that title is not technically required. When creating a loadpoint via UI you are forced to pick one and I guess very few people dont have loadpoints without title. But we should check that it's not empty.
Titles also dont have to be unique. But I think this is someone we could or even should enforce. At least for creating new loadpoints or changing names. Dont see this as a blocker, it's more an additional step we should take to avoid confusion.
There was a problem hiding this comment.
From the failing integration tests: uniqueness error
There was a problem hiding this comment.
From the failing integration tests: uniqueness error
naltatis: wo kommen die her, also was sorgt dafür dass da neue metrics angelegt werden?
There was a problem hiding this comment.
Now creating the collector before loadpoint and passing it. Similar to settings and logs. This way we've access to proper names (lp-1...n for yaml and db:__ for db lp's). Solves the uniqueness problem and provides stable identifiers for later referencing.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
| PV = "pv" | ||
| Home = "home" // meter and group (virtual measurement) | ||
| Battery = "battery" | ||
| Charge = "charge" |
There was a problem hiding this comment.
I'd prefer loadpoint since this is what we're capturing here.
|
|
||
| settings := coresettings.NewConfigSettingsAdapter(log, &conf) | ||
|
|
||
| collector, err := metrics.NewCollector(metrics.Charge, cc.Name) |
There was a problem hiding this comment.
We have 3 places now where we need to setup the collector which doesn't feel good.
There was a problem hiding this comment.
Only other option would be to pass name into loadpoint.
This is symmetrical to settings/logger. We might be able to refactor/extract this lp init code to make it more dry (static & dynamic). Not sure of its worth the indirection though ...
There was a problem hiding this comment.
just pushed a version with extracted lp init. now we only have one metrics.NewCollector call
3 participants
A follow-up to #23185, this PR will provide the basis for tracking heatpump consumption during non-boost times.
@naltatis we need to decide under which group and metric name we'd want to do this?
Next step
Per loadpoint chart. Currently grouped.