-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
Fixes #27784
Each loadpoint only receives a full Update() call (including pvMaxCurrent) once every N x interval seconds via the round-robin in site loopLoadpoints. With 5 loadpoints at 15s interval, that is every 75s. In my situation that is exactly half of 2:30 configured enable delay.
This means I can get the log to display "pv enable in 0s" but still not enabling the loadpoint when there is slight jitter in the timer processing, especially when the enable interval is a multiple of the site interval.
[lp-2 ] DEBUG 2026/02/27 13:14:57 pv enable in 0s
[lp-2 ] DEBUG 2026/02/27 13:14:57 pv enable timer remaining: 0s
[lp-2 ] DEBUG 2026/02/27 13:15:12 charge power: 0W
I have included a regression test TestPVEnableTimerSubSecondBoundary that demonstrates the failure and passes with the fix applied. I think the best fix is to use the same rounding to second-precision as the log statements to for the enable/disable decision, as this ensures logs are consistent with control behavior.
Unfortunately I had to adapt a few other test cases that relied on nanosecond deltas for setting up timing cases, but those changes should hopefully be easy to review (i just changed them to use whole second deltas).
- test: reproduce PV timer sub-second jitter causing missed enable/disable
- fix: round pvTimer elapsed to second to prevent sub-second boundary miss
- fix: adapt existing timer test cases to use second-precision triggering
nearest second using .Round(time.Second). This prevents the PV enable/
disable timer from missing its target when the poll cycle lands a
fraction of a second short of the configured delay.
With multiple loadpoints (e.g. 5 at 15s interval = 75s cycle per LP),
sub-second jitter can cause the timer check to fail, delaying enable/
disable by an additional full cycle.
Update existing hysteresis tests to use second-granularity delays
instead of nanosecond precision, matching the new rounding behavior.
instead of nanoseconds (+/- 1) to setup timing thresholds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- By rounding
sincefor all timers inpublishTimer(not just PV), you're slightly changing the semantics of other timers (they can now fire up to ~0.5s earlier than before); consider constraining the rounding behavior to the PV timer paths if this change isn't explicitly desired globally. - In
pvTimerElapsed, rounding elapsed time can cause enable/disable to trigger up to half a second earlier than the configured delay; if you want to guarantee a minimum delay while still avoiding the jitter issue, consider usingTruncate(time.Second)for comparison and keeping rounding only for logging.
Prompt for AI Agents
## Overall Comments
- By rounding `since` for all timers in `publishTimer` (not just PV), you're slightly changing the semantics of other timers (they can now fire up to ~0.5s earlier than before); consider constraining the rounding behavior to the PV timer paths if this change isn't explicitly desired globally.
- In `pvTimerElapsed`, rounding elapsed time can cause enable/disable to trigger up to half a second earlier than the configured delay; if you want to guarantee a minimum delay while still avoiding the jitter issue, consider using `Truncate(time.Second)` for comparison and keeping rounding only for logging.
## Individual Comments
### Comment 1
<location path="core/loadpoint_test.go" line_range="898-901" />
+
+ for _, tc := range tc {
+ t.Run(tc.name, func(t *testing.T) {
+ clk := clock.NewMock()
+
+ Voltage = 100
**suggestion (testing):** Restore global Voltage after the test to avoid cross-test interference
This test modifies the global `Voltage` but never restores it, which can affect later tests that assume the default or another value. Please save the previous value and restore it with a `defer` after setting it here to keep tests isolated.
```suggestion
t.Run(tc.name, func(t *testing.T) {
clk := clock.NewMock()
prevVoltage := Voltage
defer func() {
Voltage = prevVoltage
}()
Voltage = 100
```
Help me be more useful! Please click or on each comment and I'll use the feedback to improve your reviews.
| t.Run(tc.name, func(t *testing.T) { | ||
| clk := clock.NewMock() | ||
|
|
||
| Voltage = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Restore global Voltage after the test to avoid cross-test interference
This test modifies the global Voltage but never restores it, which can affect later tests that assume the default or another value. Please save the previous value and restore it with a defer after setting it here to keep tests isolated.
| t.Run(tc.name, func(t *testing.T) { | |
| clk := clock.NewMock() | |
| Voltage = 100 | |
| t.Run(tc.name, func(t *testing.T) { | |
| clk := clock.NewMock() | |
| prevVoltage := Voltage | |
| defer func() { | |
| Voltage = prevVoltage | |
| }() | |
| Voltage = 100 |
| // rounded to the nearest second. Rounding is required because poll cycles can land a | ||
| // fraction of a second short of the configured delay, which would prevent the timer | ||
| // from firing until the next full poll cycle. | ||
| func (lp *Loadpoint) pvTimerElapsed() time.Duration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name is confusing. elapsed implies a true/false result
|
It would be nice if we could fix this without needing to touch all the (synthetic) tests. |
@andig I see no way to do this without touching the tests. The test data setup assumes full tick resolution of timers (which should be ns) but the intent of this PR is to effectively make timers use second resolution. I'll leave it up to you to decide whether that change and complexity is worth it to fix this problem in user experience. One other alternative that I could think of is to change the message e.g. "pv enable in 0s at next turn of the control loop for this loadpoint" or something like that. That is either immediately (if we are not having the pathological rounding issue case) or at the next turn, which may be a site_interval*#loadpoints away. Btw. after learning all this about how evcc works internally I feel like I should tune my site_interval to take the number of loadpoints into account... or am I fundamentally misunderstanding something here? |
|
Give this is purely a cosmetic problem I don't want to fix this in such an invasive way. If you tink the log message can be improved please provide a PR for that. Worth noting the UI already performs as expected. |