Conversation
This reverts commit c5ca432.
![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 left some high level feedback:
- The plugin keeps a single
totalvalue for allparams, so if multiple parameters use this plugin they will interfere with each other; consider tracking deltas per parameter (e.g., a map keyed byparam) instead of a single shared accumulator. - The
deltaPluginstate (total) is mutated without any synchronization; if setters can be called concurrently, you may need to protect this with a mutex or otherwise enforce single-threaded access. - The decoded
pipeline.SettingsinNewDeltaFromConfigare currently unused; either apply them (e.g., for behavior configuration) or remove them from the config struct to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The plugin keeps a single `total` value for all `param`s, so if multiple parameters use this plugin they will interfere with each other; consider tracking deltas per parameter (e.g., a map keyed by `param`) instead of a single shared accumulator.
- The `deltaPlugin` state (`total`) is mutated without any synchronization; if setters can be called concurrently, you may need to protect this with a mutex or otherwise enforce single-threaded access.
- The decoded `pipeline.Settings` in `NewDeltaFromConfig` are currently unused; either apply them (e.g., for behavior configuration) or remove them from the config struct to avoid confusion.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Heatpump is a slowly self-regulating device. It is not designed to provide power offsets etc to it. See the discussion about Lambda heatpumps etc. The right way to interface heatpumps is SG-Ready or a similar simple digital logic. |
|
@premultiply thanks for the comment, but OT. We already have the ability for setting a power envelope. Let's please not discuss this capability here. Think AC Elwa...
That said, an actual HP can still integrate internally. This PR only ensures that it gets the "measurements" that it expects.
See linked issue. The expectation is apparently not met by Ochsner and we know that Elwa needs specific configuration. Ok? |
|
Elwa is fast, a heatpump is very slow. Yes, might be a bit OT here but I think this plugin is a bit obsolete for that reason? |
|
Ah, thank you for the hit on the head. For heatpump, |
|
Bis jetzt habe ich eine benutzerdefinierte WP mit nur einem Eintrag zum Testen, mit dem zumindest schon einmal das Schreiben von Überschussmeldungen geht, so dass sie bei der WP ankommen: setmaxpower: Ich habe in der Doku gesucht, aber nicht gefunden, wie ich jetzt das delta-plugin hier integrieren kann. |
|
@ganzton du musst deine Posts bitte formatieren. So geht delta: setmaxpower:
source: delta
set:
source: modbus
uri: 192.168.9.88:50284
id: 50
register:
address: 2201 # SUR Überschussleistung Auflösung 1 W
type: writeholding
decode: uint16 |
|
@ganzton ich habe jetzt noch einen getter mit ergänzt gem. #26875 (comment): setmaxpower:
source: delta
get:
source: modbus
uri: 192.168.9.88:50284
id: 50
register:
address: 2012 # Totale Überschussleistung Auflösung 1 W
type: input
decode: uint16
set:
source: modbus
uri: 192.168.9.88:50284
id: 50
register:
address: 2201 # SUR Überschussleistung Auflösung 1 W
type: writeholding
decode: uint16 |
|
Danke - muss ich das delta Plugin noch installieren? Wenn ich die benutzerdefinierte WP einrichten will, kommt |
|
Ja klar, das ist ja noch nicht enthalten. Du musst compilieren. |
|
Oh, oh - komplettes Neuland für mich ... |
|
so, ziemlich steile lernkurve ... |
|
Indem du diesen PR hier auscheckst und compilierst und eben nicht "master". |
|
Danke für den Hinweis. Hab's hinbekommen. Noch 2 Anfragen zum WWWP UI:
|
|
Das wären |
|
Jetzt habe ich eine Rückmeldung vom Ochsner Kundendienst, der den Sinn des Registers 2201 zumindest schlüssig erklärt:
Ich habe noch nachgefragt, ob es nicht doch eine Möglichkeit gibt, den "totaler_berechneter_Überschuss" direkt zu schreiben - geht leider nicht. Die leichten Differenzen bei diesem Wert entstehen wohl durch zeitliche Abläufe intern. Heute bei Sonne hat die PV schon mal das Warmwasser überschuss-geladen. Ich schau mir bald nochmal genauer an, ob die Regelung soweit sinnvoll funktioniert hat. Kannst Du beim delta-plugin oder hinterher bei dem Ochsner-Template dafür sorgen, dass diese 5 sekündige Übermittlung stattfindet? |
|
Sollte gehen, wenn wir das nochmal in einen watchdog verpacken. Schaffst du das selbst? |
|
Wobei ich nicht verstehe, wieso die WP den Wert alle 5s haben will… |
|
Ich probier's mal selbst mit dem Watchdog. 
|
|
Kann ich beim Watchdog auch als reset Wert "kleiner als" eingeben? Wenn ich keinen reset eingebe, dann pendelt evcc immer so mit 10 W rauf und runter, weil die WP diese komische, möglicherweise zeitverzögerte Berechnung macht. Sinnvoll wäre hier vielleicht etwas wie +/-50 W oder so, um den anzuhalten, damit sich die WP beruhigt und so langsam selbst auf 0 W geht. Nee, das muss anders gehen. Denn wenn gerade Überschuss gemeldet wird, der sich aber nicht oder wenig ändert, dann sollen ja weiter Werte, also eben auch einige Nullen gesendet werden. Als Trigger für den Watchdog müsste nicht die Überschussleistung, sondern das Register 2012 "berechnete ..." genommen werden. Wenn die irgendwo zwischen +/- 30 ist, kann der Watchdog aus, bis wieder Ü Energie da ist. |
|
Oder wir schreiben einfach, wie es mit dem "dummen" Stromzähler ja auch wäre, ganz unabhängig von der von der WP berechneten Ü-Leistung einfach strack den Überschuss rein, der da ist? |
|
...oder man lässt die WP einfach laufen und gibt ihr per SG-Ready ein Signal wenn mehr Energie verbraucht werden soll. Die gleiche Diskussion hatten wir bei Lambda schon und dann abgebrochen, da es letztendlich unsinnig ist zwei Regelungen gegeneinander arbeiten zu lassen. |
|
Einen externen Zähler hatte ich ja schon vorgeschlagen. |
|
Könnte man das delta plugin auch mit einem anderen Bezugspunkt nutzen? Ich denke da an den "echten" Zähler, der den Strom von der WP misst (mit dem Ruheleistungsoffset), falls das sinnvoll ist? Dann wäre das Delta klar und entspräche den Werten von dem "dummen" Zähler, oder habe ich hier einen Denkfehler? |
|
Die Regelung der WP arbeitet mit externen Zählerdaten im Prinzip so autonom wie ein Batteriespeicher, nur eben sehr viel langsamer. Auch bei Batteriespeichern ersetzen wir aus gutem Grund nicht die interne Leistungsregelung. Auch hier gibt es bei aktiver Batteriekontrolle nur digitale Modus-Logik: Normal/Hold/Charge. Nirgends wird da kontinuierlich in die Leistungsregelung eingegriffen. |
|
Hm. Das habe ich noch nicht ganz verstanden. Die Regelung ist doch sehr schnell, denn sobald ich den Parameter 2201 Überschussleistung ändere, reagiert die WP sofort auf einzelne Watt. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In both IntSetter and FloatSetter, p.get is dereferenced unconditionally (e.g. p.get.IntGetter) and only checked for nil inside the returned closure, which will panic if Get is not configured; consider guarding the getter construction or handling a nil Get by skipping the read and just using the existing p.total.
- The plugin stores mutable state in p.total, which may be accessed and updated from multiple goroutines via the returned setter functions; if these setters can be called concurrently, consider protecting p.total with synchronization or avoiding shared mutable state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both IntSetter and FloatSetter, p.get is dereferenced unconditionally (e.g. p.get.IntGetter) and only checked for nil inside the returned closure, which will panic if Get is not configured; consider guarding the getter construction or handling a nil Get by skipping the read and just using the existing p.total.
- The plugin stores mutable state in p.total, which may be accessed and updated from multiple goroutines via the returned setter functions; if these setters can be called concurrently, consider protecting p.total with synchronization or avoiding shared mutable state.
## Individual Comments
### Comment 1
<location> `plugin/delta.go:10` </location>
<code_context>
+ "github.com/evcc-io/evcc/util"
+)
+
+type deltaPlugin struct {
+ ctx context.Context
+ total float64
</code_context>
<issue_to_address>
**issue (complexity):** Consider making the plugin stateless and reusing a shared delta-calculation helper so each setter keeps its own local running total instead of using shared struct state.
You can remove most of the complexity without changing behavior by (1) making `total` closure-local instead of struct state and (2) factoring the common logic + fixing the `get` pointer handling.
### 1. Make `total` closure-local (struct becomes stateless)
You don’t need `p.total` at all; keep it inside each setter factory. That removes hidden coupling between different setters.
```go
type deltaPlugin struct {
ctx context.Context
set Config
get *Config
}
func (p *deltaPlugin) IntSetter(param string) (func(int64) error, error) {
set, err := p.set.IntSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (int64, error)
if p.get != nil {
getter, err = p.get.IntGetter(p.ctx)
if err != nil {
return nil, err
}
}
var total float64
return func(val int64) error {
if getter != nil {
current, err := getter()
if err != nil {
return err
}
total = float64(current)
}
delta := float64(val) - total
if err := set(int64(delta)); err != nil {
return err
}
total = float64(val)
return nil
}, nil
}
```
Same idea for `FloatSetter`:
```go
func (p *deltaPlugin) FloatSetter(param string) (func(float64) error, error) {
set, err := p.set.FloatSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (float64, error)
if p.get != nil {
getter, err = p.get.FloatGetter(p.ctx)
if err != nil {
return nil, err
}
}
var total float64
return func(val float64) error {
if getter != nil {
current, err := getter()
if err != nil {
return err
}
total = current
}
delta := val - total
if err := set(delta); err != nil {
return err
}
total = val
return nil
}, nil
}
```
This keeps the “delta relative to last total” behavior but avoids shared mutable state across closures and avoids concurrent access issues.
### 2. Factor common delta logic (optional but reduces duplication)
If you want to go a step further, you can encapsulate the shared pattern and keep the concrete setters very small:
```go
func makeDeltaFloatSetter(
getter func() (float64, error),
setter func(float64) error,
) func(float64) error {
var total float64
return func(val float64) error {
if getter != nil {
current, err := getter()
if err != nil {
return err
}
total = current
}
delta := val - total
if err := setter(delta); err != nil {
return err
}
total = val
return nil
}
}
```
Use it in both methods:
```go
func (p *deltaPlugin) FloatSetter(param string) (func(float64) error, error) {
set, err := p.set.FloatSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (float64, error)
if p.get != nil {
getter, err = p.get.FloatGetter(p.ctx)
if err != nil {
return nil, err
}
}
return makeDeltaFloatSetter(getter, set), nil
}
func (p *deltaPlugin) IntSetter(param string) (func(int64) error, error) {
set, err := p.set.IntSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (int64, error)
if p.get != nil {
getter, err = p.get.IntGetter(p.ctx)
if err != nil {
return nil, err
}
}
floatGetter := func() (float64, error) {
if getter == nil {
return 0, nil
}
v, err := getter()
return float64(v), err
}
floatSetter := func(delta float64) error {
return set(int64(delta))
}
f := makeDeltaFloatSetter(floatGetter, floatSetter)
return func(v int64) error { return f(float64(v)) }, nil
}
```
This keeps all existing behavior, but:
- `deltaPlugin` no longer carries mutable state.
- Nil handling for `get` is explicit and confined.
- Delta computation logic lives in one place instead of being duplicated.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "github.com/evcc-io/evcc/util" | ||
| ) | ||
|
|
||
| type deltaPlugin struct { |
There was a problem hiding this comment.
issue (complexity): Consider making the plugin stateless and reusing a shared delta-calculation helper so each setter keeps its own local running total instead of using shared struct state.
You can remove most of the complexity without changing behavior by (1) making total closure-local instead of struct state and (2) factoring the common logic + fixing the get pointer handling.
1. Make total closure-local (struct becomes stateless)
You don’t need p.total at all; keep it inside each setter factory. That removes hidden coupling between different setters.
type deltaPlugin struct {
ctx context.Context
set Config
get *Config
}
func (p *deltaPlugin) IntSetter(param string) (func(int64) error, error) {
set, err := p.set.IntSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (int64, error)
if p.get != nil {
getter, err = p.get.IntGetter(p.ctx)
if err != nil {
return nil, err
}
}
var total float64
return func(val int64) error {
if getter != nil {
current, err := getter()
if err != nil {
return err
}
total = float64(current)
}
delta := float64(val) - total
if err := set(int64(delta)); err != nil {
return err
}
total = float64(val)
return nil
}, nil
}Same idea for FloatSetter:
func (p *deltaPlugin) FloatSetter(param string) (func(float64) error, error) {
set, err := p.set.FloatSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (float64, error)
if p.get != nil {
getter, err = p.get.FloatGetter(p.ctx)
if err != nil {
return nil, err
}
}
var total float64
return func(val float64) error {
if getter != nil {
current, err := getter()
if err != nil {
return err
}
total = current
}
delta := val - total
if err := set(delta); err != nil {
return err
}
total = val
return nil
}, nil
}This keeps the “delta relative to last total” behavior but avoids shared mutable state across closures and avoids concurrent access issues.
2. Factor common delta logic (optional but reduces duplication)
If you want to go a step further, you can encapsulate the shared pattern and keep the concrete setters very small:
func makeDeltaFloatSetter(
getter func() (float64, error),
setter func(float64) error,
) func(float64) error {
var total float64
return func(val float64) error {
if getter != nil {
current, err := getter()
if err != nil {
return err
}
total = current
}
delta := val - total
if err := setter(delta); err != nil {
return err
}
total = val
return nil
}
}Use it in both methods:
func (p *deltaPlugin) FloatSetter(param string) (func(float64) error, error) {
set, err := p.set.FloatSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (float64, error)
if p.get != nil {
getter, err = p.get.FloatGetter(p.ctx)
if err != nil {
return nil, err
}
}
return makeDeltaFloatSetter(getter, set), nil
}
func (p *deltaPlugin) IntSetter(param string) (func(int64) error, error) {
set, err := p.set.IntSetter(p.ctx, param)
if err != nil {
return nil, err
}
var getter func() (int64, error)
if p.get != nil {
getter, err = p.get.IntGetter(p.ctx)
if err != nil {
return nil, err
}
}
floatGetter := func() (float64, error) {
if getter == nil {
return 0, nil
}
v, err := getter()
return float64(v), err
}
floatSetter := func(delta float64) error {
return set(int64(delta))
}
f := makeDeltaFloatSetter(floatGetter, floatSetter)
return func(v int64) error { return f(float64(v)) }, nil
}This keeps all existing behavior, but:
deltaPluginno longer carries mutable state.- Nil handling for
getis explicit and confined. - Delta computation logic lives in one place instead of being duplicated.
4 participants
Refs #26875 (comment)
Some heatpumps expect current excess power, which is the delta of target power and current power. The delta plugin calculates that difference.
/cc @premultiply @ganzton @docolli