Skip to content

feat: external_solar_mode for AC-coupled PV setups - #167

Draft
jdungen wants to merge 6 commits into
johanzander:mainfrom
jdungen:feat/external-solar-mode
Draft

feat: external_solar_mode for AC-coupled PV setups#167
jdungen wants to merge 6 commits into
johanzander:mainfrom
jdungen:feat/external-solar-mode

Conversation

@jdungen

@jdungen jdungen commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds opt-in battery.external_solar_mode (default false) so AC-coupled installations can charge during solar hours.
  • When enabled, SOLAR_STORAGE periods map to grid_charge=True in the inverter controller; all other intents keep their default mapping.
  • Wired end-to-end: dataclass + from_ha_config, settings_store bootstrap + schema migration, API setup-complete payload, Settings → Battery toggle, and the setup wizard.
  • DC-coupled users see no behavioral change (flag defaults false).

Closes #162.

Why

On AC-coupled installations (e.g. SolarEdge for PV + Growatt for battery, microinverters, or any external-inverter setup) the battery inverter has no DC solar input. Surplus solar reaches the battery only via the meter. Today inverter_controller.py:35 hard-codes SOLAR_STORAGE to grid_charge=False, so the battery sits idle the entire solar window even though the DP planner has scheduled storage.

Files

  • core/bess/settings.pyexternal_solar_mode: bool = False on BatterySettings
  • core/bess/inverter_controller.py_effective_grid_charge helper applied in _map_intent_to_rates, get_period_settings, and get_detailed_period_groups
  • backend/settings_store.py — bootstrap default + schema-migration entry
  • backend/api.py_BATTERY_MAP + live-update payload for /api/setup/complete
  • backend/api_dataclasses.pyexternalSolarMode on APISetupCompletePayload
  • frontend/src/components/settings/BatteryFormSection.tsx — new "PV coupling" section with toggle
  • frontend/src/pages/SettingsPage.tsx, SetupWizardPage.tsx — wire load/save
  • frontend/src/types.ts — optional externalSolarMode

Test plan

  • pytest core/bess/tests/unit/test_external_solar_mode.py — 10 new tests pass (default-disabled, SOLAR_STORAGE override on/off, other intents unaffected, get_period_settings + get_detailed_period_groups apply override)
  • pytest core/bess/tests/unit/ -m "not slow" — 600 passed, 12 skipped
  • pytest backend/tests/ -m "not slow" — 201 passed
  • Frontend type check / E2E in CI (no local node_modules in this environment)
  • Live verification on AC-coupled HA install

On AC-coupled installations the PV panels are wired to a separate
inverter (e.g. SolarEdge, microinverters) and the battery inverter has
no DC solar input. The only physical charging path is via the grid —
surplus solar returns through the meter. With SOLAR_STORAGE hard-coded
to grid_charge=False the battery sits idle the entire solar window.

Adds an opt-in battery.external_solar_mode flag (default false, so
DC-coupled users see no change). When enabled, the SOLAR_STORAGE intent
maps to grid_charge=True in the inverter controller; all other intents
keep their default mapping.

Wired end-to-end:
- BatterySettings dataclass + from_ha_config
- InverterController helper applied in _map_intent_to_rates,
  get_period_settings, and get_detailed_period_groups
- settings_store bootstrap defaults + schema migration
- Settings → Battery tab toggle (PV coupling section)
- Setup wizard load + complete payload

Tests: 10 new behavioral tests covering the override in isolation and
through get_period_settings / get_detailed_period_groups. Full unit
suite (600) and backend suite (201) pass.

Closes johanzander#162
@jdungen
jdungen marked this pull request as ready for review June 23, 2026 22:03
jdungen added a commit to jdungen/bess-manager that referenced this pull request Jun 23, 2026
Combines two pending upstream PRs into a local fork build so the AC-coupled
installation can use them before they land on johanzander/main:
- johanzander#164: extend Nordpool area hints to NL/BE/DE/FR/AT/PL
- johanzander#167: external_solar_mode for AC-coupled PV setups
@johanzander

