Hems/FNN: add curtail and dim for TAB26 Steuerbox relay standard#29886
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
Fnn4.Run, usingtime.Tickwithout stopping it can leak a ticker over the process lifetime; consider switching totime.NewTickerwith adefer ticker.Stop()and a loop overticker.C. NewFnn4FromConfigassumesS1andS2are non-nil and directly callsBoolGetteron them; add explicit nil checks (and a clear error) to avoid panics when these inputs are not configured.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Fnn4.Run`, using `time.Tick` without stopping it can leak a ticker over the process lifetime; consider switching to `time.NewTicker` with a `defer ticker.Stop()` and a loop over `ticker.C`.
- `NewFnn4FromConfig` assumes `S1` and `S2` are non-nil and directly calls `BoolGetter` on them; add explicit nil checks (and a clear error) to avoid panics when these inputs are not configured.
## Individual Comments
### Comment 1
<location path="hems/fnn/fnn-4.go" line_range="58-63" />
<code_context>
+ site.SetCircuit(gridcontrol)
+
+ // fnn-3 inputs
+ s1G, err := cc.S1.BoolGetter(ctx)
+ if err != nil {
+ return nil, err
+ }
+
+ s2G, err := cc.S2.BoolGetter(ctx)
+ if err != nil {
+ return nil, err
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing optional S1/S2 configs without nil checks can panic at runtime if they are omitted from configuration.
`S1` and `S2` are `*plugin.Config` but are dereferenced via `cc.S1.BoolGetter(ctx)` and `cc.S2.BoolGetter(ctx)` without checking for nil. If either is optional, this will panic on startup when omitted. Even if they are required, consider validating and returning a clear config error instead of relying on a nil dereference.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I guess this PR should just remove Fnn3 and make either dim or curtail optional. |
|
Ok, I understood to not replace the fnn3 but use it in fnn4. |
|
I think so, yes. Noticed the duplicate setup so this seems better. |
|
I changed (added) the dim function into fnn-3 this is now testet an working. Either dim or curtail or both can be configured. |
|
Next step rename it to fnn-4. |
|
Lets name it |
|
ok i will rename it to fnn and alias it so fnn-3 config could be used |
The CSV Dokumentation system is currently partially working:
The UI behavior needs to be adapted as described above. Please note that all existing fnn-3 configurations will run without any changes or interruptions, and can be extended with the new dim configuration. |
|
config: type: fnn # or fnn-3
# §9 EZA
maxPower: 10000 # maximale Erzeugerleistung in Watt
#interval: 10s
## 0% Abregelung
w3: # Eingang für 100% Abregelung → setzt auf 0% Einspeisung/Leistung
source: http
## 30% Abregelung
s2: # optionaler Eingang für 30%
source: http
## 60% Abregelung
s1: # optionaler Eingang für 60%
source: http
# §14a / Verbrauch / SteuVE / Dim
maxPowerdim: 4200
## Dim
w4:
source: http |
thecem
changed the title
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
curtail,c.limit = new(c.maxPower * frac)will not compile sincenewexpects a type, not an expression; allocate afloat64and assign (e.g.v := c.maxPower * frac; c.limit = &v) or usenew(float64)then set the value. Runusestime.Tickin afor rangeloop with no way to stop, which can leak a goroutine; consider accepting acontext.Contextand usingtime.NewTickerwith properStophandling.- The
limitfield is only written incurtailand never read elsewhere; if it’s not needed for external consumers, consider removing it to avoid confusion or wire it into the control logic so the stored value is actually used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `curtail`, `c.limit = new(c.maxPower * frac)` will not compile since `new` expects a type, not an expression; allocate a `float64` and assign (e.g. `v := c.maxPower * frac; c.limit = &v`) or use `new(float64)` then set the value.
- `Run` uses `time.Tick` in a `for range` loop with no way to stop, which can leak a goroutine; consider accepting a `context.Context` and using `time.NewTicker` with proper `Stop` handling.
- The `limit` field is only written in `curtail` and never read elsewhere; if it’s not needed for external consumers, consider removing it to avoid confusion or wire it into the control logic so the stored value is actually used.
## Individual Comments
### Comment 1
<location path="hems/fnn/fnn.go" line_range="25-34" />
<code_context>
+ limit *float64
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix creation of pointer to computed limit value in `curtail`
In `curtail`, `c.limit = new(c.maxPower * frac)` won’t compile because `new` expects a type, not an expression. To store a pointer to the computed value while keeping `nil` when inactive, use:
```go
c.limit = nil
if active {
v := c.maxPower * frac
c.limit = &v
}
```
</issue_to_address>
### Comment 2
<location path="hems/fnn/fnn.go" line_range="31" />
<code_context>
+ interval time.Duration
+}
+
+// NewFromConfig creates an nn4 HEMS from generic config
+func NewFromConfig(ctx context.Context, other map[string]any, site site.API) (*Fnn, error) {
+ cc := struct {
</code_context>
<issue_to_address>
**nitpick (typo):** Correct the comment to match the FNN implementation
The comment still mentions "nn4" while the type and package are `Fnn`. Please update it to reference FNN (or the correct variant) to keep the documentation accurate.
```suggestion
// NewFromConfig creates an FNN HEMS from generic config
```
</issue_to_address>
### Comment 3
<location path="hems/fnn/fnn.go" line_range="97" />
<code_context>
+ return c, nil
+}
+
+func boolGetter(ctx context.Context, cfg *plugin.Config) (func() (bool, error), error) {
+ if cfg == nil {
+ return nil, nil
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the Fnn control flow to use non-nil getters, data-driven curtailment rules, and a shared helper for smartgrid/root updates to reduce duplication and bug surface area.
You can keep the new functionality but reduce complexity (and surface area for bugs) with some targeted refactors.
### 1. Avoid `nil` getters and scattered nil checks
`boolGetter` returning `nil` forces every caller to branch on it. You can return a no-op getter instead and remove all `nil` checks in `Update` and `runDim`:
```go
func boolGetter(ctx context.Context, cfg *plugin.Config) (func() (bool, error), error) {
if cfg == nil {
// neutral default: “feature disabled”
return func() (bool, error) { return false, nil }, nil
}
return cfg.BoolGetter(ctx)
}
```
Then `Update` and `runDim` can assume non-nil functions:
```go
func (c *Fnn) runDim() error {
if c.maxPowerDim <= 0 {
return nil
}
active, err := c.w4()
if err != nil {
return err
}
limit := 0.0
if active {
limit = c.maxPowerDim
}
return c.setDim(limit)
}
```
### 2. Make `Update` data-driven to remove duplicated logic
You can replace the three repeated branches with a compact priority table. This keeps behavior identical but reduces duplication and makes changes easier:
```go
type curtailRule struct {
getter func() (bool, error)
fraction float64
}
func (c *Fnn) Update() error {
rules := []curtailRule{
{getter: c.w3, fraction: 0.0}, // 0%
{getter: c.s2, fraction: 0.3}, // 30%
{getter: c.s1, fraction: 0.6}, // 60%
}
for _, r := range rules {
active, err := r.getter()
if err != nil {
return err
}
if active {
return c.curtail(r.fraction)
}
}
// 100%
return c.curtail(1.0)
}
```
With the non-nil `boolGetter` above, you don’t need nil checks here, and adding new signals becomes a one-line change in `rules`.
### 3. Factor out the common “session + root” update pattern
`curtail` and `setDim` both lock, derive an `active` flag, call methods on `root`, and update a smartgrid session. You can centralize that pattern while preserving semantics:
```go
func (c *Fnn) applyMode(
id *uint,
mode smartgrid.Mode,
active bool,
limit float64,
applyRoot func(),
) {
applyRoot()
if err := smartgrid.UpdateSession(id, mode, c.root.GetChargePower(), limit, active); err != nil {
c.log.ERROR.Printf("smartgrid session: %v", err)
}
}
func (c *Fnn) curtail(frac float64) error {
c.mu.Lock()
defer c.mu.Unlock()
active := frac < 1.0
var limitPtr *float64
if active {
v := c.maxPower * frac
limitPtr = &v
}
c.limit = limitPtr
c.applyMode(&c.smartgridID, smartgrid.Curtail, active, c.maxPower*frac, func() {
c.root.Curtail(active)
// TODO: c.root.SetMaxPower(c.maxPower*frac) if/when enabled
})
return nil
}
func (c *Fnn) setDim(limit float64) error {
c.mu.Lock()
defer c.mu.Unlock()
active := limit > 0
c.applyMode(&c.smartgridDimID, smartgrid.Dim, active, limit, func() {
c.root.Dim(active)
c.root.SetMaxPower(limit)
})
return nil
}
```
This keeps functionality intact, keeps the locking behavior as-is, and reduces duplication so the two flows stay consistent (including future changes like logging or metrics).
</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>
|
We should probably also emit log warning if dim/curtail are executed but no limits configured. |
|
But we will stay on bool not a relativ value like 0/30/60/100? |
|
Updated fields in the configuration struct to use pointers for plugin.Config. Modified the BoolGetter calls to handle nil checks for the configuration fields.
andig
changed the title
|
Fails if W3 (S1/S2/W3 same) or W4 is configured: type: fnn-3 panic: runtime error: invalid memory address or nil pointer dereference goroutine 60 [running]: type: fnn panic: runtime error: invalid memory address or nil pointer dereference goroutine 123 [running]: If S1, S2, W3, or W4 are missing in YAML, no error is thrown; instead, nil is returned. In the FNN code, these getters are later called without a nil check. What happens in your two examples: Only W3 configured Only W4 configured |
Nil guard in the Curtail rule loopIn fnn.go:129 to fnn.go:136, rule.get is called without validation.Change: Before making the call, check if rule.get is nil and, if so, simply skip to the next rule. Nil guard for W4 in runDimIn fnn.go:151, c.w4 is called directly.Change: If c.w4 is nil, treat Dim as inactive (i.e., set the consumption limit to 0) instead of calling it.
- runDim/runCurtail are now no-ops when their inputs are not configured; previously they would overwrite c.root.Dim/Curtail/SetMaxPower on every tick, clobbering state set by other HEMS sharing the root circuit. - Legacy fnn-3 MaxPower (PV/curtail cap) now falls back to MaxCurtailPower instead of MaxDimPower so existing fnn-3 configs keep working. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Tested! With all config variants W3/W4/S1/S2/S2W3/S1W3/W4S1/W4S2/S1S2W3W4/S1S2W3. |
2 participants
This pull request introduces support for a new
fnnHEMS (Home Energy Management System) that replaces the existingfnn3implementation (backwards compatible).Config UI has to be adapted since "Externe Begrenzung" should have two limits for dim (§14a) and curtail (§9). Also "Kommunikaton via FNN-4" is Missing:
The limits are aligned with EEBus/EnWG:
TODO