Bms/pdb #415
Bms/pdb #415
Conversation
| int cell_voltages[12]; | ||
| bool is_cell_balancing = false; | ||
| int coloumb_count = 0; | ||
| const int battery_capacity = 22000; // mAh | ||
| int battery_percent_couloumb; | ||
| int battery_percent_voltage; | ||
| int battery_voltage; |
There was a problem hiding this comment.
Our convention is to prefix private member variables with m_, eg. m_battery_voltage
| SPI spi(SDI, SDO, CLK); // mosi, miso, sclk | ||
| AnalogIn currentADC(CURR_ANLG_IN); | ||
| DigitalOut buzzer_en(BUZZER_EN); | ||
| DigitalOut chip_select(SAMPL); |
There was a problem hiding this comment.
These should be private member variables
| buzzer_en.write(0); | ||
| chip_select.write(1); | ||
| spi.format(24, 3); | ||
| spi.frequency(1000000); |
There was a problem hiding this comment.
Make this a constant (no magic numbers)
| int spi_message = 0b000000000000000000000000; | ||
| if (is_cell_balancing) { | ||
| spi_message = 0b111111111111111100000000; | ||
| } |
There was a problem hiding this comment.
nit: can make this a ternary expression:
int spi_message = (is_cell_balancing ? 0b111111111111111100000000 : 0b000000000000000000000000);
There was a problem hiding this comment.
Also using hex here instead of binary might make it more readable
| } | ||
|
|
||
| battery_voltage = 0; | ||
| for (int i = 0; i < 16; i++) { |
There was a problem hiding this comment.
Why 16? The size of your cell_voltages array is 12 so this will seg fault
| if (current < 0) { | ||
| current = 0; | ||
| } else if (current > 29) { | ||
| current = 29.1; | ||
| } |
There was a problem hiding this comment.
Suggest using std::clamp (https://en.cppreference.com/w/cpp/algorithm/clamp)
| } else if (current > 29) { | ||
| current = 29.1; | ||
| } | ||
| current = current * 1000; // converts Amps to MilliAmps |
| current = current * 1000; // converts Amps to MilliAmps | ||
| coloumb_count += current * 0.001; // converts to MilliAmp Seconds | ||
| int battery_capacity_seconds = battery_capacity * 3600; // converts to MilliAmp Seconds | ||
| battery_percent_couloumb = (1 - (coloumb_count / battery_capacity_seconds)) * 100; |
There was a problem hiding this comment.
Integer division will truncate, is this intended?
| @@ -20,15 +20,12 @@ class PDBMonitoring final : public Module { | |||
| void load_monitoring(); | |||
| void rail_monitoring(); | |||
| void temperature_monitoring(); | |||
| void LED_matrix(); | |||
There was a problem hiding this comment.
Looks like there were unintended changes to pdb_2021, can you revert these?
| @@ -16,6 +16,9 @@ pdb_2021: | |||
| science_2021: | |||
| - SCIENCE_REV2_2021 | |||
|
|
|||
| bms_2022: | |||
| - PDB_REV2_2021 | |||
There was a problem hiding this comment.
You should be making a new target for PDB_REV1_2022 (or whatever the rev number is) for the new PDB, keep PDB_REV2_2021 as it is
No description provided.