I will start with a brief introduction to what I'm doing.
I am writing a library for a microcontroller device that can be used to control a certain application on PC over UART. The first draft of the application was purely sequential - a set of functions that can be called from the library in order to send certain data to PC. This worked but had potential problems since the execution was sequential and if something went wrong, the whole program would collapse. I decided to incorporate freeRTOS into the library in order to have better control over the execution and there is where the problems ensued. I will try to the best of my abilities to bring the problem closer to you by presenting only the essential code parts.
Let me briefly give you a summed up review of the first version of the library. I will provide only the code that I think is essential.
/* main.c
Contains a task that uses library functions to send commands.
*/
/* prvControlTask
Uses library functions to control PC application over UART.
*/
static void prvControlTask( void *pvParameters )
{
TickType_t xNextWakeTime;
/* Remove compiler warning about unused parameter. */
( void ) pvParameters;
xNextWakeTime = xTaskGetTickCount();
/* Using some library functions */
connect(0x00, "Mike", "Mike-123"); // Connects to the PC app
play(0x00, "move", "right"); // Control some app aspect
play(0x00, "move", "right-up");
play(0x00, "move", "right-down");
play(0x00, "jump", "left-down");
for( ;; )
{
/* Place this task in the blocked state until it is time to run again. */
vTaskDelayUntil( &xNextWakeTime, mainQUEUE_SEND_FREQUENCY_MS );
}
}
Over in the library, there are 3 data structures that the library uses:
/* library
Contains structures and functions for UART-PC control
*/
char uart_bytes[100]; // Used to send bytes to uart
struct CONNECT{ // To help store data for connect function
uint8_t player_type;
char* username;
char* password;
};
struct PLAY{ // To help store data for play function
uint8_t player_type;
char* command;
char* direction;
};
The functions themselves are pretty straight-forward. The called function fills the structure and sends it to another function which turns it's data into bytes and fills uart_bytes and sends those bytes over UART.
/* library
Contains structures and functions for UART-PC control
*/
void connect(uint8_t player_type, char* username, char* password){
struct CONNECT connect_data = {0};
connect_data.player_type = player_type;
connect_data.username = username;
connect_data.password = password;
send_connect_struct(&connect_data);
}
void send_connect_struct(struct CONNECT * connect_data){
/*
Turns data player_type, username, password into byte array and fills uart_bytes and sends to PC over UART.
*/
}
/*
Exactly the same for the play functions.
*/
This all works as it should. I am observing the data being sent and it all agrees with what the function was called with. This shows that there are no problems with the functions themselves if the data is properly given.
The problems could arise when there is delay or an error in connect or some play command and if it fails, all others will fail as well since it disconnects on wrong command. Here I thought about using freeRTOS.
The idea is to have a task inside the library that will take data from a queue and call the corresponding command based on that data. When the user calls a function from the main.c, the function will simply create a structure of that command, store it in the queue and finish. Then the queue will have pending commands and the task can simply take one by one from those commands and execute the appropriate send function.
Here is the structure that will be stored in a queue.
struct COMMAND{ // Used to hold commands into a queue for the task to execute
uint8_t command_type;
struct CONNECT * connect_data;
struct PLAY * play_data;
}
Here is the queue that holds the commands.
QueueHandle_t xCommandQueue; // global variable
void start_tasks(void){
xCommandQueue = xQueueCreate(32, sizeof(struct COMMAND*));
/* Creating a task */
xTaskCreate( prvLIBTask, "LibTask", configMINIMAL_STACK_SIZE, NULL, TASK_PRIORITY, &xTaskHandle );
}
And here is the task that will run.
void prvLIBTask( void *pvParameters ) // Task that receives from the queue and sends command
{
struct COMMAND* rec_command;
/* Remove compiler warning about unused parameter. */
( void ) pvParameters;
for( ;; )
{
/* Wait until something is received from the queue */
while(!xQueueReceive( xCommandQueue, &rec_command, portMAX_DELAY ));
if (rec_command->command_type == 0x00)
{
send_connect_struct(&(*rec_command->connect_data));
free(rec_command);
}
else if (rec_command->command_type == 0x01)
{
send_play_struct(&(*rec_command->play_data));
free(rec_command);
}
}
}
The functions that are called from the main (connect and play) look like this.
void connect(uint8_t player_type, char* username, char* password){
struct CONNECT *connect_data = malloc(sizeof(struct CONNECT));
connect_data->player_type = player_type;
connect_data->username = malloc(strlen(username)+1);
memcpy(connect->data.username, username, strlen(connect_data.username)+1);
connect_data->password = malloc(strlen(password)+1);
memcpy(connect_data->password, password, strlen(connect_data.password)+1);
struct COMMAND* latest_command = malloc(sizeof(struct COMMAND));
latest_command->command_type = 0x00;
latest_command->connect_data = &(*connect_data);
xQueueSend( xCommandQueue, &(latest_command), portMAX_DELAY );
}
/*
Almost exactly the same process for play.
Note that in the send_connect_struct and send_play_struct, at the end of the function I am freeing all the char* fields first and then the passed structure.
Example:
free(connect_data->username);
free(connect_data->password);
free(connect_data);
Since the queue will copy the contents of the pointer, the receive end of the queue will see the actual memory location of the allocated data and therefore can free it at the end of the execution.
*/
The problem is that the CONNECT command goes fine and I see the right data getting sent but the PLAY commands have problems where sometimes the first PLAY will go but the second one will have messed up data. Sometimes it even gets stuck after CONNECT function as I see that the function doesn't even exit the play(). It definitely is something with the dynamic memory allocation but I have no idea what I'm doing wrong.
The weirdest thing is that when I do not allocate the structure fields but just say for example (part of the connect()):
struct CONNECT connect_data = malloc(sizeof(struct CONNECT));
connect_data->player_type = player_type;
connect_data->username = username;
it does send correct data for connect and play but this cannot be since the pointers of the structures will point to stack memory (that holds username and pass from parameters passed to the function), right?
Do you see a mistake here?
Edit:
Here are the functions I'm calling:
connect(0x00, "Mike", "Mike123");
play(0x00, "move", "right");
play(0x00, "move", "right-up");
play(0x00, "move", "right-down");
Here is the queue:
xCommandQueue = xQueueCreate(32, sizeof(struct COMMAND));
Here is the receiving task:
void prvLIBTask( void *pvParameters ) // Task that receives from the queue and sends command
{
struct COMMAND rec_command;
/* Remove compiler warning about unused parameter. */
( void ) pvParameters;
for( ;; )
{
/* Wait until something is received from the queue */
while(!xQueueReceive( xCommandQueue, &rec_command, portMAX_DELAY ));
if (rec_command.command_type == 0x00)
{
/* functions to print structure field */
send_connect_struct(rec_command.connect_data);
}
else if (rec_command.command_type == 0x01)
{
/* functions to print structure field */
send_play_struct(rec_command.play_data);
}
}
}
And here are the functions:
void connect(uint8_t player_type, char* username, char* password){
struct CONNECT *connect_data = malloc(sizeof(struct CONNECT));
connect_data->player_type = player_type;
connect_data->username = malloc(strlen(username)+1);
memcpy(connect_data->username, username, strlen(username)+1);
connect_data->password = malloc(strlen(password)+1);
memcpy(connect_data->password, password, strlen(password)+1);
struct COMMAND* latest_command = malloc(sizeof(struct COMMAND));
latest_command->command_type = 0x00;
latest_command->connect_data = connect_data;
xQueueSend( xCommandQueue, latest_command, portMAX_DELAY );
}
void play(uint8_t player_type, char* command, char* direction){
struct PLAY *play_data = malloc(sizeof(struct PLAY));
play_data->player_type = player_type;
play_data->command = malloc(strlen(command)+1);
memcpy(play_data.command, command, strlen(command)+1);
play_data->direction = malloc(strlen(direction)+1);
memcpy(play_data->direction, direction, strlen(direction)+1);
struct COMMAND* latest_command = malloc(sizeof(struct COMMAND));
latest_command->command_type = 0x01;
latest_command->play_data = play_data;
xQueueSend( xCommandQueue, latest_command, portMAX_DELAY );
}
The Results:
The result of the print functions right after I receive from the Queue:
CONNECT
type = 0x00
username = Mike
password = Mike123
and then the code blocks somewhere in the publish function. I don't receive any publish messages.
If I however don't allocate for the char* fields in the publish and instead use:
play_data->player_type = player_type;
play_data->player_type = command;
play_data->player_type = direction;
I get the following output:
CONNECT
type = 0x00
username = Mike
password = Mike123
PUBLISH
type = 0x00
username = move
password = right
PUBLISH
type = 0x00
username = move
password = right-up
PUBLISH
type = 0x00
username = move
password = 0
So the first two publishes are OK but the second one says 0. I don't get this since they are created by the same function.
The send_connect_struc and send_play_struc work fine when given correct data like in the first three cases. Both functions are of this template:
void send_play_struc(struc PLAY* play_data) {
/* fill uart_bytes */
free(play_data->command);
free(play_data->direction);
free(play_data);
}
The function connect() has some problems:
void connect(uint8_t player_type, char* username, char* password){
// you must check for NULL
struct CONNECT *connect_data = malloc(sizeof(struct CONNECT));
connect_data->player_type = player_type;
// you must check for NULL
connect_data->username = malloc(strlen(username)+1);
// this line is wrong
//memcpy(connect->data.username, username, strlen(connect_data.username)+1);
// corrected version
memcpy(connect_data->username, username, strlen(username) + 1);
// you must check for NULL
connect_data->password = malloc(strlen(password)+1);
// this line is wrong
//memcpy(connect_data->password, password, strlen(connect_data.password)+1);
// corrected version
memcpy(connect_data->password, password, strlen(password) + 1);
// you must check NULL
struct COMMAND* latest_command = malloc(sizeof(struct COMMAND));
latest_command->command_type = 0x00;
// NOTE: `&(*p)` is equivalent to `p`
latest_command->connect_data = &(*connect_data);
// here you must pass `latest_command` as argument, not `&(latest_command)`
xQueueSend( xCommandQueue, &(latest_command), portMAX_DELAY );
}
EDIT: the code in which you create the queue is also not correct: since you are pushing items of type struct COMMAND, the code to create the queue should be:
xCommandQueue = xQueueCreate(32, sizeof(struct COMMAND)); // this will hold 32 items of type `struct COMMAND`
Also, if this code appears to work:
// NOTE: DON'T DO THIS
connect_data.username = username;
connect_data.password = password;
... it's because probably you pass string literals as parameters (so username and password are string literals), and these are usually allocated in the read-only memory area and they usually have the same lifetime of the program itself (i.e.: string literals do not become invalid after you return from the function, because they're not allocated on the stack, usually). Nevertheless, it's a dangerous and restricting assumption to make.
EDIT 2: It appears to be a problem related to malloc(). You might consider using pvPortMalloc() instead of malloc() (and pvPortFree() instead of free()). As a notable remark, pvPortMalloc() is thread-safe, while malloc() is not.