-
Notifications
You must be signed in to change notification settings - Fork 20
Fix log when multiple batteries are attached to an inverter #1262
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
Conversation
This makes it possible to specialize `battery_distribution` to be more suitable for logging. Signed-off-by: Sahas Subramanian <[email protected]>
Without this, when 10 kW is distributed to 3 batteries, 2 of them
attached to a single inverter, the debug log displayed the same power
for the two batteries, totalling to higher than what was requested,
which is misleading (formatted for readability):
Distributing power 10 kW between the batteries {
ComponentId(1005): Power(value=6720.751321413104, exponent=0),
ComponentId(1004): Power(value=6720.751321413104, exponent=0),
ComponentId(1001): Power(value=3279.248678586895, exponent=0)
}
With this change, the log looks like this:
Distributing power 10 kW between the batteries: (CID1004, CID1005): 6.72 kW, CID1001: 3.28 kW
Also improved the formatting.
Signed-off-by: Sahas Subramanian <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a misleading debug log when multiple batteries are attached to a single inverter by properly grouping batteries and their power distribution. The change improves both the accuracy and readability of power distribution logging.
- Corrected the logging to show combined power for batteries on the same inverter instead of duplicating power values
- Enhanced log formatting to clearly show battery groupings and their respective power allocations
- Fixed the tracking of battery IDs to use the actual battery set rather than distribution keys
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py
Show resolved
Hide resolved
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py
Show resolved
Hide resolved
Signed-off-by: Sahas Subramanian <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the part:
This makes it possible to specialize
battery_distributionto be more suitable for logging.
In the commit message, but besides that, LGTM.
This is a description of the next commit, where we change the type of |
Without this, when 10 kW is distributed to 3 batteries, 2 of them
attached to a single inverter, the debug log displayed the same power
for the two batteries, totalling to higher than what was requested,
which is misleading (formatted for readability):
With this change, the log looks like this:
Also improved the formatting.