Conversation
IbrahimFadel
left a comment
IbrahimFadel
left a comment
There was a problem hiding this comment.
Overall it looks good :) There are two big things I would change
- Try to be more granular with mutexes. Rn u have one mutex for everything, which will work but it kind of defeats the purpose of concurrency if only one thread can ever do work at a time. Mutexes can be created for every shared resource (individually). I think you were planning to do something more granular because you made two mutexes in
setup, but never used them? - Use the FreeRTOS API - so instead of declaring the magic
mutex_createetc. Use the actual FreeRTOS functions and datastructures for mutexes and tasks etc.
I also left comments in the code
|
|
||
| void motor_control_thread(void) { | ||
| for(;;) { // Infinite For Loop | ||
| delay(MOTOR_CONTROL_TIME); |
There was a problem hiding this comment.
delay will pause every thread, not just the one you call it in. Look for how to do delays in FreeRTOS
| while(!mutex_take(vehicle_mutex)) { | ||
| } // Return true or false, to know whether mutex lock was acquired or not | ||
|
|
||
| calculate_torque_cmd( |
There was a problem hiding this comment.
The function signature looks like this:
extern void calculate_torque_cmd(
float *torques, float current, float *wheelspeeds, float steering_angle
);
The biggest issue rn (aside from the order ur passing them in), is that torques and wheelspeeds should be arrays, but in your struct movement_vehicle_data they are single values.
The idea behind calculate_torque_cmd is that the output is an array of floats for torque. So I must initialize an array of floats somewhere, then give calculate_torque_cmd a pointer to that array so it can populate it with values. On the other hand, wheelspeeds is an input, and should be an array containing 4 wheelspeeds (one for each wheel).
|
|
||
| while(!mutex_take(vehicle_mutex)) {} | ||
|
|
||
| lcd_printf( |
There was a problem hiding this comment.
very small thing but lcd_printf should take a format string as the first parameter, something that looks like "Current: %f, FL Wheelspeed: %f, FR Wheelspeed: %f" etc.
| lcd_init(MOSI, MISO, SCK, LCD_CS); | ||
| bms_init(MOSI, MISO, SCK, LCS_CS); | ||
|
|
||
| data_mutex = mutex_t(); |
There was a problem hiding this comment.
I think you meant to call mutex_create
| } | ||
| } | ||
|
|
||
| void setup(void) { |
There was a problem hiding this comment.
You need to actually spawn threads and start the scheduler in order for it to run. Right now your code will run whats in setup, then since the scheduler never starts, it'll go into loop where it does nothing forever
| double t = 0; | ||
|
|
||
| for(int i = 0; i < num_cells; i++) { | ||
| v = bms_get_voltage(i); |
There was a problem hiding this comment.
The bms shares a SPI bus with the LCD, so this isn't safe
| static movement_vehicle_data vehicle_data; | ||
| static mutex_t vehicle_mutex; | ||
|
|
||
| void calculate_rpm(void) { |
There was a problem hiding this comment.
This is fine, but it might not work at high speeds, and there's a better way to do it. Think about how you could do this with interrupts
2 participants
No description provided.