Conversation
…dded a state machine to transition between states, updated calculation for wheelspin RPM, and initalized missing things from setup(). Also added a safe gaurd for torque_update() and fixed some errors.
…n on LCD. Wrote more relevant info to lcd. Fixed a few bugs
AveryLor
left a comment
AveryLor
left a comment
There was a problem hiding this comment.
Overall really well done, didn't find any actual issues, you handled thread safety very well with the different mutexes you used.
Most of my comments are about small stuff and fun facts lol :), but again very well done
|
|
||
| void state_machine_task(void* parameters) { | ||
| // set all relay pins as outputs | ||
| pinMode(AIR_MINUS_PIN, arduino::OUTPUT); |
There was a problem hiding this comment.
I would init your pins in some sort of setup loop not necesarilly something that is wrong, but the actual pin mode only needs to be set once, not every time the state_machine_task is run.
You weren't given a setup loop so this was probably the best place to do it lol
| // precharge sequence | ||
| digitalWrite(AIR_MINUS_PIN, arduino::HIGH); // close AIR- | ||
| digitalWrite(PRECHARGE_PIN, arduino::HIGH); // close precharge relay | ||
| vTaskDelay(pdMS_TO_TICKS(5000)); // wait 5 seconds |
There was a problem hiding this comment.
You have good logic for checking the carstate after the 5 second delay before actually changing the pins. I noticed that you had a bool in the project for bms fault. Ideally that should be checked as well considering that is another possible action that could have occurred during the 5 second delay.
| wheel_speed_last_pulse = current; | ||
| } | ||
|
|
||
| float sensor_wheel_speed_get(void) { |
There was a problem hiding this comment.
I believe this calculation also works:
return wheel_speed_interval * 17;
I think floating point math is harder on hardware, but I doubt its an issue.
| /* | ||
| Initialize the LCD peripheral | ||
| */ | ||
| extern void lcd_init(uint8_t mosi, uint8_t miso, uint8_t sck, uint8_t lcs_cs); |
There was a problem hiding this comment.
Not an issue at all, but just pointing out in case you ever end up in this situation, but since we don't expect any input from the LCD (it can take touch feedback I believe) you can technically get away with not using MISO at all. It will save you a GPIO, just wanted to point that out despite it being included in the project.
2 participants
No description provided.