-
Notifications
You must be signed in to change notification settings - Fork 4
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 Refactor + State of Charge #292
Conversation
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.
lgtm
@@ -43,10 +43,8 @@ static const uint16_t s_resistance_lookup[TABLE_SIZE] = { | |||
|
|||
int calculate_temperature(uint16_t thermistor) { | |||
// INCOMPLETE |
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.
what's still incomplete about this?
@@ -73,7 +71,7 @@ static const LtcAfeSettings s_afe_settings = { | |||
.cell_bitset = { 0xFFF, 0xFFF, 0xFFF }, | |||
.aux_bitset = { 0x14, 0x15, 0x15 }, | |||
|
|||
.num_devices = 3, | |||
.num_devices = 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.
was this change for testing?
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.
Yes ill put it back to 3
} | ||
|
||
StatusCode cell_sense_run() { | ||
static inline StatusCode cell_sense_run() { |
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 know if it makes sense to inline this. It's a large function that itself calls other functions. I think it's best to leave the compiler to optimize
ltc_afe_init(ltc_afe_storage, &s_afe_settings); | ||
delay_ms(10); | ||
TickType_t xLastWakeTime = xTaskGetTickCount(); | ||
while (true) { | ||
prv_cell_sense_conversions(); |
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.
Should cell_sense_run()
also be prv? Also again maybe don't inline this. I remember the justification being Jarvis saying it'll save us some memory but did this fix anything? It's probably best to leave it to the compiler.
if (xTaskGetTickCount() > s_last_params_update + pdMS_TO_TICKS(UPDATE_FLASH_PARAMS_PERIOD_MS)) { | ||
LOG_DEBUG("Updating params\n"); | ||
s_last_params_update = xTaskGetTickCount(); | ||
max17261_get_learned_params(&s_fuel_guage_storage, &s_fuel_params); |
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.
is this not necessary anymore?
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.
Not needed anymore this was for fuel gauge SOC. Also the loading from flash memory was causing issues at comp I think jarvis/mitch got rid of it
|
||
s_fuel_gauge_settings->sense_resistor_mohms = SENSE_RESISTOR_MOHM; | ||
|
||
status = max17261_init(s_fuel_guage_storage, s_fuel_gauge_settings, &s_fuel_params); | ||
tasks_init_task(current_sense, TASK_PRIORITY(2), NULL); |
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.
should current sense be higher priority than others since it handles the notification from the interrupt pin? I think we talked about this before but can't remember
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.
ngl I don't know how this works plz link whatever you based it off of 🙏
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.
https://uwmidsun.atlassian.net/wiki/spaces/ELEC/pages/3667918850/Custom+State+of+Charge+Algorithm
This should explain the math decisions. Most of this is based off of open source repos + personal decisions for better accuracy. Coulomb counting formula from online and Voltage mapped to a SOC are the two key ideas. There isn't much more to do other than imrpvoe the accuracy based on battery characteristics + discrete time controls
No description provided.