Fonius GEN24: add curtail#29983
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In plugin/sunspec.go the interface assertion uses
(*Modbus)(nil)forBoolGetter, but the method is defined on*ModbusSunspec, so the conformance check should likely reference*ModbusSunspecinstead. - The new BoolGetter implementations in sunspec and map implicitly treat any non-zero numeric value as true; if the underlying protocols distinguish between specific enum/bitfield values (e.g. >1 meaning error or different state), it may be worth constraining the accepted values or at least documenting this assumption near the conversion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In plugin/sunspec.go the interface assertion uses `(*Modbus)(nil)` for `BoolGetter`, but the method is defined on `*ModbusSunspec`, so the conformance check should likely reference `*ModbusSunspec` instead.
- The new BoolGetter implementations in sunspec and map implicitly treat any non-zero numeric value as true; if the underlying protocols distinguish between specific enum/bitfield values (e.g. >1 meaning error or different state), it may be worth constraining the accepted values or at least documenting this assumption near the conversion.
## Individual Comments
### Comment 1
<location path="plugin/sunspec.go" line_range="160" />
<code_context>
}, err
}
+var _ BoolGetter = (*Modbus)(nil)
+
+// BoolGetter executes configured modbus read operation and implements BoolGetter.
</code_context>
<issue_to_address>
**issue (bug_risk):** The BoolGetter interface assertion references Modbus instead of ModbusSunspec, which will panic at init or fail to compile depending on types.
`BoolGetter` is defined on `*ModbusSunspec`, but the interface assertion uses `(*Modbus)(nil)`. If these are different types, this either won’t compile or will wrongly assert that `Modbus` implements `BoolGetter`. This should instead be:
```go
var _ BoolGetter = (*ModbusSunspec)(nil)
```
to match the actual receiver type.
</issue_to_address>
### Comment 2
<location path="plugin/sunspec.go" line_range="169" />
<code_context>
+ return nil, err
+ }
+
+ return func() (b bool, err error) {
+ defer func() {
+ if r := recover(); r != nil {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated type-to-bool conversion in BoolGetter into a separate helper to keep the getter focused on I/O and error handling.
You can reduce the repetition and keep the behavior by extracting the conversion into a small helper and using the local `v` in the error. This keeps `BoolGetter` focused on I/O and error handling.
Example refactor:
```go
func boolFromPointValue(v any) (bool, error) {
switch x := v.(type) {
case bool:
return x, nil
case sunspec.Enum16, sunspec.Enum32,
sunspec.Bitfield16, sunspec.Bitfield32,
int16, int32, int64,
uint16, uint32, uint64:
return x != 0, nil
default:
return false, fmt.Errorf("invalid point type: %T", v)
}
}
```
Then `BoolGetter` becomes:
```go
func (m *ModbusSunspec) BoolGetter() (func() (bool, error), error) {
block, point, err := m.blockPoint()
if err != nil {
return nil, err
}
return func() (b bool, err error) {
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("panic: %v", r)
}
}()
if err := block.Read(); err != nil {
return false, fmt.Errorf("model %d block %d point %s: %w",
m.op.Model, m.op.Block, m.op.Point, err)
}
return boolFromPointValue(point.Value())
}, nil
}
```
This preserves all supported types and behavior, but removes the large repetitive switch from the closure and keeps the conversion logic in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Was ist, wenn |
richtig |
|
Bildet das Template so aber nicht ab.. [EDIT] |
|
wenn du 100 in der pv meter config einträgst nein dann nicht. |
|
bei curtail enable / true: WMaxLim_Ena true (1) und WMaxLimPct (was du in der pv meter config eingetragen hast) |
|
Die Curtail-Schnittstelle ist bool-basiert. Übergabe von beliebigen Prozentwerten wie 30, 60, 100 müsste die Curtail-API selbst von Bool auf Prozent/Faktor erweitert werden. |
| description: | ||
| generic: Integer | ||
| deprecated: true | ||
| - name: curtailpercent |
There was a problem hiding this comment.
Bad name probably? What's being set IMHO is the active power in % of the max. inverter power. curtailpercent is more like 100-active power.
| uri: {{ joinHostPort .host .port }} | ||
| id: 1 | ||
| value: 123:WMaxLimPct | ||
| - source: map |
|
|
||
| var _ BoolGetter = (*ModbusSunspec)(nil) | ||
|
|
||
| func boolFromPointValue(v any) (bool, error) { |
There was a problem hiding this comment.
Looks like
cast.ToInt64(cast.ToBool())
?
| // battery control support | ||
| CapabilityBatteryControl // battery-control | ||
| // inverter curtailment support | ||
| CapabilityCurtail // curtail |
There was a problem hiding this comment.
Why? This is already in master?!
Isn't it part of pending #29905?
There was a problem hiding this comment.
Ah, sorry, yes. In that case it's usually better to branch from there instead of master. Also helps to document the dependency in the PR description to avoid such confusion.
| "enum": [ | ||
| "1p3p", | ||
| "battery-control", | ||
| "curtail", |
Depends on #29905.
Ich habe den Scope bewusst klein gehalten und nur den GEN24-Curtail-Pfad ergänzt, ohne Bool/Int-Verträge in bestehenden Settern zu vermischen.
Was wurde gebaut:
GEN24-Template um Curtail ergänzt in fronius-gen24.yaml
Neue Template-Capability curtail registriert in capability.go und generiert in capability_enumer.go
Schema um curtail erweitert in devices-schema.json
Map-Plugin um bool-int Delegation erweitert in map.go, damit ein boolescher Curtail-Input deklarativ auf numerische SunSpec-Register gemappt werden kann
SunSpec-Plugin um BoolGetter erweitert in sunspec.go, damit enum16-Statuspunkte wie WMaxLim_Ena als bool gelesen werden können
Warum diese Änderungen nötig sind:
Der evcc-Curtail-Pfad ist API-seitig bool (Curtail/Curtailed).
GEN24 benötigt gleichzeitig numerische Registerwrites:
WMaxLimPct (Prozentwert)
WMaxLim_Ena (0/1 als enum16)
Ohne BoolGetter in SunSpec schlägt das Lesen von WMaxLim_Ena als Curtailed mit dem bekannten scaled-value Panic fehl.
Ohne bool-int Brücke im map-Plugin lässt sich der boolesche Curtail-Input im deklarativen Template nicht sauber auf die benötigten numerischen Writes abbilden.
Was bewusst nicht gemacht wurde:
Keine Aufweichung von BoolSetter-Verträgen in const/switch
Keine zusätzlichen Skript-/Go-Sonderpfade im Template
Keine Änderungen an hems/config.go, fnn.go oder anderen themenfremden Dateien im Commit