-
Notifications
You must be signed in to change notification settings - Fork 0
Added basic implementation for precharge. One function for initializi… #34
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
base: develop
Are you sure you want to change the base?
Conversation
…ng a given struct to given values, another one for running in an infinite loop.
caiodasilva2005
left a comment
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.
Awesome work!
| float lower_ratio; | ||
| nertimer_t debounce_timer; | ||
| uint32_t debounce_time; | ||
| } prechargeconfig_t; |
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.
Love the struct for this, could you maybe add some docs for what each field is for?
Core/Inc/precharge_routine.h
Outdated
| * @param precharge_config is the empty configuration to configure. | ||
| * @return a precharge configuration for running the precharge thread. | ||
| */ | ||
| prechargeconfig_t *precharge_init(cell_asic_2950 ic, SPI_HandleTypeDef *hspi, |
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.
Can we add an assert that the transition ration is <= 1 and >= 0 and that hspi is not null
Core/Inc/precharge_routine.h
Outdated
| * @brief Runs the pre-charge until the threshold-ratio is reached, after which the AIR switch is set to open. | ||
| * @param precharge_config is the configuration to run. | ||
| */ | ||
| void precharge_run(prechargeconfig_t *precharge_config); No newline at end of file |
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.
nit: rename to vPrecharge like the tasks defined in shep_tasks.c and define it there. You can also set the thread parameters like delays (let's probably do about 500 ms to start) and other settings there. Use other tasks as a reference
Core/Inc/precharge_routine.h
Outdated
| /** | ||
| * @brief Given the cell, SPI, AIR switch, initializes the pre-charge structure for the given BATT/TS voltage capacitor threshold ratio, | ||
| * and with the given debounce time to wait when the AIR opens before closing again.ADI1_delay_ms | ||
| * @param ic idk what this is T-T. |
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 is the struct for the 2950 chipon hv plate for docs
Core/Inc/precharge_routine.h
Outdated
| * @brief Given the cell, SPI, AIR switch, initializes the pre-charge structure for the given BATT/TS voltage capacitor threshold ratio, | ||
| * and with the given debounce time to wait when the AIR opens before closing again.ADI1_delay_ms | ||
| * @param ic idk what this is T-T. | ||
| * @param hspi idk what this is either T-T. |
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 is the struct for the spi handler that goes into the isospi channel wae are using to comm to the chip for docs
Core/Src/precharge_routine.c
Outdated
|
|
||
| void precharge_run(prechargeconfig_t *precharge_config) | ||
| { | ||
| if (precharge_config == 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.
Make this an assert here, the code should crash if something critical like this is missing
Core/Src/precharge_routine.c
Outdated
| &precharge_config->debounce_timer)) { | ||
| cancel_timer(&precharge_config->debounce_timer); | ||
| } else { | ||
| // SOME SORT OF DELAY? |
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.
See above comment for delay timings, but yes a delay here is necessary
Core/Src/precharge_routine.c
Outdated
| precharge_config->debounce_time); | ||
| } | ||
| } | ||
| // DELAY? |
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.
See above comment about delay time.
Core/Src/precharge_routine.c
Outdated
| precharge_config->hspi, | ||
| precharge_config->gpo); | ||
| air_switch_closed = false; | ||
| // I would assume there could be a voltage spike here as well. |
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 is good logic, comment can be removed
Core/Src/precharge_routine.c
Outdated
| } | ||
| } else if (!air_switch_closed) { | ||
| // Check if charged up until threshold. | ||
| // Should there be another check for depleted battery voltage? |
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.
ah do you mean like welded AIR check? Let's not have that here now (not really in scope). This comment can be removed
caiodasilva2005
left a comment
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.
Funny thing @jr1221 I am almost sure that our debouncing control code in Cerberus 1.0 did not work sometimes because we were using the same timer for the device on debounce and device off debounce lol
jr1221
left a comment
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.
nice stuff. idk what the chipdata diff means so didnt review that.
| precharge_config->close_debounce_timer = | ||
| (nertimer_t){ 0, 0, false, false }; | ||
| precharge_config->debounce_time = debounce_time; | ||
| precharge_config->air_switch_closed = false; |
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.
where is the return here?
Core/Src/precharge_routine.c
Outdated
| hv_plate_t *hv_plate = precharge_config->hv_plate; | ||
| bool should_precharge = // TODO: mutex hv plate data | ||
| hv_plate->ts_volts * precharge_config->transition_ratio >= | ||
| hv_plate->batt_volts; |
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.
shouldnt it be batt_volts * transition ratio? Because its when ts gets above 90% of battery?
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.
Ah whoops yeah thanks for catching that
Core/Src/shep_tasks.c
Outdated
|
|
||
| prechargeconfig_t precharge_config; | ||
| precharge_init(&precharge_config, hv_plate, 0.9f, | ||
| 50 /* ms debounce time */); |
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.
lets start more like with 200ms. time is not of the essence but safety sure is imo.
| precharge_config->debounce_time, close_relay, | ||
| precharge_config); | ||
|
|
||
| debounce(!should_precharge, &precharge_config->close_debounce_timer, |
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.
im going to do some review and start a thread with markow and pablo tmrw night, however I think we need to de-assert the GPIO whenever shutdown is open, because when shutdown closes we dont want to have our GPIO already high in effect only precharging the first time.
…ng a given struct to given values, another one for running in an infinite loop.
Changes
Function that takes an empty struct and initializes it with given values + function that runs an infinite loop for voltages.
To Do
Any remaining things that need to get done
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please reach out to your Project Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
Closes #5