Scope of data for xQueueSend in a callback function

Kernel
1

Hi

I have some misconception about the requirement of scope of data in a callback function when xQueueSend is involved. This type of question is rather basic so I hope it is not a kind of nonsense here.

I need to develop a BLE application with callback functions for BLE characteristic write. The pseudocode of the callback is listed below:

///first I have described a generic footprint for all queue data
typedef struct {
  enum_event_t event;
  void *pData;
} queue_descriptor_t;

///This is the callback function when there is a BLE message written to my device.
///When the incoming message (sender could be a smartphone app) is a correct JSON message like {"cmd":"show_a_list"},
///my device will post a message to a global message queue (gMessageQueue) with 
///variable json_response=128 as a simple illustration here
void onWrite(BLECharacteristic* pCharacteristic, esp_ble_gatts_cb_param_t* param)
{
 queue_descriptor_t txQueueMessage;
 uint8_t json_response;

 deserializeJson(doc, json_in_stream); //doc is a json document coming from the BLE server
 if(doc["cmd"] == "show_a_list")
 {
   json_response = 128;
   txQueueMessage.event = RESP_SHOW_LIST;
   txQueueMessage.pData = &json_respose;
   xQueueSend(gMessageQueue, (void*)&txQueueMessage, (TickType_t)0);
 }
}

The scenario is similar to data handling when ISR is involved such that we need to declare any data static to ensure its existence when the ISR service exits.

My question is: do I need to declare txQueueMessage and json_response above as static to protect them? Without static I find the message content is wrong from time to time.

John

2

static is not needed for txQueueMessage because queues (binary) copy in and out the message data. But txQueueMessage.pData gets a so called dangling pointer if it points to an object on stack and the code leaves the onWrite callback. Hence you must ensure that the lifetime of the object is appropriate if you want to transfer it by reference.
Also just declaring json_response static might be not the right choice because it’s value might get changed while a previously enqueued event is not yet consumed…
It’s much more simple and robust if you transfer everything by copy if the event payload is not too large and the frequency of the callback invocation is not too high.
So what about changing pData to an array sized to cover all possible event payloads ?
BTW Since you set the send timeout to 0 and don’t check the return code of xQueueSend you intentionally allow events getting dropped if the queue gets full, right ?

3

Thank you for the comment. Right, the data should have been protected until it is released by the queue receiving side. Otherwise, the variable uint8_t json_response may have been overwritten by the sender especially when its priority is higher than the receiver.

I think this bug is not restricted to ISR or callback function. Even in the task body if the variable is not protected, we may still run into the same issue with void *pData that points to the same memory.

Yes, but there are other data with different types and lengths outside this BLE task. I find keeping a generic data type allows me to use a single queue and data structure for any data type and source. So I still wish to use it although an extra caution is required.

For simplicity I have skipped that part. I do have a simple assertion test like:

if(xQueueSend(gMessageQueue, (void*)&txQueueMessage, (TickType_t)0) != pdPASS){
 printf("Exception in BLETask.cpp: %d \r\n", __LINE__);
}
4

Well, it’s a compromise. You can surely go on with by reference payload and use e.g. malloc/free to allocate the memory for the payload and transfer just the pointer. But this is also not for free and is often only worth the effort for large payloads and/or very limited RAM resources.
Or you define a (kind of) raw payload buffer and use it to transfer the payload by value.
With the event ID you know how to interpret the content of the raw payload buffer in the receiver task as you already do it with pData.
This would avoid the costs of dynamic allocations and it’s other drawbacks and would simplify your application. If you know up front the max size of all possible payloads you know the raw payload buffer size to reserve.
Just my 2 ct :slight_smile:

5

If you know the maximum number of buffers to be used concurrently and the max size of a single buffer is reasonable, you can also statically preallocate all your buffers in an array of buffers and pass the array index in the queue’s payload. All of this is a question of system design, and there is no one size fits all answer.

Just my 1 cent! :upside_down_face:

6

Does it mean a new queue descriptor like this:

typedef struct {
 const char sensor_name[16];
 uint8_t reg;
 uint16_t value;
} sensor_data_t;

typedef struct {
  enum_event_t event;
  sensor_data_t sensor_data;
} queue_descriptor_t;

void Task(void *pvParameters)
{
  (void)pvParameters;
  queue_descriptor_t xTransmitMessage;

  for(;;){
  xTransmitMessage.event = EVT_SENSOR;
  xTransmitMessage.sensor_data.value = fcn_sensor_data_get();
  //...
      if(xQueueSend(sensorMessageQueue, (void *)&xTransmitMessage, (TickType_t)0)!= pdPASS){
      Serial.print(F("Exception at line#: ")); Serial.println(__LINE__);
    }
  vTaskDelay(10);
  }
}
7

Yes, this is exactly what hs2@ meant. Now you are copying everything into the queue, so there is no question of dangling pointer or data races.

8

Just make sure to initialize your queue to the correct size!