-
Notifications
You must be signed in to change notification settings - Fork 19
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
BMS Coulomb Counting SOC #988
Conversation
const bool acc_fault = | ||
overtemp_fault || undertemp_fault || overvoltage_fault || undervoltage_fault || communication_fault; | ||
|
||
return acc_fault; |
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.
Personally find this easier to debug without needing to set custom breakpoints
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.
Are you going to broadcast some kind of warning or something if the SOC if corrupt and to not read it during like a 100HZ or a certain cycle?
*/ | ||
EEPROM_StatusTypeDef App_Eeprom_Read4CopiesOfAddress(struct Eeprom *eeprom, uint16_t page, uint16_t *addresses); | ||
ExitCode App_Eeprom_ReadMinSoc(struct Eeprom *eeprom, uint16_t address, float *min_soc); |
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.
This module could probably use a better name. By name it sounds like an application-level wrapper around the EEPROM, but it seems highly specific to SOC. Consider renaming to something more accurate.
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 guess one thing I could do is rather than include the EEPROM object in the world, have it be a part of the SocStats struct as it's the only thing using the EEPROM; then I could place these SOC-related EEPROM functions in App_Soc.c
@@ -1,10 +1,13 @@ | |||
#include "App_Eeprom.h" | |||
#include "string.h" | |||
|
|||
#define NUM_PAGES (128U) | |||
#define NUM_PAGES (16U) |
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.
this seems extremely sus but OK
maybe leave a comment about this being corrupted
#include "App_SharedProcessing.h" | ||
#include "lut/App_Lut_CellVoltageToSoc.h" | ||
|
||
#define STATE_OF_HEALTH (1.0f) |
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.
when you update it, please leave a comment on where the number comes from
/** | ||
* return Coulombs of the SE with the lowest SOC | ||
* @param soc_stats The charge stats of the pack | ||
* @return cloulombs of lowest SOC SE | ||
*/ | ||
float App_SocStats_GetMinSocCoulombs(struct SocStats *soc_stats); | ||
|
||
/** | ||
* return percent SOC of the SE with the lowest SOC | ||
* @param soc_stats The charge stats of the pack | ||
* @return soc % of lowest SOC SE | ||
*/ | ||
float App_SocStats_GetMinSocPercent(struct SocStats *soc_stats); | ||
|
||
/** | ||
* Get the minimum series element open circuit voltage approximation given current pack SOC status | ||
* @param soc_stats current SOC of each series element in pack | ||
* @return float minimum series element open circuit voltage approximation | ||
*/ | ||
float App_SOC_GetMinOcvFromSoc(struct SocStats *soc_stats); |
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.
Instead of having a bunch of getters that are only sent over CAN, I like the pattern of just having a single "Broadcast" function that handles setting all the TX values. I did this on the accumulator in the BMS refactor and it cuts the file size in half, and is a lot clearer + more maintainable in my opinion
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.
Only App_SocStats_GetMinSocPercent
is being broadcast over CAN. App_SOC_GetMinOcvFromSoc
and App_SocStats_GetMinSocCoulombs
are implemented for future use, but not currently utilized.
I can remove App_SocStats_GetMinSocCoulombs
if it bothers you, but App_SOC_GetMinOcvFromSoc
will be needed for current limiting in the near future.
Yes there is a CAN message to broadcast if SOC value read is corrupt. In this case, it will reset based on min-cell voltage, but that is inaccurate unless cell voltages have settled. So we need to know it's a dirty starting-value. |
"unit": "%" | ||
}, | ||
"SocCorrupt": { | ||
"bits": 1 |
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.
Added to Bms_Vitals as we have hit the limit on the # of messages the BMS can send before hitting tx overflow, will need to address this
Testing on Nov 12 2023 with driving testing, started the day at around 77% and ended the day at 30.2%. Referencing the OCV LUT and min cell voltage revealed that the "actual" SOC was 24.9%, so we are about 5% optimistic at the moment. Further tuning will be needed, but otherwise tested working. |
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.
dub
### Summary <!-- Quick summary of changes, optional --> Track state-of-charge using coulomb counting on BMS. No current limiting implemented, solely statistical tracking. SOC is saved to EEPROM on 1s intervals, utilizing a basic wear-leveling algorithm - Address of up-to-date soc stored in page 0 of EEPROM, this is only written once per power cycle, no need to wear-level - SOC value stored in page 1-127, written once per second. Active SOC page written to will increment once per power cycle. - SOC value read is checked for corruption: - When SOC value saved to EEPROM, 4 copies saved to page - When SOC value read, 4 values are checked to see if they match and are within range - If not, read SOC from Ocv to SOC lookup table (inaccurate but better than corrupted value) SOC can be reset in one of two ways, both controlled by debug CAN messages - Update from Ocv-Soc LUT - Update with manually specified SOC value ### Changelist <!-- Give a list of the changes covered in this PR. This will help both you and the reviewer keep this PR within scope. --> - Added SOC tracking logic - Added EEPROM reading/writing - Added simple test for SOC ### Testing Done <!-- Outline the testing that was done to demonstrate the changes are solid. This could be unit tests, integration tests, testing on the car, etc. Include relevant code snippets, screenshots, etc as needed. --> - Test checking for accuracy added to TestStateMachine - LUT lookup tested in TestStateMachine (will be moved during refactor) - Tested on car: - SOC can be reset using both custom value and via OCV lookup from CAN debug message - Test value saved and recovered between power cycles from EEPROM - Checked that SOC tracks linearly with current draw (tested with full charge cycle) ### Resolved Issues <!-- Link any issues that this PR resolved like so: `Resolves #1, #2, and #5` (Note: Using this format, Github will automatically close the issue(s) when this PR is merged in). --> ### Checklist *Please change `[ ]` to `[x]` when you are ready.* - [x] I have read and followed the code conventions detailed in [README.md](../README.md) (*This will save time for both you and the reviewer!*). - [x] If this pull request is longer then **500** lines, I have provided *explicit* justification in the summary above explaining why I *cannot* break this up into multiple pull requests (*Small PR's are faster and less painful for everyone involved!*). --------- Co-authored-by: will-chaba <[email protected]>
### Summary <!-- Quick summary of changes, optional --> Track state-of-charge using coulomb counting on BMS. No current limiting implemented, solely statistical tracking. SOC is saved to EEPROM on 1s intervals, utilizing a basic wear-leveling algorithm - Address of up-to-date soc stored in page 0 of EEPROM, this is only written once per power cycle, no need to wear-level - SOC value stored in page 1-127, written once per second. Active SOC page written to will increment once per power cycle. - SOC value read is checked for corruption: - When SOC value saved to EEPROM, 4 copies saved to page - When SOC value read, 4 values are checked to see if they match and are within range - If not, read SOC from Ocv to SOC lookup table (inaccurate but better than corrupted value) SOC can be reset in one of two ways, both controlled by debug CAN messages - Update from Ocv-Soc LUT - Update with manually specified SOC value ### Changelist <!-- Give a list of the changes covered in this PR. This will help both you and the reviewer keep this PR within scope. --> - Added SOC tracking logic - Added EEPROM reading/writing - Added simple test for SOC ### Testing Done <!-- Outline the testing that was done to demonstrate the changes are solid. This could be unit tests, integration tests, testing on the car, etc. Include relevant code snippets, screenshots, etc as needed. --> - Test checking for accuracy added to TestStateMachine - LUT lookup tested in TestStateMachine (will be moved during refactor) - Tested on car: - SOC can be reset using both custom value and via OCV lookup from CAN debug message - Test value saved and recovered between power cycles from EEPROM - Checked that SOC tracks linearly with current draw (tested with full charge cycle) ### Resolved Issues <!-- Link any issues that this PR resolved like so: `Resolves #1, #2, and #5` (Note: Using this format, Github will automatically close the issue(s) when this PR is merged in). --> ### Checklist *Please change `[ ]` to `[x]` when you are ready.* - [x] I have read and followed the code conventions detailed in [README.md](../README.md) (*This will save time for both you and the reviewer!*). - [x] If this pull request is longer then **500** lines, I have provided *explicit* justification in the summary above explaining why I *cannot* break this up into multiple pull requests (*Small PR's are faster and less painful for everyone involved!*). --------- Co-authored-by: will-chaba <[email protected]>
Summary
Track state-of-charge using coulomb counting on BMS. No current limiting implemented, solely statistical tracking.
SOC is saved to EEPROM on 1s intervals, utilizing a basic wear-leveling algorithm
SOC can be reset in one of two ways, both controlled by debug CAN messages
Changelist
Testing Done
Resolved Issues
Checklist
Please change
[ ]
to[x]
when you are ready.