What is wrong with my code ?

Hi everyone @Middleware & OS , please what is wrong with my code ?

void SensorActions(void *parameters)
{
 PushMessageToSerial("Setup", "Sensor's Monitor task is initialized..", LOG_INFO);
    while (1)
    {
        while (xQueueReceive(queue_sensors, (void *)&action, 10) == pdTRUE)
        {
            switch (action)
            {
            case actions_t::ACTION_SENSOR_VERSION:
            {
                uint8_t version[4] = {0};
                sensor.GetVersions(version);
                uint8_t length = sprintf(NULL, 0, "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                assert(length >= 0);
                char *buf = new char[length + 1];
                sprintf(buf, length + 1, "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                std::string text(buf);
                free(buf); PushMessageToSerial("Sensors", text, LOG_INFO);
            }
            break;
            default:
                break;
            }
        }
      vTaskSuspend(NULL); // so as to auto suspend task to not block the other lower priority tasks
    }
}

void SensorActions(void *parameters)
{
 PushMessageToSerial("Setup", "Sensor's Monitor task is initialized..", LOG_INFO);
    while (1)
    {
        while (xQueueReceive(queue_sensors, (void *)&action, 10) == pdTRUE)
        {
            switch (action)
            {
            case actions_t::ACTION_SENSOR_VERSION:
            {
                uint8_t version[4] = {0};
                sensor.GetVersions(version);
                uint8_t length = sprintf(NULL, 0, "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                assert(length >= 0);
                char *buf = new char[length + 1];
                sprintf(buf, length + 1, "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                std::string text(buf);
                free(buf); PushMessageToSerial("Sensors", text, LOG_INFO);
            }
            break;
            default:
                break;
            }
        }
      vTaskSuspend(NULL); // so as to auto suspend task to not block the other lower priority tasks
    }
}

I'm using Arduino/ESP32 with FreeRTOS and I'm aware std::string isn't available. That's why I used sprintf to convert an int value to a string. Now I noticed that when I call sprintf in the task function as follows ESP32 crashes and I can't find the reason.

bool PushMessageToSerial(const std::string &source, const std::string &message, log_t log)

bool PushMessageToSerial(const std::string &source, const std::string &message, log_t log)

6 Replies

Sterling•7mo ago

Hmm, I think the issue with your code lies in the misuse of sprintf and memory management. The second argument of sprintf should be a buffer wherein the result can be stored, not the length. This is causing undefined behavior. @Marvee Amasi

Sterling•7mo ago

Also, you are making use of new to allocate memory but free to deallocate it, which is incorrect. You should use delete[] to free memory allocated with new[].Plus, In embedded environments, I think assert may not be appropriate because it might halt the system.

Sterling•7mo ago

Perhaps you could try the code this way 😗:

void SensorActions(void *parameters)
{
    PushMessageToSerial("Setup", "Sensor's Monitor task is initialized..", LOG_INFO);
    while (1)
    {
        while (xQueueReceive(queue_sensors, (void *)&action, 10) == pdTRUE)
        {
            switch (action)
            {
            case actions_t::ACTION_SENSOR_VERSION:
            {
                uint8_t version[4] = {0};
                sensor.GetVersions(version);
                // Calculate the length of the version string
                char tempBuffer[20]; // Buffer large enough to hold the version string
                int length = snprintf(tempBuffer, sizeof(tempBuffer), "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                if (length < 0 || length >= sizeof(tempBuffer))
                {
                    // Handle error
                    break;
                }
                // Allocate memory for the final string
                char *buf = new char[length + 1];
                snprintf(buf, length + 1, "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                PushMessageToSerial("Sensors", std::string(buf), LOG_INFO);
                delete[] buf; // Correctly deallocate memory
            }
            break;
            default:
                break;
            }
        }
        vTaskSuspend(NULL); // Suspend task to not block the other lower priority tasks
    }
}

void SensorActions(void *parameters)
{
    PushMessageToSerial("Setup", "Sensor's Monitor task is initialized..", LOG_INFO);
    while (1)
    {
        while (xQueueReceive(queue_sensors, (void *)&action, 10) == pdTRUE)
        {
            switch (action)
            {
            case actions_t::ACTION_SENSOR_VERSION:
            {
                uint8_t version[4] = {0};
                sensor.GetVersions(version);
                // Calculate the length of the version string
                char tempBuffer[20]; // Buffer large enough to hold the version string
                int length = snprintf(tempBuffer, sizeof(tempBuffer), "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                if (length < 0 || length >= sizeof(tempBuffer))
                {
                    // Handle error
                    break;
                }
                // Allocate memory for the final string
                char *buf = new char[length + 1];
                snprintf(buf, length + 1, "%d.%d.%d.%d", version[0], version[1], version[2], version[3]);
                PushMessageToSerial("Sensors", std::string(buf), LOG_INFO);
                delete[] buf; // Correctly deallocate memory
            }
            break;
            default:
                break;
            }
        }
        vTaskSuspend(NULL); // Suspend task to not block the other lower priority tasks
    }
}

Marvee Amasi•7mo ago

Undefined behavior Yeah that's why it crashes

Enthernet Code•7mo ago

@Marvee Amasi your code is crashing mainly because of three mistakes you are making which are: using the new operator to allocate memory for buf while you are freeing it using free(). This is incorrect and can lead to memory leaks or crashes. Since you are using C++, it is recommended to use new and delete for memory management. secondly, In the sprintf call you are passing length + 1 as the buffer length argument. whereas the buffer length argument should be the size of the buffer, not the size of the string you are going to write into it. You can simply pass length as the buffer length. thirdly, In the sprintf call, you are using a format specifier of %d for each version number. However, since the version numbers are uint8_t you should use the format specifier %hhu instead. your code should look like this 👇

c++
void SensorActions(void *parameters)
{
    PushMessageToSerial("Setup", "Sensor's Monitor task is initialized..", LOG_INFO);
    while (1)
    {
        while (xQueueReceive(queue_sensors, (void *)&action, 10) == pdTRUE)
        {
            switch (action)
            {
            case actions_t::ACTION_SENSOR_VERSION:
            {
                uint8_t version[4] = {0};
                sensor.GetVersions(version);
                char buf[16];
                int length = sprintf(buf, "%hhu.%hhu.%hhu.%hhu", version[0], version[1], version[2], version[3]);
                assert(length >= 0);
                std::string text(buf, length);
                PushMessageToSerial("Sensors", text, LOG_INFO);
            }
            break;
            default:
                break;
            }
        }
        vTaskSuspend(NULL); // so as to auto suspend task to not block the other lower priority tasks
    }
}

c++
void SensorActions(void *parameters)
{
    PushMessageToSerial("Setup", "Sensor's Monitor task is initialized..", LOG_INFO);
    while (1)
    {
        while (xQueueReceive(queue_sensors, (void *)&action, 10) == pdTRUE)
        {
            switch (action)
            {
            case actions_t::ACTION_SENSOR_VERSION:
            {
                uint8_t version[4] = {0};
                sensor.GetVersions(version);
                char buf[16];
                int length = sprintf(buf, "%hhu.%hhu.%hhu.%hhu", version[0], version[1], version[2], version[3]);
                assert(length >= 0);
                std::string text(buf, length);
                PushMessageToSerial("Sensors", text, LOG_INFO);
            }
            break;
            default:
                break;
            }
        }
        vTaskSuspend(NULL); // so as to auto suspend task to not block the other lower priority tasks
    }
}

Marvee Amasi•7mo ago

Lol I have seen mistakes here

Gaming

Programming

What is wrong with my code ?