Repository navigation
Stop an export target landing in the reserved range (#4914) - #5020
Closed
chalfontchubby wants to merge 2 commits into
Closed
chalfontchubby wants to merge 2 commits into
chalfontchubby wants to merge 2 commits into
Conversation
An export limit packs the target SoC in the integer part and the export power in the fraction, with 99.0 reserved for freeze and 100.0 for idle. Reserving 99.0 therefore consumes the whole of [99.0, 100.0): a 99% target at a low power rung packs to 99.3/99.5/99.7, which is neither == 99.0 (freeze) nor < 99.0 (forced export), so the window is silently inert. Two paths reach it, and both are capped below the reserved range here. optimise_export's ladder clamps the target up to the SoC floor, so a best_soc_min at 99% of soc_max produces it. That is the path #4914 describes, and it needs an unusual but reachable expert setting. clip_export_slots is the more reachable one and was not in the report: it narrows the target to whatever the simulation says the battery actually reaches, less a ten minute discharge margin. With a modest discharge rate that margin is small, so a nearly-full battery clips straight up to 99 - no unusual config at all. At lower rates it overshoots further: calc_percent_limit caps the integer part at 100 and the fraction is added afterwards, giving 100.3, which reads as above the idle sentinel and disables the window outright. A target of 98% and 99% are the same request in practice, so clamping loses nothing real. The idle and freeze rungs are the reserved values themselves and pass through untouched. This is a stopgap. Splitting the limit into explicit mode/target/power fields removes the reserved range altogether and makes the clamp redundant; it can be deleted when that lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The regression permits sentinel outcomes and does not cover the optimiser path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents packed export targets from entering reserved freeze/idle ranges.
Changes:
- Adds a 98% maximum force-export target.
- Applies it during optimisation and export-slot clipping.
- Adds regression coverage for clipping.
File summaries
| File | Description |
|---|---|
apps/predbat/const.py |
Defines the maximum export target. |
apps/predbat/plan.py |
Clamps export targets before adding power fractions. |
apps/predbat/tests/test_clip_export_slots.py |
Tests near-full low-power clipping. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ort_slots result Two review findings on the #4914 export-target-clamp regression tests. test_clip_up_never_lands_in_the_reserved_range let a sentinel outcome pass: its third assertion exempted limit in (100.0, 99.0), so a clip that incorrectly collapsed to freeze (99.0) or idle (100.0) - losing the requested forced low-power export exactly as silently as landing in [99.0, 100.0) - reported PASS. Confirmed by reverting the clip_export_slots clamp: the unfixed code produces 99.3, which the old loose checks accepted. Pin the exact packed result (98.3) instead. optimise_export's own floor clamp (plan.py:2358) had no coverage at all - only the post-simulation clamp in clip_export_slots was tested. Added test_optimise_export_floor_clamped_below_reserved_range, which monkeypatches launch_run_prediction_export to record every candidate the optimiser hands to simulation with best_soc_min set to round to the 99% boundary. Confirmed by reverting just that clamp: the unfixed code simulates candidates at 99.3/99.5/99.7, exactly the reserved-range values the fix exists to avoid. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
(Posted by Rik; written by Claude.) Closing as superseded by #5047. That refactor replaced the packed-float export-limit encoding with a Rebasing this onto main would mean reapplying dead code against an encoding that no longer exists, so closing rather than carrying it forward. |
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.
Fixes #4914.
The problem
An export limit packs three signals into one double: target SoC in the integer part, export power in the fraction, and mode as two reserved whole values (
EXPORT_LIMIT_FREEZE = 99.0,EXPORT_LIMIT_IDLE = 100.0).Because the fraction is live, reserving
99.0actually consumes the whole of[99.0, 100.0). A 99% target at a low power rung packs to 99.3 / 99.5 / 99.7, which is neither== 99.0(freeze) nor< 99.0(forced export) — so the window silently does nothing. No error, no warning; the plan shows an export window and the battery just ignores it.Two paths reach it, not one
optimise_export's ladder clamps the target up to the SoC floor, so abest_soc_minat 99% ofsoc_maxproduces it. This is the path #4914 describes, and it needs an unusual (though reachable) expert setting.clip_export_slotsis the more reachable one, and was not in the original report. It narrows the target to whatever the simulation says the battery actually reaches, less a ten-minute discharge margin. With a modest discharge rate that margin is small, so a nearly-full battery clips straight up to 99 — no unusual configuration at all. Measured against current main:That last row is a third failure mode the issue does not mention:
calc_percent_limitcaps the integer part at 100 and the fraction is added afterwards, so the value can overshootEXPORT_LIMIT_IDLEentirely.The fix
Cap the target at
EXPORT_TARGET_MAX_PERCENT = 98before the power fraction is attached, in both paths. A 98% and a 99% target are the same request in practice, so nothing real is lost. The idle and freeze rungs are the reserved values themselves and pass through untouched.Testing
New
test_clip_up_never_lands_in_the_reserved_rangeintest_clip_export_slots.py, which reproduces the 600 W case. Verified to fail without the fix (produces 99.3) and pass with it. It also asserts the power fraction survives the clamp, so a low-power export cannot silently become full rate. Full suite passes;run_pre_commitclean.Note
This is a stopgap, and deliberately the cheaper of the two options set out in the issue. Splitting the limit into explicit mode/target/power fields removes the reserved range altogether and makes this clamp redundant — that work exists but is much larger and touches the C++ kernel ABI, so this lands the user-visible fix now rather than waiting on it.
🤖 Generated with Claude Code