Copy link
Copy Markdown
Owner

Thanks for this — the feature is well-structured and the end-to-end wiring (dataclass, store migration, API, wizard, Settings page) is clean. We'd be happy to accept it, but we need the fix to work correctly across all supported inverter types before it lands. The logic layer is fully unit-testable without real hardware, so no AC-coupled device is required to close the gaps.

Required before merge

1. inverter_simulator._map_rates ignores external_solar_mode

_map_rates in core/bess/simulation/inverter_simulator.py (line 50) hard-codes SOLAR_STORAGE → (False, 0) and never reads settings.external_solar_mode. It is documented as a mirror of _map_intent_to_rates, and TestSimulatorMapRates exists precisely to keep them in sync — but it only covers LOAD_SUPPORT. With external_solar_mode=True the real hardware writes grid_charge=True while the savings simulator models the battery as idle for the same period.

Fix: add the same check to _map_rates, and add a SOLAR_STORAGE case to TestSimulatorMapRates.

2. GrowattSphController — feature is silently a no-op

_effective_grid_charge is called in display/info methods (get_period_settings, get_detailed_period_groups) so the UI correctly shows grid_charge=True for SOLAR_STORAGE when AC-coupled. But the hardware path never sees it: _group_sph_periods uses CHARGE_INTENTS = frozenset({"GRID_CHARGING"}) and _write_period_to_hardware is a no-op on SPH. An SPH user enabling the toggle gets a misleading UI and an unchanged inverter.

Fix — one change in _group_sph_periods:

effective_charge_intents = (
    self.CHARGE_INTENTS | {"SOLAR_STORAGE"}
    if self.battery_settings.external_solar_mode
    else self.CHARGE_INTENTS
)
# use effective_charge_intents instead of self.CHARGE_INTENTS in the loop

Note: SPH has a 3-period charge slot limit. If both GRID_CHARGING and SOLAR_STORAGE blocks are present the existing _enforce_period_limit will drop the shortest one. Worth a log warning or UI note.

No real hardware needed to test this — _build_sph_periods is pure Python:

def test_sph_solar_storage_becomes_charge_period_when_ac_coupled():
    ctrl = GrowattSphController(battery_settings=_settings(external_solar_mode=True))
    ctrl.strategic_intents = ["SOLAR_STORAGE"] * 96
    ctrl._build_sph_periods()
    assert len(ctrl._charge_periods) > 0

def test_sph_solar_storage_no_charge_period_when_dc_coupled():
    ctrl = GrowattSphController(battery_settings=_settings(external_solar_mode=False))
    ctrl.strategic_intents = ["SOLAR_STORAGE"] * 96
    ctrl._build_sph_periods()
    assert ctrl._charge_periods == []

3. Tests call a private method directly

test_external_solar_mode.py lines 33, 37, and the parametrize loop call ctrl._map_intent_to_rates(...) from outside the class. Project rule: never call _private methods from outside their class.

Fix: use ctrl.strategic_intents = ["SOLAR_STORAGE"] * 96 + the public ctrl.compute_rates_for_period(0, 0.0) instead (same pattern the two passing tests in the same file already use).

4. Class docstring is stale

inverter_controller.py line 28 still says SOLAR_STORAGE → grid_charge=False unconditionally. One-line update: SOLAR_STORAGE → grid_charge=False (True when external_solar_mode=True).


Design note — SolaX VPP

SolaX with external_solar_mode=True does charge during SOLAR_STORAGE hours (the VPP path is correct), but it charges at max_charge_power_kw regardless of actual solar surplus, which can cause grid import if production is below the cap. This is a pre-existing limitation of the VPP model rather than a gap in this PR specifically — a comment in the UI description or the PR body acknowledging it is enough for now.


