Huawei: add HoldCharge#30598
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 left some high level feedback:
- The new
holdchargecase duplicates themaxchargepowerwrite logic that was added to the other cases; consider extracting a shared sequence or helper template to avoid repeating the same47075/47077write blocks in multiple branches. - In the
holdchargesequence you explicitly zero47075while keeping47077atmaxdischargepower; if this asymmetry is intentional, a brief inline comment explaining why discharge is left at max while charge is zeroed would make the behavior clearer to future maintainers. - The
watchdoghandler’s newcase: 4uses asleepwithduration: 0s; if this case does not need any special handling, you can omit the branch entirely rather than adding a no-op step.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `holdcharge` case duplicates the `maxchargepower` write logic that was added to the other cases; consider extracting a shared sequence or helper template to avoid repeating the same `47075`/`47077` write blocks in multiple branches.
- In the `holdcharge` sequence you explicitly zero `47075` while keeping `47077` at `maxdischargepower`; if this asymmetry is intentional, a brief inline comment explaining why discharge is left at max while charge is zeroed would make the behavior clearer to future maintainers.
- The `watchdog` handler’s new `case: 4` uses a `sleep` with `duration: 0s`; if this case does not need any special handling, you can omit the branch entirely rather than adding a no-op step.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Looks to be working. But I want to do some more stability testing, also see #30467. |
|
I am again experiencing a (reproducible) race-condition when switching from Splitting modes in |
|
Sounds as if the watchdog reset happens too late? It would be helpful if we could have a demo config that allows to reproduce this without having a Huawei inverter. |
|
Fix options, roughly in order of preference:
Imho 3 would be easier to understand unless this kills any eeprom register? |
Hope not. I am just trying to only do as much as necessary to keep the burden on those devices as low as possible... |
|
BTW... So far I haven't realized that error because as soon as the 1 minute watchdog expires, the forcible charging is disabled anyway. I just realized now because I am actively checking the register states after a mode switch... |
|
I measured response times when writing the respective registers and they indeed seem costly, between 500ms and multiple seconds(!) per register. |
|
So how do you want to proceed here? I think it makes a ton of sense to have a setup and a periodic phase for a state. Plus I want to keep the numbers of registers written low as writes are indeed expensive for Huawei. I will finish testing |
|
Up to you. Accept the broken config or do this. |
|
Good call — simplified in 1872726 using the switch plugin's ```yaml Verified directly against 🤖 Generated with Claude Code |
|
@CiNcH83 that's good news, actually — the bug is fixed. Your new trace shows The double 🤖 Generated with Claude Code |
|
Oooops Still racy. This time with latest change. |
|
I now made stopWdt synchronous and handle the racy 47100 register solely in the watchdog. No more races so far. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The max charge power clamping logic for register 47075 is duplicated in three different cases in the Huawei template; consider extracting this into a shared include/partial to keep behavior consistent and easier to maintain if the clamping rules change.
- In the watchdog plugin,
stopWdtnow temporarily releaseso.muwhile waiting onwdtDone; it would help future maintainers if this lock/unlock behavior and its implications for callers (e.g. any assumptions about atomicity aroundset) were clearly documented at the type or method level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The max charge power clamping logic for register 47075 is duplicated in three different cases in the Huawei template; consider extracting this into a shared include/partial to keep behavior consistent and easier to maintain if the clamping rules change.
- In the watchdog plugin, `stopWdt` now temporarily releases `o.mu` while waiting on `wdtDone`; it would help future maintainers if this lock/unlock behavior and its implications for callers (e.g. any assumptions about atomicity around `set`) were clearly documented at the type or method level.
## Individual Comments
### Comment 1
<location path="plugin/watchdog.go" line_range="25" />
<code_context>
timeout time.Duration
deferred bool
cancel func()
+ wdtDone chan struct{} // closed by the running wdt goroutine once it has exited
clock clock.Clock
}
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the watchdog start/stop lifecycle into a dedicated helper type so `setter` only coordinates via simple start/stop calls instead of managing locks and channels directly.
You can encapsulate the watchdog lifecycle into a small helper type and remove most of the lock/chan juggling from `setter`, while still ensuring “no tick in-flight after reset”.
### 1. Encapsulate the lifecycle in a `watchdogRunner`
Instead of exposing `cancel` and `wdtDone` as separate fields on `watchdogPlugin`, move them into a small helper that owns its own context and completion signalling:
```go
type watchdogRunner struct {
mu sync.Mutex
cancel context.CancelFunc
wg sync.WaitGroup
}
func (r *watchdogRunner) Start(run func(ctx context.Context)) {
r.mu.Lock()
defer r.mu.Unlock()
// stop any existing runner first
if r.cancel != nil {
r.cancel()
}
if r.cancel != nil {
r.wg.Wait()
}
ctx, cancel := context.WithCancel(context.Background())
r.cancel = cancel
r.wg.Add(1)
go func() {
defer r.wg.Done()
run(ctx)
}()
}
func (r *watchdogRunner) Stop() {
r.mu.Lock()
cancel := r.cancel
r.cancel = nil
r.mu.Unlock()
if cancel != nil {
cancel()
r.wg.Wait()
}
}
```
Then change the plugin to use this, instead of `cancel` + `wdtDone`:
```go
type watchdogPlugin struct {
mu sync.Mutex
// ...
// cancel func()
// wdtDone chan struct{}
runner *watchdogRunner
}
```
Initialize `runner` in the constructor:
```go
o := &watchdogPlugin{
// ...
runner: &watchdogRunner{},
}
```
### 2. Simplify `setter` by delegating to the runner
Inside `setter`, you only coordinate with `runner.Start`/`Stop`; `setter` no longer needs to manipulate channels or split lock/unlock inside `stopWdt`:
```go
// stop running wdt and block until it has exited
stopWdt := func() {
// drop the lock before blocking to avoid splitting critical sections implicitly
o.mu.Unlock()
defer o.mu.Lock()
o.runner.Stop()
}
```
Starting the watchdog becomes:
```go
setAndStartWdt := func(val T) error {
if err := set(val); err != nil {
return err
}
lastUpdated = o.clock.Now()
last = &val
if !slices.Contains(reset, val) {
o.runner.Start(func(ctx context.Context) {
o.wdt(ctx, func() error {
o.mu.Lock()
defer o.mu.Unlock()
if ctx.Err() != nil {
return nil
}
if err := set(val); err != nil {
return err
}
lastUpdated = o.clock.Now()
return nil
})
})
}
return nil
}
```
Key benefits without changing behavior:
- The “no in-flight tick after reset” invariant is enforced by `runner.Stop` using a `WaitGroup`, not an exposed `wdtDone` channel.
- `watchdogPlugin` no longer has to manage two coordination primitives (`cancel` + `wdtDone`) or drop/reacquire its own lock inside `stopWdt`.
- The lifecycle logic (ctx, cancel, wait for completion) is localized inside `watchdogRunner`, reducing mental overhead for future changes.
</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:
- The clamping logic for registers 47075/37046 and 47077/37048 is duplicated across several cases; consider extracting this into a reusable template or helper to reduce repetition and the risk of inconsistencies in future changes.
- In the watchdog plugin,
wdtDoneis set and cleared aroundstopWdt; it may be safer to document or enforce thatsetteris not called concurrently for the same plugin instance, or add guards to avoid races where multiple goroutines manipulatecancel/wdtDoneat the same time. - The new default branch in the 47100 switch unconditionally sends a "stop" command; verify that this behavior is intended for all non-[1,2,3,4] modes and consider explicitly listing supported modes to make the mapping easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The clamping logic for registers 47075/37046 and 47077/37048 is duplicated across several cases; consider extracting this into a reusable template or helper to reduce repetition and the risk of inconsistencies in future changes.
- In the watchdog plugin, `wdtDone` is set and cleared around `stopWdt`; it may be safer to document or enforce that `setter` is not called concurrently for the same plugin instance, or add guards to avoid races where multiple goroutines manipulate `cancel`/`wdtDone` at the same time.
- The new default branch in the 47100 switch unconditionally sends a "stop" command; verify that this behavior is intended for all non-[1,2,3,4] modes and consider explicitly listing supported modes to make the mapping easier to reason about.
## Individual Comments
### Comment 1
<location path="plugin/watchdog.go" line_range="93" />
<code_context>
+ // cancellation and no-opped) before the caller proceeds. Without this,
+ // a tick that fired just before cancel() can still race a subsequent
+ // write issued right after stopWdt() returns.
stopWdt := func() {
- if o.cancel != nil {
- o.cancel()
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `stopWdt` to return the `wdtDone` channel and keep all lock manipulation at the call site so the watchdog shutdown flow is easier to reason about.
The main complexity comes from the hidden `o.mu` unlock/lock inside `stopWdt`. You can keep the race‑fixing behavior (wait for the last tick to finish) while making the locking and `wdtDone` lifecycle easier to reason about by:
1. Making `stopWdt` *pure* w.r.t. locking (no `o.mu.Unlock()/Lock()`).
2. Returning the `done` channel from `stopWdt`, and having the caller handle lock release/ reacquisition around the blocking wait.
This keeps all semantics (including “no tick races with subsequent writes”) but avoids subtle lock juggling hidden inside `stopWdt`.
### Suggested refactor
Change `stopWdt` to only manage cancellation and the `wdtDone` channel:
```go
// stop running wdt and return a channel that is closed once the wdt goroutine has exited.
stopWdt := func() (done <-chan struct{}) {
if o.cancel == nil {
return nil
}
o.cancel()
done = o.wdtDone
o.cancel = nil
o.wdtDone = nil
return done
}
```
Then, at the call site (inside the returned setter closure where `o.mu` is held), explicitly handle the unlock/wait/lock pattern:
```go
return func(val T) error {
o.mu.Lock()
defer o.mu.Unlock()
// cancel deferred update
// ...
// stop watchdog and wait for completion without holding o.mu
done := stopWdt()
if done != nil {
o.mu.Unlock()
<-done
o.mu.Lock()
}
// proceed with writes / state updates knowing no tick is in flight
// ...
return nil
}
```
This retains:
- Waiting for the watchdog goroutine to fully exit before proceeding (no races with subsequent writes).
- The `wdtDone` lifecycle tied to each watchdog instance.
But it:
- Removes implicit assumptions about lock state inside `stopWdt`.
- Makes the lock and channel coordination explicit at the call site, which is much easier to audit and reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The wdtDone barrier guarded a stale-tick write that only existed while 47100=0 (reset) and 47100=1 (charge) lived in separate plugin instances with separate mutexes. Now that all 47100 writes go through the single watchdog plugin, the existing ctx.Err() guard under o.mu already prevents a cancelled tick from writing. Restore the simpler non-blocking stopWdt.
|
@CiNcH83 this should be resolved now. Your diagnosis was right: the leftover The fix is to stop splitting: Given that, the earlier "synchronous Could you re-run your 🤖 Generated with Claude Code |
|
@CiNcH83 is this one good to merge with latest changes? |
This matches my custom LUNA device implementation. No races. |
|
Thank you, much appreciated. |
2 participants
SUN2000/SDongle Modbus Primer
Testing
States
normal: battery charged with surplus only / discharged to home ...hold: only discharging locked / charging with surplus still possible ...charge: force-charge atmaxchargepowerfrom surplus/grid combined ...holdcharge: only charging locked / discharging still possible ...State Transitions
normal→holdnormal→chargenormal→holdchargehold→normalhold→chargehold→holdchargeholdcharge→normalholdcharge→holdholdcharge→chargecharge→normalcharge→holdcharge→holdchargeRegister states are being checked with
modbus-clivia evcc Modbus Proxy:Please beware that when checking states with FusionSolar portal, displayed settings might not reflect register settings immediately! Fetching the whole configuration via the portal also puts quite a lot of load on the SUN2000 which may cause Modbus errors for evcc.
Remarks
Old configs may still have the old default of
10000formaxchargepowerset which is higher than what most installed battery setups support (only the LUNA2000-S1 with 3 battery packs supports a higher charge power). While the Forcible charge power register (47247) allows out-of-bounds values to be set, trying to set it for the Maximum charging power register (47075) introduced in this PR fails. The introduced clamping mechanism takes care of that. So this is not a breaking change. Broken configs will remain broken though.This PR also fixes a race-condition in the watchdog plugin. After leaving
charge, there could have been another watchdog tick, prolongingchargefor another minute in the Huawei case.It would be great if Huawei had something similar to the SunSpec
StorCtl_Modregister. With Huawei, every mode requires quite some registers to be written for cleaning up every potential previous mode/state as the number of modes increases. Implementing all modes (force-charge, force-discharge, lock, lock-charge, lock-discharge) via register47100, also being guarded by a HW watchdog, would indeed be superb. It unfortunately only supports force-charge and force-discharge today.