Why data corruption with local variable in a local function?

Hi

I am working on a data logging project for 3 sensors.
Basically there are three tasks:
(1) DAQTask to read data
(2) ControllerTask as an intermediate thread to dispatch data to SD card and optionally apply filter for other display node like Wifi terminal
(3) SDCardTask

The workflow is :
DAQTask ->|daqMsgQueue|->ControllerTask->|sdMsgQueue|->SdCardTask

Problem: intermittent data corruption on SD card.
After several days of debugging I have narrowed the problem to a local function that is to serialize raw sensor data to a character stream with code snippet below:

static void SDDataSerializer(daq_data_t *inBuf, const char *serBuf)
{
   String sensorData = ""; ///delcare it as static String sensorData to solve data corruption issue
   sensorData = String(inBuf->sensorA) 
                + "," + String(inBuf->sensorB)
                + "," + String(inBuf->sensorC);
   serBuf = (const char *)sensorData.c_str();
}

To solve the problem of intermittent data corruption I needed to declare String sensorData as static String sensorData. This method contradicts my knowledge of RTOS that, as long as it is a local function with a single task calling it, all local variables are saved in the stack of the task and there is no need to declare that variable as static in the global space. But somehow once I have changed String sensorData to static String sensorData in SDDataSerializer() above, no more data corruption is observed. Why?

Below please find the full listing in pseudocode format. The fcn SDDataSerializer() is declared in SDCardTask.cpp as a local function.

///main data structure
typedef struct{
  sensorA_data_t   sensorA;
  sensorB_data_t   sensorB;
  sensorC_data_t   sensorC;
}daq_data_t;
//Filename: DAQTask.cpp
//This task ranks the highest priority (6) to read data from 3 sensors at 50Hz and dispatch to daqMsgQueue
void DAQTask(void *pvParameters)
{
  (void) pvParameters;
  TickType_t xLastWakeTime;
  daq_data_t result;
  DAQInit();
  xLastWakeTime = xTaskGetTickCount();
  for(;;){
   vTaskDelayUntil(&xLastWakeTime, 20); //50Hz DAQ rate
   SensorADataGet(&result.sensorA);
   SensorBDataGet(&result.sensorB);
   SensorCDataGet(&result.sensorC);
   xQueueSend(daqMsgQueue, (void *)&result, (TickType_t)2);
 }
}
//Filename: ControllerTask.cpp

//Data handler function to dispatch messages to sdMsgQueue and optional wifiMsgQueue
static void DaqDataHandler(daq_data_t *inBuf)
{
   if(SDCardAvailable()){
     xQueueSend(sdMsgQueue, (void *)inBuf, 2);
   }
   ///...optionally apply filter to raw data and send the results to a Wifi node 
   if(WifiAvailable()){
    daq_data_t filter_result = Filter(inBuf);
    xQueueSend(wifiMsgQueue, (void *)&filter_result, 2);
   }  
}

//This task ranks the lower next to DAQTask as an intermediate task 
void ControllerTask(void *pvParameters)
{
   daq_data_t inResult;
   for(;;){
     if(xQueueReceive(daqMsgQueue, (void*)&inResult, 0)==pdTRUE)
     {
       DaqDataHandler((daq_data_t *)&inResult);
     }
     vTaskDelay(5);
   }
}
//Filename: SDCardTask.cpp
//Data serializer function for SD Card
static void SDDataSerializer(daq_data_t *inBuf, const char *serBuf)
{
   String sensorData = "";
   sensorData = String(inBuf->sensorA) 
                + "," + String(inBuf->sensorB)
                + "," + String(inBuf->sensorC);
   serBuf = (const char *)sensorData.c_str();
}

//SD card task to receive data from sdMsgQueue and write to SD card
void SDCardTask(void *pvParameters)
{
   daq_data_t inResult;
   for(;;){
     if(xQueueReceive(sdMsgQueue, (void*)&inResult, 0)==pdTRUE)
     {
       if(SdCardAvailable()){
         const char *p_char;
         SDDataSerializer(&inResult, p_char);
         SDCard.file.println(p_char); ///write a serialized character stream to SD card
       }
     }
     vTaskDelay(10);
   }
}

