Skip to content

Battery Charging Bug#1362

Open
dguittet wants to merge 5 commits intodevelopfrom
batt_pwr_fix
Open

Battery Charging Bug#1362
dguittet wants to merge 5 commits intodevelopfrom
batt_pwr_fix

Conversation

@dguittet
Copy link
Copy Markdown
Collaborator

The calculate_current_for_power_kw function reduces the battery's target power and current to what is available given SOC. Fix the bug in the charging portion where the reduced power and current should have been negative.

Fix #1361

@dguittet dguittet requested a review from brtietz November 14, 2025 13:03
@dguittet dguittet changed the title fix #1361 Battery Charging Bug Nov 14, 2025
@brtietz
Copy link
Copy Markdown
Collaborator

brtietz commented Nov 14, 2025

Thanks for looking into this! A few requests:

  1. Can you add a test that establishes this sign convention explicitly? There are a few tests in lib_resilience_test that are close, but I'm getting lost with multiple negative sign conventions. Bonus points for establishing the convention in the discharging function as well.
  2. Can you review other uses of the function to ensure the sign convention is correct? I'm seeing some additional inconsistencies in the below.
image

@dguittet
Copy link
Copy Markdown
Collaborator Author

Thanks Brian for the good catch. Turns out it was just a few edge cases here and there where calculate_max_discharge_kw and calculate_max_charge_kw could return values that were outside of their expected bounds so I added limit checks and removed the erroneous sign change in calculate_current_for_power_kw

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 20, 2026

Coverage Report for PR #1362

Coverage decreased (-0.006%) to 56.284%

Diff Coverage: No coverable lines changed

Coverage Regressions

16 previously-covered lines in 3 files lost coverage.

File Lines Lost Coverage
ssc/shared/lib_battery_powerflow.cpp 4 95.94%
ssc/shared/lib_geothermal.cpp 5 65.99%
ssc/shared/lib_battery.cpp 7 89.87%

Coverage Status
Change from base Build 23860928407: -0.006%
Covered Lines: 68172
Relevant Lines: 121122

💛 - Coveralls

@brtietz brtietz added this to the SAM 2026 Release milestone Apr 1, 2026
Copy link
Copy Markdown
Collaborator

@brtietz brtietz left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue in Battery

3 participants