Code review main charge system #1
Conversation
…ncluded param files for easy changes later on
.vscode/test/test_maincharge.cpp
Outdated
There was a problem hiding this comment.
I realized it was in the wrong spot right before you posted this and forgot to push the changes. Will be pushed out with some unit tests soon
lib/constants/CCUParams.h
Outdated
| struct CCUParams{ | ||
| double target_voltage = 3.1; //per cell | ||
| int max_AC_current = 15; //need to add the DC current setpoint probably 30 amps | ||
| float ideal_cell_temp = 40; | ||
| }; |
There was a problem hiding this comment.
i would recommend using a namespace instead for this and make these const instead of a struct.
what this means essentially is you can create an instance of CCUParams and these are the default values for CCUParams instance structs. we dont really ever need multiple instances of CCUParams floating around so we dont need to make a struct for them.
lib/constants/pidParams.h
Outdated
| struct PIDParams { | ||
| uint32_t PIDrunInterval = 12; | ||
| float Kp = 1; | ||
| float Ki = 1; | ||
| float Kd = 1; | ||
| }; |
lib/interfaces/EthernetInterface.h
Outdated
| { | ||
| std::array<std::array<std::optional<volt>, 12>, num_chips> voltages; | ||
| std::array<celcius, 4 * num_chips> cell_temperatures; | ||
| std::array<float, num_humidity_sensors> humidity; | ||
| std::array<float, num_board_thermistors> board_temperatures; | ||
| float min_voltage; | ||
| float max_voltage; | ||
| size_t min_voltage_cell_id; // 0 - 125 | ||
| size_t max_voltage_cell_id; // 0 - 125 | ||
| size_t max_board_temperature_segment_id; // 0 - 5 | ||
| size_t max_humidity_segment_id; // 0 - 5 | ||
| size_t max_cell_temperature_cell_id; // 0 - 47 | ||
| float total_voltage; | ||
| float average_cell_temperature; | ||
| }; |
There was a problem hiding this comment.
pls dont push commented code. looks like this is just a copy-paste of what exists in the shared firmware types repo. you can just include a url link to the type instead
lib/interfaces/EthernetInterface.h
Outdated
| struct EthernetInterface | ||
| { | ||
| float maxCellTemp = 70; //in degress C | ||
| float maxCellV = 2.3; //in Volts | ||
| }; |
There was a problem hiding this comment.
what is this for? is this just the beginnings of the ethernet interface im assuming?
There was a problem hiding this comment.
This is just placeholder values for what the ethernet interface will return, I was using it as input into my unit tests but I think now I will be setting those values within the actual tests.
There was a problem hiding this comment.
typically we do not have a library dedicated to constants within firmware projects but its not the worst pattern. typically the systems are themselves configurable via their constructors and the args of the constructors are passed in on construction from a platformio project specific include (such as MCU_rev15_defs.h)
lib/systems/MainChargeSystem.h
Outdated
| bool current_flow = false; | ||
| float current_setpoint; | ||
|
|
||
| /* Function Init */ | ||
|
|
||
| int set_charge_current(); | ||
| /* Object Instance Creation */ | ||
| EthernetInterface ethernetInterface; | ||
| CCUParams ccuParams; | ||
| PID_RT pid; | ||
| PIDParams pidParams; |
There was a problem hiding this comment.
dont just put variables in a header generally, this is a good way to get duplicate symbol errors and it is bad practice (generally speaking)
lib/systems/MainChargeSystem.h
Outdated
| void charge_setup() { | ||
| pid.setInterval(pidParams.PIDrunInterval); | ||
| pid.setKp(pidParams.Kp); | ||
| pid.setKi(pidParams.Ki); | ||
| pid.setKd(pidParams.Kd); | ||
| } | ||
|
|
||
| void charge_cycle() { | ||
| double max_cell_V = 1; // ethernet_message.get_max_cell_V(); returns the max cell Voltage to allow for decision whether or not to continue charging or not (not yet created) the code for individual cell temps is in the AMS code that already exists | ||
|
|
||
| if (max_cell_V >= target_voltage) { //checks whether or not any of the cells are charged yet | ||
| void charge_cycle() { //add safe start that ramps up to max over 10 seconds | ||
| if (ethernetInterface.maxCellV >= ccuParams.target_voltage || ethernetInterface.maxCellTemp > ccuParams.ideal_cell_temp) { //checks whether or not any of the cells are charged yet | ||
| current_flow = false; //this should be a function that actually turns on/off current flow | ||
| } | ||
| else { | ||
| set_charge_current(); //sets the charge current to the PID adjusted value | ||
| current_flow = true; //same as line 32 | ||
| pid.compute(ethernetInterface.maxCellTemp-ccuParams.ideal_cell_temp); //also probably account to voltage | ||
| float pidTemp = pid.getOutput(); | ||
| pid.compute(ethernetInterface.maxCellV-ccuParams.target_voltage); | ||
| float pidVolts = pid.getOutput(); | ||
| float scalar = min(pidTemp, pidVolts); | ||
| current_setpoint = ccuParams.max_AC_current * scalar; //replace with just a Pd | ||
| current_flow = true; //same as line 32 | ||
| } |
There was a problem hiding this comment.
please put this functionality into a class and not just an anonymous function that affects random global variables. also please prefer functions that take args (if input is needed) and return the results of some computation to be more functional instead of stateful (stateful is what you are doing here)
…to remove reliance on Arduino lib
…n the teensy 4.1 env
I am not sure what else needs to be added but it is at a descent stopping point