Track broken components in BatteryPool and PowerDistributor(MicrogridPowerManagement)

[ela-kotulska-frequenz]

BatteryPool is another kind of LogicalMeter that stores pool of batteries, subscribe for its metrics and returns overall (sum) of metrics for the pool. Some metrics like: active_power should be stream metrics.
Some metrics like power_upper_bound or capacity should be returned on demand. As it makes no sense to return them with frequency.
It should be possible to create many instances of BatteryPool.
Question: Should it be possible to create BatteryPool with only 1 battery? Or we will have separate instance for Battery?

BatteryPowerDistributor - should control the power that is set to the microgrid batteries. In future will be extended with PowerManagerActor

Both BatteryPowerDistributor and BatteryPool should use only working batteries.

Separate instance: BatteryStatus should track what batteries are working.
BatteryStatus should be Singleton that stores status of all battery-inverter pairs in the microgrid.

Implementation proposition:

class Sample:
   # Single sample from the MicrogridResampleActor
   timestamp: datetime
   value: float

Class BatteryPool:
    components: Dict[battery_id, inverter_id]

    # stores latest metrics for each component separately
    latest_metrics : Dict[str(metric_name), Dict[component_id, Sample]]
    
    # list of all supported stream metrics e.g: active_power
    supported_stream_metrics: Set[str]
    
    # list of all supported metrics that are not streams e.g.: overall_capacity, power_upper_bounds
    supported_metrics: Set[str] 

    def __init__(self, api: MicrogridApi, batteries_ids: Set[int], stream_metrics: Dict[str, Sender[Sample]], frequency_sec: float, metrics: Set[str]):
        # Get ids of batteries and adjacent inverters and create self.component.
        # Subscribe for the given metrics for each battery-inverter pair.
        self.frequency_sec = frequency_sec


    def run(self):
	# Wait for data from metrics stream and update latest_metrics.
	# Every self.frequency_sec check BatteryStatus and compute necessary metrics for all working 
        # batteries. Push metrics to user with current timestamp.
	# Note: The result timestamp will be different then the resampler timestamp.
  
   def batteries_capacity(self) -> float:
      # returns capacity of the pool

   async def set_power(self, value: int) -> sdk.power_distribution.Result : 
      # charge / discharge the batteries pool
      # Calls power_distributor to charge/discharge batteries ( In the future calls powerManagerActor )

class BatteryStatus:
def __init__(self, api: MicrogridApi):
        # Subscribe for the metrics below for internal use
        metrics = [".result.inverter.state.componentState",  ".result.battery.state.componentState", ".result.battery.state.relayState" ]
        metric_channels = # For each battery-inverter pair in Microgrid: Subscribe for `self.metrics`, each channel should be peekable. 

def get_working_batteries(self, batteries_pool: Set[int]) -> Set[battery_id]:
   # Returns set of working batteries

   # Battery is consider as broken if:
   #    * inverter state in [UNSPECIFIED, UNAVAILABLE]
   #    * battery relay in [CLOSED]
   #    * battery state in [ERROR]
   #    * component didn't send any data for some time ( how long? it should be configurable )
   #    * latest error level returned from `set_power` method is Critical.
   #    * battery-inverter didn't respond for the latest `set_power` call
   # If battery is considered as NOT working, then change it to working if:
   #    * state of the inverter or battery changed - valid state
   #    * RelayState or state of the battery changed
   #    * some time has passed (increase exponentially until reach 30 sec - this should be configurable)
   #    * component started to send data
  
   # If all batteries given in argument are considered as broken, then relax the conditions to below:
   # Battery is consider as broken if:
   #    * inverter state in [UNSPECIFIED, UNAVAILABLE]
   #    * battery relay in [CLOSED]
   #    * battery state in [ERROR]
   #    * component didn't send any data for some time

class PowerDistributor
   def run(self):
      # Wait for the set of batteries to be charged/discharged.
      # Check which are working (using BatteryStatus) and make operation on working batteries only.
      # After `charge` / `discharge` operation pass status of each battery-inverter to BatteryStatus.
      # Return to the user if operation failed/succeed and how much requested power was charged/discharged.

[leandro-lucarella-frequenz]

I have a few naming suggestions (or course, it is much easier to nitpicking about names than think about the overall design 😆 ):

• BatteryPool.components -> inverters or battery_inverter (something that it's more explicit about mapping batteries to their inverters)
• BatteryPool.frequency_sec -> period_s (is not really a frequency, frequency is X times per second and also it matches the resampling actor that also should use period_s)

Now about the design:

• If this is an actor, it should be named accordingly: Maybe BatteryPoolAggregatingActor.

  This opens up a much bigger question. I think we don't have an option here, because if a battery pool will send charge/discharge commands, it needs to be centralized, we can't have one process sending a charge command and another a discharge. Or even the topic of overlapping batteries in different pools.

  This is pretty much related to the power manager, who will arbitrate between different charge/discharge commands on batteries. We need some coordination to operate over shared resources like batteries.

• Consider subscribing to stuff in run() instead of __init__. Or better create a initialize() (should do nothing if called twice) and call it from the run() so users can decide when to initialize, but if they forget it will be initialized when it starts running.

• Why specifying a new frequency/period to stream results and not just stream everything that comes in as soon as you accumulated all the samples for a single timestamp? That was more or less the main point of having the resampler.

  This opens a bigger question/issue. At some point the idea was that anything can be plugged to any "source", so for example a battery pool could be plugged to the resampled data or to the raw microgrid API source. But if we do the later is not guaranteed that all samples will come with the same timestamp so we would we need to duplicate all the logic of the resampler, which doesn't make any sense!

  So I think we should limit higher level actors like the battery pool to consume from resampled data.

• We mentioned this during some design meeting when you were on vacation, but probably metrics like battery capacity should be event-based. I.e. they should be streams, but instead of sending periodically a value that hardly ever changes, it sends the value only when it changed. So we should save the current value and then send a new value only when it was different from the previous value we sent.

  This makes me also think that these kind of metrics should probably not go through the resampler, so maybe is not one or the other data source, but both depending on the metric!

• I would make all the metrics lazy, as we wanted to do with the resampling actor, so we shouldn't subscribe to all metrics but instead only subscribe when somebody asks to subscribe to a particular aggregation.

  Then it wouldn't be necessary to pass the metrics in the constructor. Again, assuming this is an actor, not a plain class.

OK, I only reviewed the BatteryPool, I will probably come back with more comments on the rest later.

PS: I too the liberty to edit your original message to add py to the code blocks so they are syntax highlighted :)

