fix(fetch): split charge windows at dawn again, unify low-power PV thresholds - #4726
Open
chalfontchubby wants to merge 1 commit into
Open
fix(fetch): split charge windows at dawn again, unify low-power PV thresholds#4726chalfontchubby wants to merge 1 commit into
chalfontchubby wants to merge 1 commit into
Conversation
…resholds Fixes #4699. self.pv_forecast_minute was reset to {} at the top of fetch_sensor_data() but calc_pv_light_dark()/calc_dawn() still ran before the line that actually repopulates it - so the dawn split (#4557) always saw an empty forecast and never fired, for any combine_charge_slots user, since the feature first shipped. Fixed by moving the populating fetch_pv_forecast() call earlier. Separately, the debug-replay test harness's own rate-rescan reimplementation never passed pv_light_dark through at all, so no debug.yaml replay could exercise this path even after the fix - both are now fixed. While investigating, found the two low-power PV thresholds (LOW_POWER_PV_LIGHT_FRACTION, a % of the forecast's own peak, and LOW_POWER_PV_THRESHOLD, a fixed kWh figure) were independently tuned and could disagree on genuine twilight PV. A % of this forecast's own peak is also unreliable on a heavily overcast day, whose own peak is low too. Replaced both with a single new config item, low_power_pv_threshold_w (an absolute Watts figure, default 150), used consistently by calc_dawn (split boundary) and find_charge_rate (low-power abandon decision, now comparing average power over the remaining window rather than accumulated energy, so a long window at a low constant trickle no longer creeps past the threshold). Added binary_sensor.predbat_dawn, docs updated for both the sensor and the new config item. Verified the average-power fix as a genuine regression: temporarily reverted to the old fixed-kWh comparison and confirmed the new low_power_below_threshold_stays_low_power test fails without it. One random regression scenario (seed 15) shifted as an expected consequence of the behaviour actually changing - baseline regenerated, confirmed it's the only one that moved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Threshold boundary handling, configuration visibility, dawn sensor state, and production-order test coverage need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes dawn-based charge-window splitting and unifies low-power PV thresholds.
Changes:
- Fetches PV forecasts before calculating dawn splits.
- Adds a configurable 150 W PV threshold and dawn sensor.
- Updates low-power behavior, tests, documentation, and regression baseline.
File summaries
| File | Description |
|---|---|
apps/predbat/config.py |
Adds the PV threshold setting. |
apps/predbat/const.py |
Removes superseded constants. |
apps/predbat/execute.py |
Passes the threshold during execution. |
apps/predbat/fetch.py |
Reorders PV fetching, calculates dawn, and publishes its state. |
apps/predbat/output.py |
Applies the threshold when displaying charge rates. |
apps/predbat/predbat.py |
Initializes dawn classification state. |
apps/predbat/prediction.py |
Propagates the threshold into predictions. |
apps/predbat/utils.py |
Uses average PV power for low-power decisions. |
apps/predbat/tests/test_find_charge_rate.py |
Updates PV overlap tests. |
apps/predbat/tests/test_find_charge_window.py |
Tests absolute-threshold dawn detection. |
apps/predbat/tests/test_model.py |
Adds a low-PV regression scenario. |
apps/predbat/tests/test_single_debug.py |
Includes dawn splitting during debug replay. |
coverage/cases/random_results.json |
Updates the affected random baseline. |
docs/customisation.md |
Documents threshold behavior and configuration. |
docs/output-data.md |
Documents the dawn sensor. |
Review details
Suppressed comments (1)
apps/predbat/fetch.py:1643
- A configured threshold of 0 W makes every all-zero bucket satisfy
average >= 0, so an empty-production day is classified as light from midnight and the dawn sensor reports on. Since 0 is an allowed value, require some positive PV before latching light; this preserves the useful meaning “any non-zero PV” without treating darkness as dawn.
light_threshold = self.low_power_pv_threshold_w / MINUTE_WATT
- Files reviewed: 14/15 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "step": 10, | ||
| "unit": "W", | ||
| "icon": "mdi:weather-sunny", | ||
| "enable": "set_charge_low_power", |
| # at max rate instead - a throttled rate would cap the PV going into the battery, exporting the | ||
| # surplus and importing to make the target up later | ||
| low_power_pv_threshold_kwh = (low_power_pv_threshold_w / MINUTE_WATT) * max(abs_minutes_left, 0) | ||
| if pv_window_kwh > low_power_pv_threshold_kwh: |
Comment on lines
+1064
to
+1066
| self.dashboard_item( | ||
| "binary_sensor." + self.prefix + "_dawn", | ||
| state="on" if pv_light_dark.get(self.minutes_now) == 1 else "off", |
Comment on lines
+1042
to
+1047
| # Fetch PV forecast if enabled, today must be enabled, other days are optional. Needed here, | ||
| # ahead of "Find charging windows" below, because calc_pv_light_dark() reads | ||
| # self.pv_forecast_minute to locate dawn - it was previously fetched further down in this | ||
| # function, after the dawn calculation had already run against the freshly-reset empty dict | ||
| # from the top of fetch_sensor_data(), so the dawn split could never actually trigger (#4699). | ||
| self.pv_forecast_minute, self.pv_forecast_minute10, self.pv_forecast_minute90 = self.fetch_pv_forecast() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4699.
self.pv_forecast_minutewas reset to{}at the top offetch_sensor_data(), butcalc_pv_light_dark()/calc_dawn()still ran before the line that actually repopulates it - so the dawn split (#4557) always saw an empty forecast and never fired, for anycombine_charge_slotsuser, since the feature first shipped. Fixed by moving the populatingfetch_pv_forecast()call earlier.Separately, the debug-replay test harness's own rate-rescan reimplementation (
tests/test_single_debug.py) never passedpv_light_darkthrough torate_scan_window()at all - an independent gap that meant no debug.yaml replay could exercise this path even after the production fix. Both are fixed here, and together they let the real fix be verified against the reporter's own debug.yaml: the window now splits at dawn with the pre-dawn slice going to low power instead of full rate.Threshold unification
While investigating, found the two low-power PV thresholds were independently tuned and could disagree on genuine twilight PV:
LOW_POWER_PV_LIGHT_FRACTION- a % of this forecast's own peak, used to decide where to split a window at dawnLOW_POWER_PV_THRESHOLD- a fixed kWh figure, used to decide whether to abandon low-power charging for a windowA %-of-peak threshold is also unreliable on a heavily overcast day, since that day's own peak is low too - the same % ends up being a much lower absolute wattage than on a clear day, and Predbat has no declared panel capacity to normalise against instead (only
inverter_limit, which caps the inverter's own output, not the array's rating).Replaced both constants with a single new config item,
low_power_pv_threshold_w(absolute Watts, default 150W), used consistently by:calc_dawn(fetch.py) - the split boundaryfind_charge_rate(utils.py) - the abandon decision, now comparing average power over the remaining window against the threshold rather than accumulated energy against a fixed kWh figure, so a long window at a low constant trickle no longer creeps past it regardless of window length (the actual twilight-creep failure mode this was hiding)Also added
binary_sensor.predbat_dawn(on past dawn / off before or unclassified) and updated docs for both the sensor and the new config item.Verification
git blame+ reading (deterministic, not probabilistic): the reset and the read are in the same function, no path repopulates in between.low_power_below_threshold_stays_low_powertest fails without it (13.62kWh -> 14.2kWh final SoC), passes with it.run_pre_commitclean.Test plan
./run_all --quickpasses./run_all --test find_charge_window --test find_charge_rate --test model- new/updated scenarios pass./run_all --test random- only seed 15 changed, as expected./run_pre_commitpasses (ruff, black, cspell, markdownlint, full test suite)🤖 Generated with Claude Code