Add a steam boiler pool - #1429
Conversation
github-actions
Bot
added
part:docs
shsms
changed the title
8cda58a to
ba9e618
Compare
cwasicki
left a comment
cwasicki
left a comment
There was a problem hiding this comment.
Any advice what to review here? On a first glimpse this looks like a lot of boiler-plate code.
| Metric.AC_REACTIVE_POWER_PHASE_3: lambda msg: msg.reactive_power_per_phase[2], | ||
| } | ||
|
|
||
| _STEAM_BOILER_DATA_METHODS: dict[ |
There was a problem hiding this comment.
Why do we need to repeat these for every device?
There was a problem hiding this comment.
"historical reasons". This comes from the old days where the API have one message per component for the data. Now it just needs modernization. Hopefully soon™️.
There was a problem hiding this comment.
It would be a lot of work to do it differently, which is not worth it, because this stuff will all be rewritten in rust in the next several months.
The big things to check are:
I'll need to do some testing as well. |
cwasicki
left a comment
cwasicki
left a comment
There was a problem hiding this comment.
LGTM, the PR description mentions a wrong default though.
github-project-automation
Bot
moved this from To do
to Review approved
in Python SDK Roadmap
There was a problem hiding this comment.
Oh, damn. I sill need to remove all this cr*p... 🤦
There was a problem hiding this comment.
Maybe you can forget about this for now. There's a big cleanup coming soon anyway, with the moving of the algorithmic parts to be behind the microgrid API.
| Metric.AC_REACTIVE_POWER_PHASE_3: lambda msg: msg.reactive_power_per_phase[2], | ||
| } | ||
|
|
||
| _STEAM_BOILER_DATA_METHODS: dict[ |
There was a problem hiding this comment.
"historical reasons". This comes from the old days where the API have one message per component for the data. Now it just needs modernization. Hopefully soon™️.
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
🤖 rAIview (AI review here!)
The overall structure is consistent with the existing component pools. I found three concrete control-path issues around failure feedback, stale bounds when all boilers become unavailable, and reversed reactive-power documentation. I also left two questions about whether the deliberately simple allocation and exclusion-bound behavior should be accepted and documented for this initial version.
| inclusion_bounds = Bounds( | ||
| lower=Power.from_watts( | ||
| sum( | ||
| data.active_power_inclusion_lower_bound | ||
| for data in self._latest_component_data.values() | ||
| ) | ||
| ), | ||
| upper=Power.from_watts( | ||
| sum( | ||
| data.active_power_inclusion_upper_bound | ||
| for data in self._latest_component_data.values() | ||
| ) | ||
| ), | ||
| ) | ||
| exclusion_bounds = Bounds( | ||
| lower=Power.from_watts( | ||
| sum( | ||
| data.active_power_exclusion_lower_bound | ||
| for data in self._latest_component_data.values() | ||
| ) | ||
| ), | ||
| upper=Power.from_watts( | ||
| sum( | ||
| data.active_power_exclusion_upper_bound | ||
| for data in self._latest_component_data.values() | ||
| ) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🟡➡️ Should aggregate bounds and allocation handle each boiler's exclusion zone?
The tracker sums exclusion bounds across boilers, while the distributor considers only each boiler's inclusion bounds. Is this simplification intentional while we gather operational data and choose the final distribution algorithm?
For example, if two boilers allow [0, 0] ∪ [2, 10] kW and [0, 0] ∪ [4, 10] kW, the aggregate exclusion zone becomes 0..6 kW. A 7 kW target is accepted, but equal allocation can send 3.5 kW to both boilers, which is inside the second boiler's exclusion zone. Conversely, a feasible 5 kW target can be rejected even though the first boiler could handle it alone.
Impact
The initial algorithm can reject feasible aggregate targets or send a per-boiler target that the API rejects. If this is a deliberate first approximation, documenting that constraint would make the intended scope explicit.
Related locations
_steam_boiler_manager.py:181-205: Allocation uses inclusion bounds but not exclusion bounds.
Suggested tests
Add a heterogeneous exclusion-bounds case to record the intended behavior, whether the current approximation is accepted or the allocator is changed.
🟡 Medium Severity: Probably best to address before merging, or explicitly accept and document the initial limitation.
➡️ Medium Effort: A complete solution requires choosing how non-convex per-device ranges should be aggregated and allocated.
There was a problem hiding this comment.
When we discussed, we decided to keep it simple and distribute equally and consider later if we want more complicated algorithms.
We don't know that there will be multiple steam boilers, if they will have exclusion bounds, and what the correct algorithm is, based on what metrics etc.
| allocated_power = min( | ||
| remaining_power, | ||
| remaining_power / float(num_components - idx), | ||
| upper_bound, | ||
| ) | ||
| # A boiler with a minimum operating power can't run below it: raise | ||
| # the allocation to the minimum when the remaining power covers it, | ||
| # otherwise keep the boiler off. Power that a kept-off boiler's | ||
| # share would have used is not redistributed to earlier boilers; | ||
| # it is reported as excess power in the result. | ||
| lower_bound = Power.from_watts( | ||
| component_data.get().active_power_inclusion_lower_bound | ||
| ) | ||
| if Power.zero() < allocated_power < lower_bound: | ||
| allocated_power = ( | ||
| lower_bound | ||
| if lower_bound <= upper_bound and remaining_power >= lower_bound | ||
| else Power.zero() | ||
| ) | ||
| allocations[comp_id] = allocated_power | ||
| remaining_power -= allocated_power |
There was a problem hiding this comment.
🟡➡️ Is intentionally stranding feasible power part of the initial simple algorithm?
This single pass can leave a feasible target mostly unallocated. With boilers supporting 0..1 kW and 4..10 kW, a 4.5 kW target is reported as 1 kW allocated and 3.5 kW excess, although 0.5 + 4 kW satisfies the target. The new test_unaffordable_minimum_strands_excess explicitly preserves this behavior.
Is this intentional as part of starting with a simple distribution algorithm while gathering operational data, or is it an oversight? If intentional, it would be useful to document the tradeoff because callers may otherwise interpret excess power as unavailable capacity.
The result can also depend on iteration order when boilers have equal upper bounds, because the stable sort starts from a set. Visiting a high-minimum boiler first can fulfill the request, while visiting it later can strand power.
Impact
The pool can under-consume significantly despite having enough available capacity, and tied bounds can make fulfillment dependent on component ordering.
Related locations
test_steam_boiler_manager.py:229-265: The test preserves the known limitation.
Suggested tests
Add permutations with tied upper bounds to make the chosen order and expected approximation deterministic.
🟡 Medium Severity: Probably best to clarify before merging because this affects observable control behavior.
➡️ Medium Effort: Clarifying and documenting the behavior is small; improving allocation requires a broader algorithm decision.
There was a problem hiding this comment.
This also, I would not worry about now.
| remaining_power=remaining_power, | ||
| component_category="steam boiler", | ||
| ) | ||
| await self._results_sender.send(result) |
There was a problem hiding this comment.
🟡⬇️ Forward set-power failures to the status tracker
_set_component_power() can return a PartialFailure, but the manager only publishes the result. It never calls ComponentPoolStatusTracker.update_status(), so the new SteamBoilerStatusTracker._handle_set_power_result() path is unreachable.
Impact
A boiler whose API request failed remains WORKING and can be selected again immediately instead of being marked UNCERTAIN and temporarily excluded.
Suggested fix (⬇️ effort)
Pass the succeeded and failed component sets from the result to self._component_pool_status_tracker.update_status() before publishing the result, as the battery manager does.
Suggested tests
Fail one boiler's API request while another succeeds, then verify that the failed boiler becomes UNCERTAIN and is excluded from the next allocation.
🟡 Medium Severity: Probably best to fix before merging because the status tracker's failure recovery currently cannot run.
⬇️ Low Effort: The status tracker and result data already exist; only the forwarding call and regression test are missing.
There was a problem hiding this comment.
I guess at some point we stopped caring about the distributor results. As long as there are meaningful logs, operations and debugging are no problem.
It is like this in PV and EV as well. Currently the steam boiler is based on the PV pool for many things. If we want to fix it, I think we can do it separately covering the other pools as well.
| async def _send_bounds(self) -> None: | ||
| """Calculate and send the aggregate system bounds if they have changed.""" | ||
| if not self._latest_component_data: | ||
| return |
There was a problem hiding this comment.
🟡➡️ Clear bounds when the last boiler becomes unavailable
After the final cached component is removed, _send_bounds() returns without publishing a replacement. Consumers and the power manager therefore retain the previous nonzero capacity.
Impact
power_status continues to advertise stale capacity. A subsequent request reaches the distributor with no usable boilers, where it only logs and returns, so power_distribution_results emits nothing for that request.
Related locations
_steam_boiler_manager.py:154-159: Requests are dropped without a result when no boiler is usable.
Suggested fix (➡️ effort)
Publish unavailable system bounds when the cache becomes empty. Also emit an Error, or an explicit result with all requested power as excess, when no boiler can accept the request.
Suggested tests
Start with one working boiler, transition it to NOT_WORKING, verify that its old capacity disappears, then propose power and assert that a distribution result arrives.
🟡 Medium Severity: Probably best to fix before merging because stale capacity can persist indefinitely.
➡️ Medium Effort: The state transitions are straightforward, but the desired empty-bounds and no-result semantics need to be selected consistently.
There was a problem hiding this comment.
This corresponds with PV and it doesn't matter, because the component becomes NOT_WORKING, and then any other param is irrelevant.
| reactive_power: float = 0.0 | ||
| """The total reactive 3-phase AC power, in Volt-Ampere Reactive (VAr). | ||
|
|
||
| * Positive power means capacitive (current leading w.r.t. voltage). | ||
| * Negative power means inductive (current lagging w.r.t. voltage). | ||
| """ | ||
|
|
||
| reactive_power_per_phase: PhaseTuple = (0.0, 0.0, 0.0) | ||
| """The per-phase AC reactive power, in Volt-Ampere Reactive (VAr). | ||
|
|
||
| The provided values are for phase 1, 2, and 3 respectively. | ||
|
|
||
| * Positive power means capacitive (current leading w.r.t. voltage). | ||
| * Negative power means inductive (current lagging w.r.t. voltage). |
There was a problem hiding this comment.
🟡⬇️ Correct the reactive-power sign convention
These descriptions reverse the Microgrid API convention. The API defines negative reactive power as capacitive, with current leading voltage, and positive reactive power as inductive, with current lagging voltage. Both the total and per-phase docstrings currently say the opposite.
API reference: microgrid.proto:183-187.
Impact
Users can interpret measured reactive power with the wrong physical direction and build control or reporting logic around the reversed convention.
Suggested fix (⬇️ effort)
Change both docstrings to say that negative is capacitive and positive is inductive.
🟡 Medium Severity: Probably best to fix before merging because this documents a public data type with the opposite convention from the API.
⬇️ Low Effort: This requires changing four documentation lines.
There was a problem hiding this comment.
I guess we're contradicting the proto everywhere. in the old_component_data.py file, not just here.
Besides, both the proto and the SDK docs are completely wrong about the meaning of reactive power signs. So both need to be fixed, but separate project I guess.
Steam boilers are controllable electrical loads. Their active power and its inclusion/exclusion bounds, together with the per-phase power, current, voltage and frequency, are reported through the corresponding AC_* metrics. SteamBoilerData derives directly from ComponentData and declares its own fields and from_samples/to_samples handling, rather than reusing another category's type, so steam boilers are not coupled to the CHP data model. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Adds the timeseries.steam_boiler_pool package: the SteamBoilerPool, its reference store and report types, and a SteamBoilerSystemBoundsTracker that aggregates the active-power bounds of the working boilers, mirroring the PV pool. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The data sourcing actor handles the steam boiler component category so the power formula can resample per-boiler power. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The SteamBoilerStatusTracker reports per-boiler health so that broken boilers can be excluded from power distribution, mirroring the PV inverter status tracker. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The SteamBoilerManager splits the target power equally across the working boilers. Each allocation stays inside the boiler's inclusion bounds: a share below a boiler's minimum operating power is raised to the minimum when the remaining budget covers it, and the boiler is kept off otherwise. Like the PV manager, the measured draw of unreachable boilers is subtracted from the target, so the working boilers don't make the site overshoot it. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Wires the steam boiler pool into the data pipeline factory, using the plain matryoshka power-manager algorithm with DefaultPower.ZERO, so that unmanaged boilers are switched off and fall back to gas heating, and documents the new pool in the microgrid module docs. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Adds SteamBoilerDataWrapper, steam boiler streaming in the mock microgrid, resampling in the mock resampler, and graph-generator support. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Tests for the steam boiler pool power formula (including its meter fallback) and the power control methods. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Tests for the SteamBoilerManager's distribution (minimum-power and unreachable-power handling, formula delegation, shutdown) and the status tracker. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
ba9e618 to
b7d7402
Compare
3 participants
Steam boilers are controllable electrical loads the SDK could not monitor or
manage. This adds a steam boiler pool with power readings and power control,
following the structure of the EV charger and PV pools.
Changes
microgrid.new_steam_boiler_pool()returning aSteamBoilerPoolwithpower,power_status, and a consumption-onlypropose_power(negative power is rejected).SteamBoilerDatacomponent data type and data sourcing support.DefaultPower.ZEROwhen no actor is proposing.SteamBoilerManagersplits the target power equally across the workingboilers, keeping every allocation inside the boiler's inclusion bounds: a
share below a boiler's minimum operating power is raised to the minimum, or
the boiler is kept off. Like the PV manager, it subtracts the measured draw
of unreachable boilers from the target.
SteamBoilerStatusTrackerfor per-boiler health, mirroring the PV tracker.Worth a look
boiler can't be afforded instead of topping up earlier boilers
(
test_unaffordable_minimum_strands_excesspins this).same behavior as the PV and EV managers; a follow-up could make all three
send an
Errorresult the way the battery manager does.