[ela-kotulska-frequenz]

Hi @leandro-lucarella-frequenz , thanks for the comments.

If this is an actor, it should be named accordingly: Maybe BatteryPoolAggregatingActor.

Ok. I didin't pay much attention to the names at this step. But I will the next time.

Consider subscribing to stuff in run() instead of init. Or better create a initialize() (should do nothing if called twice) and call it from the run() so users can decide when to initialize, but if they forget it will be initialized when it starts running.

Ok - It is anyway not possible to subscribe for data in init. But fault.

Why specifying a new frequency/period to stream results and not just stream everything that comes in as soon as you accumulated all the samples for a single timestamp? That was more or less the main point of having the resampler.

It is just harder to do, but I can try. The Resampler would send NaN if component didn't send any data, yes? Maybe we could consider sending Tuple[datetime, NaN]. Otherwise I would have to guess what timestamp is for given NaN.

We mentioned this during some design meeting when you were on vacation, but probably metrics like battery capacity should be event-based. I.e. they should be streams, but instead of sending periodically a value that hardly ever changes, it sends the value only when it changed. So we should save the current value and then send a new value only when it was different from the previous value we sent.

Ok. Good idea. It should be implemented in Resampler, yes?
Battery / inverter state and relayState could also be event stream.

I would make all the metrics lazy, as we wanted to do with the resampling actor, so we shouldn't subscribe to all metrics but instead only subscribe when somebody asks to subscribe to a particular aggregation.

You mean that it should use ChannelRegistry, yes?

PS: I too the liberty to edit your original message to add py to the code blocks so they are syntax highlighted :)

Brave act! Thanks. :)

[leandro-lucarella-frequenz]

The Resampler would send NaN if component didn't send any data, yes?

Yes, the resampler will always send data for all components for every period, and always with a timestamp, so consumers will receive a NaN with a timestamp. There is actually an issue now to deal with this with some utility because it will affect all consumers of the resampling actors, all will have to wait for all the metrics corresponding to one particular timestamp.

• Add a class that manages a buffer to get all the needed resampled data #31

We mentioned this during some design meeting when you were on vacation, but probably metrics like battery capacity should be event-based. I.e. they should be streams, but instead of sending periodically a value that hardly ever changes, it sends the value only when it changed. So we should save the current value and then send a new value only when it was different from the previous value we sent.

This makes me also think that these kind of metrics should probably not go through the resampler, so maybe is not one or the other data source, but both depending on the metric!

Ok. Good idea. It should be implemented in Resampler, yes?

That's a good question. I think we the current design we shouldn't, because the resampler should always return all the metrics for every period (time window), otherwise we run into the NaN problem above. For now I would do it only at the *Pool level, even when it is not ideal from the PoV of performance, as we'll keep resampling that data for no good reason. At some point I think it should be a separate actor or maybe just an utility function/class that works side-by-side with the resampler, that gets the data directly from the data sourcing actor.

For the first version maybe it would be better to just go with your solution that it is much simpler and maybe enough for now? We can find this out.

You mean that it should use ChannelRegistry, yes?

Yes, I think we should probably have a quick discussion about this on the next SDK devs meeting, to make sure we are all on the same page on how this subscription mechanism will work as it will be done by several actors.

Brave act! Thanks. :)

💪

[leandro-lucarella-frequenz]

We talked with Tom about the event based streams and agreed for now that would only be an optimization, so we are not doing it for now. But if we do it in the future it surely doesn't belong to the resampling actor. For now actors that need a more event based approach can do it themselves.

As for the original topic, the batteries_capacity() function (*), if we have an use case for it I would keep it (but the pool will still receive the data from the resampling actor, or the microgrid API directly if it could make more sense). If not I would drop it until we need it.

(*) I would make it a property and call it just capacity as it is already a battery pool so it should be self explanatory that is the capacity of the batteries in the pool.

[ela-kotulska-frequenz]

As for the original topic, the batteries_capacity() function (*), if we have an use case for it I would keep it

Lets ask about it on the sdk users meeting :)

[leandro-lucarella-frequenz]

Do we have a conclusion for this discussion @ela-kotulska-frequenz ?

I kind of lost track of it and I guess this is what you are working on so you should know. If there is a conclusion, can you write it down in a new comment and mark it as the answer?

[ela-kotulska-frequenz]

It is already solved. BatteryStatus is tool fro tracking broken components.
There is one instance of the BatteryStatus in the SDK. This instance is handled by PowerDistributingActor.
This is like this because BatteryStatus needs results from the power requests (power commands that PowerDistributor sends to microgrid).
BatteryStatus will send status to all subscribed users using channel communication.