Control methods for EVChargerPool using PowerManager and PowerDistributor
#900
Conversation
github-actions
bot
added
part:docs
|
|
||
| # Send initial status | ||
| await self._status_sender.send( | ||
| ComponentStatus(self._component_id, self._last_status) |
There was a problem hiding this comment.
Hmm this means we will always send the default NOT_WORKING initially before correcting it no? Why not make default None and don't send any initial status, just let the loop update it?
There was a problem hiding this comment.
Worst case, the timer will fire and send the update
There was a problem hiding this comment.
Yes, but that would require some amount of refactoring, because the timer trigger would send something out only if the status has changed. But I'm not sure that it should be changed.
This is an internal feature, and its values are used by the power distributor and the system bounds tracker. So having it like this in practice would mean that PowerManager can report zero bounds right away on start up, until data is available.
There was a problem hiding this comment.
Yes, but that would require some amount of refactoring, because the timer trigger would send something out only if the status has changed
But something has changed if _last_status is None as that is different from the new_status which is set to not working
There was a problem hiding this comment.
This is used as a hint to the power manager and *pool metrics whether a battery is usable or not. We need to send something out right at the beginning, else they wait for a status to arrive, before streaming values out.
So changing this would mean changing all the users of this data, which is more than a few.
Sending the NOT_WORKING flag even though we don't know if it is working or not is I'm guessing the objection? Maybe renaming it to NOT_USABLE would make it more general, and can be sent before we receive any data, or when missing data, or when the component is known to be not working?
There was a problem hiding this comment.
In the battery pool too, it doesn't seem to say "NOT_WORKING" at startup, just:
INFO:frequenz.sdk.actor.power_distributing._component_status._battery_status_tracker:battery 1001 changed status ComponentStatusEnum.WORKING
INFO:frequenz.sdk.actor.power_distributing._component_status._battery_status_tracker:battery 1004 changed status ComponentStatusEnum.WORKING
There was a problem hiding this comment.
Without having seen the changes yet, and not being very familiar with how this is used (so high risk of saying nonsense), how about INITIALIZING instead of UNKNOWN, if we can use a special state for this initialization step?
There was a problem hiding this comment.
I guess related to this, reading the commit message:
It reports an EV charger as
WORKINGwhen an EV is connected to it,
and power can be allocated to it, andNOT_WORKINGotherwise.
I guess this status comes from batteries, right? Maybe it would be better to rename WORKING/NOT_WORKING to USABLE/UNUSABLE or AVAILABLE/UNAVAILABLE, it looks a bit misleading to say a EV charger is "not working" just because an EV is not connected to it.
There was a problem hiding this comment.
It could also be because of EV charger in error state. Then someone might have to go and reset.
This used for telling the Power Distributor whether power can be allocated to a component or not. And we need alternatives for WORKING, NOT_WORKING, UNCERTAIN that would be suitable for batteries, EV chargers, PV inverters, CHP, etc.
Maybe USABLE/NOT_USABLE is the way to go. UNUSABLE is not distinct enough for my eyes, I might mix things up, especially when reading.
tests/actor/power_distributing/_component_status/test_ev_charger_status.py
Show resolved
Hide resolved
src/frequenz/sdk/actor/power_distributing/_component_managers/_battery_manager.py
Show resolved
Hide resolved
|
|
||
| @override | ||
| async def stop(self) -> None: | ||
| """Stop the ev charger data manager.""" |
There was a problem hiding this comment.
There is no stopping the data manager?
There was a problem hiding this comment.
Oops, this is supposed to stop the component pool status tracker. I've added a fixup commit.
.../sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py
Show resolved
Hide resolved
.../sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py
Show resolved
Hide resolved
.../sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py
Show resolved
Hide resolved
| ) | ||
| await self._results_sender.send(result) | ||
|
|
||
| async def _set_api_power( |
|
I've added some fixup commits with the missing docstrings, so they're easy to review. Will squash them before merging. |
src/frequenz/sdk/timeseries/ev_charger_pool/_system_bounds_tracker.py
Outdated
Show resolved
Hide resolved
| !!! note | ||
| When specifying priority, bigger values indicate higher priority. The default | ||
| priority is the lowest possible value. |
There was a problem hiding this comment.
What if the priority is equal? Which one will then be selected?
There was a problem hiding this comment.
If there are multiple proposals with the same priority, they will be treated as having different priorities based on their randomly generated source_id. So the priority between actors will be stable and they won't get into a race with each other when they have the same priority values.
There was a problem hiding this comment.
Maybe add a short form or hint of htat to the docu here :)
There was a problem hiding this comment.
A small side note, in docstrings you can use normal sections for admonitions:
Note:
When specifying priority, bigger values indicate higher priority. The default ...
You only need the "special" syntax if you need some customization (like if you don't want it to be collapsible or to show it on the side for example).
|
(more for myself) I have currently reviewed up to Add a propose_power method to EVChargerPool |
|
FYI, starting a review. |
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
Besides the comments (most minor), really nice job!
src/frequenz/sdk/actor/power_distributing/_component_status/_ev_charger_status_tracker.py
Show resolved
Hide resolved
tests/actor/power_distributing/_component_status/test_ev_charger_status.py
Show resolved
Hide resolved
src/frequenz/sdk/actor/power_distributing/_component_status/_ev_charger_status_tracker.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/actor/power_distributing/_component_managers/_battery_manager.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_states.py
Outdated
Show resolved
Hide resolved
|
|
||
| @property | ||
| def bounds(self) -> Bounds[Power] | None: | ||
| """The usable bounds for the batteries. |
There was a problem hiding this comment.
| """The usable bounds for the batteries. | |
| """The usable bounds for the EV chargers. |
| priorities. | ||
| There might be exclusion zones within these bounds. If necessary, the | ||
| [`adjust_to_bounds`][frequenz.sdk.timeseries.battery_pool.BatteryPoolReport.adjust_to_bounds] |
There was a problem hiding this comment.
I guess this adjust_to_bounds should be present in the EVChargerPoolReport and this should be fixed to reference that instead?
There was a problem hiding this comment.
I was wondering if we need it for EV chargers, because the bounds are likely to be simple, and there won't be exclusion bounds for EV chargers.
Also @cwasicki was looking for ways to make sure we don't send out zero power repeatedly, and I was wondering if adjust_to_bounds is a good way to achieve that. We'll have to do some brainstorming probably next week, and maybe change the adjust_to_bounds interface in the BatteryPool too.
For now, I will remove these comments.
tests/timeseries/_ev_charger_pool/test_ev_charger_pool_control_methods.py
Show resolved
Hide resolved
tests/timeseries/_ev_charger_pool/test_ev_charger_pool_control_methods.py
Show resolved
Hide resolved
tests/timeseries/_ev_charger_pool/test_ev_charger_pool_control_methods.py
Show resolved
Hide resolved
|
LGTM as well |
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
It reports an EV charger as `WORKING` when an EV is connected to it, and power can be allocated to it, and `NOT_WORKING` otherwise. If it receives a power assignment failure from the PowerDistributor, when the component is expected to be working, it is marked as `UNCERTAIN` for a specific interval, before being marked `WORKING` again. This status would be used in the PowerDistributor and in the EVChargerPool. Signed-off-by: Sahas Subramanian <[email protected]>
Until now, the PowerDistributingActor has supported only distributing power to batteries. Batteries are (supposed to be) always available, so the distribution takes place once when the power distributor receives a request. This changes when distributing power to EV chargers, because power needs to be: - distributed when an EV is connected - redistributed to other EVs when an EV is disconnected - redistributed to other EVs if an EV appears to be not charging, etc. So a power distribution manager for EVs would be best implemented with its own `select` loop, where distribution happens. Because of this, a call to the `distribute_power` method on the EV charger manager can't immediately return a single distribution result, but that call is not the only thing that triggers the distribution, but other events too. So the manager needs to be what sends out the distribution results. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
This is exposed through the private `_system_power_bounds` method in the EVChargerPool. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
This allows the efficient creation of multiple `EVChargerPool` instances for the same set of EV chargers, but with different priorities, while reusing the formulas and bounds by having only a single instance of the `EVChargerPoolReferenceStore` class for any given set of EV chargers. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Additional tests for EV charger pool control methods can go in that directory as well. Signed-off-by: Sahas Subramanian <[email protected]>
Also remove the default value from the PowerDistributor constructor, which allows the value to be mocked/overridden before a PowerDistributor instance is created. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Co-authored-by: Leandro Lucarella <[email protected]> Signed-off-by: Sahas Subramanian <[email protected]>
|
I've applied the fixups. |
3 participants
This PR adds new
propose_powerandpower_statusmethods to theEVChargerPoolsimilar to theBatteryPool. These method interface with thePowerManagerandPowerDistributor, which currently uses a first-come-first-serve algorithm to distribute power to EVs.The following changes were made:
EVChargerStatusTrackerimplementation which tracks the health of an EV charger and whether an EV is connected.PowerDistributorandPowerManagerare now modular to support multiple component types.EVChargerManagerthat plugs into thePowerDistributorPowerDistributorandPowerManagerto theEVChargerPool.Fixes #872.