Skip to content

Make PowerManagingActor more modular and generic #773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Nov 9, 2023

Conversation

shsms
Copy link
Contributor

@shsms shsms commented Nov 6, 2023

This PR updates the PowerManagingActor to

  • take type of components we are controlling. Currently only batteries
    are supported.
  • Remove all other references to batteries from the
    PowerManagingActor, except in the interactions with the
    PowerDistributingActor.

The PowerDistributingActor needs to be modularized as well, before
the either of these actors can be used with other types of components.

All changes in this PR are internal and don't affect the external
interface of the BatteryPool.

shsms added 6 commits November 6, 2023 16:08
Because it an internal type that only holds system bounds.  It can be
used for other pools also, so it will be moved to a separate module in
a subsequent commit.

Signed-off-by: Sahas Subramanian <[email protected]>
This allows the type to be used by the other component pools and
reduces the PowerManagingActor's dependence on the battery_pool
package.

Signed-off-by: Sahas Subramanian <[email protected]>
And replace them with the word `component`, so that it can be used as
a generic report request type for all pools.

Signed-off-by: Sahas Subramanian <[email protected]>
This allows the generic `Report` class to be typed in the BatteryPool as
`BatteryPoolReport` with battery-related documentation.

This commit also removed mentions of batteries from the
`PowerManager.Report` class docstrings.

Signed-off-by: Sahas Subramanian <[email protected]>
This is in preparation for using the PowerManagingActor with different
component pools.

The power manager needs to get system bounds for the given set of
components from the right place, and only when necessary.  This
requires telling the power manager which pool to get bounds from, so
that it can do it dynamically.

Signed-off-by: Sahas Subramanian <[email protected]>
The only ones that remain are in the interactions with the
PowerDistributingActor, which will be made general purpose separately.

Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Nov 6, 2023
@shsms shsms self-assigned this Nov 6, 2023
@shsms shsms requested a review from a team as a code owner November 6, 2023 15:22
@shsms shsms requested a review from llucax November 6, 2023 15:22
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline part:actor Affects an actor ot the actors utilities (decorator, etc.) part:microgrid Affects the interactions with the microgrid labels Nov 6, 2023
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, but I have a few design concerns.

shsms added 2 commits November 8, 2023 13:04
This is supposed to establish a link between the classes, making it
easy to understand the code.

Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms requested a review from llucax November 8, 2023 12:21
llucax
llucax previously approved these changes Nov 8, 2023
Signed-off-by: Sahas Subramanian <[email protected]>
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have a question about a possible missing entry into the release notes. LGTM otherwise

Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have a question about a possible missing entry into the release notes. LGTM otherwise

shsms added 2 commits November 9, 2023 11:16
The RELEASE_NOTES.md file has diverged and needs to be synced from the
main branch, before it can be updated without conflicts.

Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
@github-actions github-actions bot added the part:docs Affects the documentation label Nov 9, 2023
@daniel-zullo-frequenz daniel-zullo-frequenz removed the cmd:skip-release-notes It is not necessary to update release notes for this PR label Nov 9, 2023
@shsms shsms added this pull request to the merge queue Nov 9, 2023
Merged via the queue into frequenz-floss:v1.x.x with commit 87b37b1 Nov 9, 2023
@shsms shsms deleted the generic-power-manager branch November 9, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
Development

Successfully merging this pull request may close these issues.

3 participants