The testing philosophy here: the hardware-specific schedule builders (_build_sph_periods, _map_rates, _map_intent_to_rates) are all pure Python — they can be fully tested by instantiating the controller class and asserting on the output. Live hardware validation is still needed for the "inverter actually responds correctly" layer, but the logic layer is 100% unit-testable without a device.

@jdungen

jdungen commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Great work Johan, and very nice to have the Modbus via solax nowadays. 🙏 I made it in a fork before but you made it nicer 😉

jdungen added a commit to jdungen/bess-manager that referenced this pull request Jun 25, 2026
Upstream has merged PR johanzander#164 (Nordpool continental areas), so the fork
now carries only the still-pending PR johanzander#167 (external_solar_mode).
Fork build wiring (image:, workflow registry owner, workflow_dispatch)
is reapplied on top of upstream 9.6.2.
jdungen added a commit to jdungen/bess-manager that referenced this pull request Jun 26, 2026
Bundle all jvdd-fork changes accumulated since rebase on upstream 9.6.2
under one minor-version label. No code changes vs jvdd.7:

  Features:
    - external_solar_mode (upstream PR johanzander#167, pending)

  Fixes:
    - AI Analyst model IDs updated + persisted-config auto-migration (PR johanzander#180)
    - SolaxModbus TOU begin/end write via time.* entity mirror (issue johanzander#181)
@jdungen

jdungen commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

What do you need for this?, it's working at my end. Is it different for other inverters? I thought it's a manual override for the schedule and sets the ac charge switch on modbus or api.

@johanzander

Copy link
Copy Markdown
Owner

I would need the review comments addressed and all test to pass.

On AC-coupled setups, switching grid_charge to True alone is not
enough: with the TOU slot in Load First mode, the inverter's EMS does
not actively initiate charging. The slot mode also needs to switch to
Battery First, which makes the inverter actively pull power from the
AC side during the planned solar window.

This commit:
- Adds _effective_mode_for_intent() mirroring _effective_grid_charge():
  returns 'battery_first' for SOLAR_STORAGE when external_solar_mode
  is enabled, otherwise the default mode.
- Applies it in inverter_controller.get_period_settings and
  get_detailed_period_groups (display paths), and in the three places
  the SolaxModbusGrowattController computes mode from intent.
- Adds 6 tests covering the override on SOLAR_STORAGE, default
  behaviour when disabled, no leakage to other intents, and propagation
  through get_detailed_period_groups.

Trade-off documented in the helper docstring: Battery First charges at
the configured rate regardless of actual solar surplus, so in a
SOLAR_STORAGE period with insufficient forecast accuracy the battery
will draw from grid. BESS only plans SOLAR_STORAGE when surplus is
expected, so the exposure is bounded by forecast quality. A future
follow-up could rate-limit the EMS charging rate to match measured
solar export, but that requires sensor data BESS does not currently
track at this granularity.

Live-verified on a Growatt MID 15KTL3-XH (SolaxModbus integration):
without this change SOLAR_STORAGE periods produced no battery action;
with this change battery charges actively during planned SOLAR_STORAGE
hours.
@jdungen

jdungen commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up commit on this branch: also override the battery mode (not just grid_charge) for SOLAR_STORAGE when external_solar_mode is enabled.

Why this matters in practice: I went live-testing the previous version of this PR (jdungen fork build) on a Growatt MID 15KTL3-XH (SolaxModbus integration). Even with grid_charge=True set on SOLAR_STORAGE periods, the battery did not charge at all. The reason: with the TOU slot still in Load First mode, the EMS waits for an internal trigger that on an AC-coupled inverter never comes (no DC solar to route to the battery). Switching the SOLAR_STORAGE TOU slot to Battery First makes the inverter actively pull from the AC side. Verified: 0 W → ~15 kW charging during the planned solar window after the change.

The change:

  • Adds _effective_mode_for_intent() alongside _effective_grid_charge(), returning battery_first for SOLAR_STORAGE when external_solar_mode is enabled.
  • Applies it in get_period_settings, get_detailed_period_groups, and the three SolaxModbusGrowattController mode-from-intent sites.
  • 6 new tests in test_external_solar_mode.py — total 17/17 green.

Documented trade-off in the helper docstring: Battery First charges at the configured rate regardless of actual solar surplus. If the forecast over-estimates solar in a SOLAR_STORAGE window, the inverter will pull from grid. The risk is bounded by forecast accuracy and the fact that BESS only plans SOLAR_STORAGE when surplus is expected. A future follow-up could rate-limit the EMS charging rate to match measured solar export, but that needs sensor data we don't currently track at this granularity.

I'm running this on my fork build now and will report back on whether real-world battery behaviour matches the planned SOLAR_STORAGE periods over the next sunny day or two.

jdungen added a commit to jdungen/bess-manager that referenced this pull request Jun 26, 2026
Upstream merged our PR johanzander#180 (AI Analyst model IDs) in 9.6.3, so that
patch is dropped from the fork diff. The fork now carries:

  - external_solar_mode (PR johanzander#167, still pending) — now with mode
    override in addition to grid_charge override
  - SolaxModbus TOU begin/end via time.* entity mirror (issue johanzander#181)

Bumped to 9.6.4-jvdd.1 to stay above upstream's 9.6.3 release.
@jdungen

jdungen commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

for example, this isn't working in the ac coupled setup. need battery first mode when solar storage strategic intent.

F9080F1B-66F0-478F-B66A-EE1AE5392995_1_101_o

@johanzander

Copy link
Copy Markdown
Owner

I am doing some fundamental changes to the algorithm and intent modes here: #187, that probably affects this PR. Lets follow up this one, after it has been merged and released.

@johanzander

Copy link
Copy Markdown
Owner

PR #187 has now merged, so this is ready to move forward. You'll need to rebase onto main and fix the Black formatting (that's the only CI failure blocking the merge gate).

One thing the rebase needs to handle: #187 introduced a passive solar charging path. IDLE periods where the optimizer chose action=0 but excess solar drifts into the battery now also get classified as SOLAR_STORAGE in the schedule. Your _effective_mode_for_intent would apply Battery First to these too — which on AC-coupled would override the optimizer's deliberate "don't actively charge" decision and pull from the AC bus at full rate. The fix is to guard on battery_action_kwh: only apply Battery First when the scheduled action is non-trivial (> 0.01 kW or similar). The value is already available in the controller from schedule.actions[period].

The intent classification and passive charging model are documented in docs/agents/bess-knowledge.md if you want the full picture.

Also curious to hear your real-world results — did battery behaviour match the planned SOLAR_STORAGE periods?

johanzander added a commit that referenced this pull request Aug 17, 2026
…e-argued (#636)

* feat: put PRs on the board so a decision about one is recorded, not re-argued

The board held issues only, so every judgement about a PR had nowhere to live.
"#167 and #354 are blocked", "#437 and #490 are lower priority, later" were
real decisions the maintainer had already made, and the rhythm pass re-reported
all four as due on every tick because nothing recorded them. The same
conversation happened every 30 minutes.

Projects v2 takes PRs as items with the identical field set, so the fix is
membership rather than a parallel mechanism. A PR card carries the same
`Priority` and `Awaiting` an issue card does; the digest emits them as
`pr_board`, and `backlog-rhythm.sh` joins by number and suppresses on them:
an `Awaiting` means parked on someone, `P4` means later-not-never.

`content.type` is what separates PR cards from issue ones, confirmed against a
real card rather than assumed — an added PR reports "PullRequest" with
number/title/url/repository alongside it. Numbers are unique across issues and
PRs in one repository, so this cannot collide with the existing issue lookup.

Suppressed PRs are COUNTED AND LISTED, never dropped: the pass ends with
`deferred: 4 (#490 priority P4; #167 awaiting discussion; ...)`. Silently
vanishing would trade one failure for another — the goal is to stop re-asking
about a settled decision, not to lose the item.

CONTRACT CHANGE: an APPROVED, green, still-draft PR is now its own action,
`mark_ready`, and it is the one thing no board decision can defer. It used to
hand back to `implement-issue` like any other unfinished draft, on the
principle that this pass must not grow a second review loop. That principle
still holds, but it is what left #629 sitting approved, green and draft: the
remedy on offer was a whole `implement-issue` session, and nobody spends one of
those to run a single command. `gh pr ready` is a terminal action, not a loop,
so naming it here duplicates nothing.

`awaiting_maintainer` is deliberately NOT carved out the same way. An approved
PR waiting on a merge is not broken; it is the maintainers call when to take
it, and P4 is exactly how they say later. #490 sat approved for a day and was
reported every tick as though that were news.

Live effect: 31 actions -> 27, with one `deferred: 4` line in place of four
recurring items, and #631 correctly still reported as merge-ready.

No CHANGELOG entry: agent tooling, no user-visible effect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR

* fix: mark_ready needs green checks, not just a clean merge

Found by running the rule against the live fleet on its first pass, which is
the only reason it was caught before the PR merged.

`mergeable` reports whether the branch merges cleanly and nothing else, so it
reads MERGEABLE while CI is still running or has failed outright. #633 was
APPROVED, MERGEABLE and had Algorithm tests and E2E still IN_PROGRESS, and the
rule duly reported "gh pr ready 633 — then it is the maintainers to merge".
GitHub itself disagreed: `mergeStateStatus` was BLOCKED.

Flipping a red or pending PR out of draft is worse than leaving it there.
`ready` is supposed to mean the maintainer can merge without checking anything
else, and that claim is the only thing making the flag worth setting.

So `mark_ready` now also requires every check to have concluded SUCCESS,
SKIPPED or NEUTRAL. SKIPPED is green on purpose: this repo path-filters
Algorithm tests and Docker build, so every backend-only PR skips them and
treating that as not-green would withhold the action from almost everything.
An empty rollup is green too — a PR with no checks configured has nothing
failing.

The deferred list mirrors the same condition, so an approved-but-pending PR
with a P4 card appears in exactly one place rather than both.

Live effect: #633 falls back to resume_implementation until its CI settles,
which is the correct answer and the one the first version got wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
johanzander added a commit that referenced this pull request Aug 18, 2026
The fixture tests could not see either of these. Both turned up on the
first live run.

1. GraphQL node limit. `--json commits` expands each commit's authors
   connection, so gh's cost estimate is limit x commits x authors. At
   --limit 100 that is 1,000,000 possible nodes and the query is rejected
   outright, so the command produced no output at all. --limit 30 keeps
   the worst case at 300,000. There is no cheaper field for "when did HEAD
   last move": `gh pr list --json` has no last-commit date, and a review's
   own commit SHA is REST-only. The cap is announced when hit rather than
   silently truncating, per sweep-prs.

2. Lazy `mergeable`, which is the dangerous one. The first query on a cold
   PR returns UNKNOWN *and* only then triggers the computation, so
   treating UNKNOWN as "not conflicted" hides precisely the stale PRs this
   script exists to surface.

   Measured: a first fleet run classified #167 and #619 with no conflict
   flag; once earlier queries had warmed them, the identical command
   returned `needs-refresh` for both. They were CONFLICTING the whole
   time. sweep-prs documents this trap and retries for the same reason;
   this reintroduced it.

   Now: re-ask while anything is UNKNOWN, and if it survives, render it as
   "(+mergeability UNKNOWN — re-run)" rather than letting it fall through
   to the clean branch.

Verified by mutation: forcing $mergeUnknown to false reddens
test_unknown_mergeability_is_never_reported_as_clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@johanzander
johanzander marked this pull request as draft August 18, 2026 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add external_solar_mode battery setting for AC-coupled PV systems

2 participants