How does this even work? You create a string object on stack and then write the naked c_str pointer to serBuf which is an input param to the function. The object goes out of scope and gets destroyed as soon as the function returns. serBuf modification will not be visible to the caller!

1 Like

Or just return the String built to the caller. Otherwise you’d have to provide the address of a char* as output argument.

Have changed

SDDataSerializer(daq_data_t *inBuf, const char *serBuf) to

SDDataSerializer(daq_data_t *inBuf, stream_data_t *serBuf) with stream_data_t as

typedef struct {
   const char *p_char;
} stream_data_t;

Now SDDataSerializer() became

static void SDDataSerializer(daq_data_t *inBuf, stream_data_t *serBuf)
{
    const String delimiter = String(",");
    const String newline = String("\r\n");

    String sensorData = ""; 

    sensorData = String(inBuf->sensorA) 
                + delimiter + String(inBuf->sensorB)
                + delimiter + String(inBuf->sensorC)
                + newline;

    serBuf->p_char = (const char *)sensorData.c_str();
}

Caller of SDDataSerializer became:


void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  delay(500);

  daq_data_t sensor_data = {.sensorA = 3.14, .sensorB=-3.14, .sensorC=0.998};

  stream_data_t daq_stream;

  SDDataSerializer(&sensor_data, &daq_stream);

  if(daq_stream.p_char!=0){
    Serial.print(daq_stream.p_char);
  }
}

The result of Serial Printing is its character stream as expected:

3.14,-3.14,1.00

Address of stream_data_t is provided in stream_data_t daq_stream.

In non-RTOS environment, declaring daq_data_t sensor_data as static or non-static both work. However, in FreeRTOS, it is only static stream_data_t daq_stream can provide its integrity.

The string sensorData is created on stack and will get destroyed as soon as the function returns. As a result, the caller will end up getting a pointer to a already freed memory.

OK, as Hartmut Schaefer suggested, this should be better.

static stream_data_t SDDataSerializer(daq_data_t *inBuf){
    stream_data_t byte_stream;
    const String delimiter = String(",");
    const String newline = String("\r\n");

    String sensorData = "";

    sensorData = String(inBuf->sensorA) 
                + delimiter + String(inBuf->sensorB)
                + delimiter + String(inBuf->sensorC)
                + newline;

    byte_stream.p_char = (const char *)sensorData.c_str(); 

    return byte_stream;
}

Caller of SDDataSerializer became:

  daq_data_t sensor_data = {.sensorA = 3.14, .sensorB=-3.14, .sensorC=0.998};

  stream_data_t daq_stream = SDDataSerializer(&sensor_data);

  if(daq_stream.p_char!=0){
    Serial.print(daq_stream.p_char);
  }

In this way, there is no static declaration for sensorData and this method will work for non-RTOS and RTOS environment. Right?

You still return a dangling pointer because the local String object get’s destroyed when leaving the function/scope. It’s not related to FreeRTOS. It’s just a programming bug.
Return the String object by value. And with RVO (return value optimization) it’s also cheap and makes the code more clear.

This is what @hs2 mean -

static String SDDataSerializer(daq_data_t *inBuf) {
    const String delimiter = String(",");
    const String newline = String("\r\n");

    String sensorData = "";

    sensorData = String(inBuf->sensorA) 
                + delimiter + String(inBuf->sensorB)
                + delimiter + String(inBuf->sensorC)
                + newline;

    return sensorData;
}

Caller of SDDataSerializer became:

  daq_data_t sensor_data = {.sensorA = 3.14, .sensorB=-3.14, .sensorC=0.998};

  String daq_str = SDDataSerializer(&sensor_data);

  Serial.print(daq_str.c_str());

Just tested it with an SD card and it is working. Thanks a lot.