Object removed from Queue does not properly reflect the item that was put onto the Queue

Kernel
1

I have a task running, on a dedicated core, whose sole responsibility is to monitor a queue, pull items off of the queue and act upon the instructions contained therein (it’s not important what it’s actually doing, but for the sake of completeness, let’s imagine that the instructions are for displaying messages on an OLED display)

The structure of the object going onto the queue is as follows

    struct MessageInfo
    {
    private:
      char _message[64];
    public:
      const TickType_t period = 0;
      const line_positions position;
      const display_properties properties;
      const uint message_length = 0;
      const char * message = nullptr;

      MessageInfo() : position(NO_POSITION), properties(NO_PROPERTY) { }
      MessageInfo(TickType_t period_) 
         : period(period_), position(NO_POSITION), properties(WAIT) { }
      MessageInfo(line_positions position_, display_properties properties_, const char * message_, uint length)
        : position(position_), properties(properties_), message_length(length)
      { 
        if(length == 0) { return; }    
        strcpy(_message, message_);
        message = &_message[0];
      }
    };

The API is essentially a disconnected Facade to the OLED controls and as such exposes methods for writing to the OLED display, one such method might be:

    void Gsdc_SSD1306::scrollToCenter(line_positions position, String message) 
    {
        MessageInfo info(position, SCROLL_TO_CENTER, message.c_str(), message.length());
        xQueueSendToBack(_messageQueue, (void *)&info, portMAX_DELAY);
    }

The point behind this is so that instructions can be sent to the OLED display which are themselves executed asynchronously so that the main program can continue with it’s processing unhindered.

The task which runs on another core thus …

    xTaskCreatePinnedToCore(messageTask, "MSG_TSK", 20480, (void *)_taskServices, 2, NULL, 1);

Will then take items from the queue and process them:

    void messageTask(void * parameter)
    {
        task_data data = *((task_data *)parameter);
    
        for( ;; )
        {
            MessageInfo currentMessage;
            MessageInfo peekMessage;
            if(xQueueReceive(data.messageQueue, &currentMessage, (TickType_t) 10) == pdPASS)
            {
                data.set_running();
                Message message(&currentMessage, data.display);
                while(!message.display()){ vTaskDelay(1);} 

                if(!xQueuePeek(data.messageQueue, &peekMessage, (TickType_t) 10))
                    data.set_stopped();
            }
        }
    }

The issue that I am experiencing is that when the MessageInfo is copied from the queue and into my currentMessage, upon inspection the only value which is as expected is MessageInfo::message, i.e. the values for members MessageInfo::properties, MessageInfo::position and MessageInfo::message_length bear no resemblance to the values which were submitted.

I’m pretty certain that it’s something I’m doing (or not), but I am currently at a loss as to what the issue might be!

2

I believe the above is your problem. If you pass a POINTER to a stack into the queue, the data on the stack will be gone after the function terminates. You must put a copy of the message itself into the queue, not a reference. At least not a reference to volatile data.

We’ve all been there, I guess…

3

Unfortunately, that does not agree with the documentation …

BaseType_t xQueueSendToBack(
                               QueueHandle_t xQueue,
                               const void * pvItemToQueue,
                               TickType_t xTicksToWait
                           );
> *pvItemToQueue* A pointer to the item that is to be placed on the queue. The size of the items the queue will hold was defined when the queue was created, so this many bytes will be copied from pvItemToQueue into the queue storage area.

… and also, MessageInfo::message does actually hold the expected data.

4

May have misread, sorry. How do you create your queue?

5

_messageQueue = xQueueCreate(64, sizeof(MessageInfo));

6

ok, contrary what I suspected earlier, the queue code looks ok. Apologies again

What strikes me funny is this:

MessageInfo currentMessage;

even though MessageInfo is declared as a struct and not a class, it has a constructor and member functions and thus a vtable. When you allocate a member of that on the stack, the constructor is called etc. I’m not sure if it is acceptable C++ to overwrite an allocated object including a vtable. Possibly the sizeof operator for a C++ object does not include the vtable or has different ideas about the true size of the object than the runtime. When you do call the xQueueCreate() function, what is the real value passed into it as the item size parameter? Does that correspond to the size of the MessageInfo object as you inspect it in the debugger?

1 Like
7

Sadly … I am writing the code using Visual Code on a a Linux machine and I have not set up any remote debugging on the microcontroller I am using.

I’m wondering if you’ve not identified the issue, and I should not try storing an item with a v-table.

I’ll have a look at changing things so that the message object only contains value items

8

One BIG issue, Queue copy by memcpy, and thus can ONLY be used with POD structures.

MessageInfo is NOT POD, as It has a constructor, and this will NOT be run when the copy is made, but the pointers in the copy will still point to the original object.

(In fact, that looks to be a defect in your class definition too, if you copy a MessageInfo, it does not reset the message pointer.)

(Note, it isn’t a ‘vtable’ issue as it has no virtual functions, it is a copy-constructor issue)

1 Like
9

Just to let you know (anyone that is interested). I changed my queued object to a POD

struct Message_Info
{
  ulong period = 0;
  line_positions position = NO_POSITION;
  display_properties properties = NO_PROPERTY;
  char message[64] = { };
};

… and I no longer have any unexpected behaviour.

10

Please explain this, Richard. Do you mean the problem is that the copy constructor (cc) is NOT being called by bitwise copying of the object? Or that a cc is called after the copy (how?) and therebye manipulates the data members?

In any case, I think we agree that it is not a good idea to use an object like a structure. The internal representations of objects are opaque, thus no code should rely on any assumption about the ordering of members.

11

Is it possible that you accidentally corrupted the trailing members when doing (bad) strcpy without length checking in the first layout version of the class ?
Seems to me that the binary layout change having the char buffer at the end of the object was the important change to make it work (somehow). You really should ensure that you don’t exceed the char buffer when copying the string into it ! This also corrupts the stack even though it seems to work (for now).
Using POD data types or structs is usually better and easier to follow, but basically binary copying member pointers (to preferably globally visible objects e.g. from heap) can be done to transfer things by reference. This is also possible for stack allocated objects if you take care about their lifetime resp. their scope to avoid that they’re destructed while their pointers are in use e…g by the receiver task.
However, you have to know EXACTLY what you’re doing when dealing with binary or bitwise copies of objects (with or without hidden vtable pointers) without running through their corresponding constructors.

1 Like
12

FreeRTOS, being written in C knows nothing of copy constructors, so just does a bit wise copy.

My comment about it being a copy constructor issue is that the original class has a member message, that is a pointer into the buffer inside it. Even if C++, if you copy this class, unless YOU define a copy constructor to do this, the copy will continue to point to the original objects _message buffer, and will exhibit this sort of error.

Any time you have a class with a pointer member in it, you need to think about copy constructors. If it points internally, the pointer likely needs to be adjusted when you copy it. If the pointer is to something external, do you mean to be sharing the external object, does it need to know it is shared, or do you need to clone that object so each has its own.

13

Even if C++, if you copy this class, unless YOU define a copy constructor to do this, the copy will continue to point to the original objects _message buffer, and will exhibit this sort of error.

Yeah, is it painfully clear that the past 17 years have been spent using higher level languages?

14

Yes, C is a very powerful language, able to let you express things with great efficiency, but you need to give it very detailed instructions.

C++ lets you automate much of this drudgery, and lets you push a lot of the details out of sight, the problem being if you forget something, it is now out of sight, and the power now replicates that error rapidly.

In C++, when you can just USE the well designed components, things get a lot easier, when you need to build one, you have to spend some of that energy you will save in the use.

When combining C++ and C, you need to remember that C won’t do the automatic stuff that C++ did, so you need to be careful.

15

Some 20 years ago now, I spent a good 18 months porting a C app to C++, but then when they released the .net framework, that’s where my energies went.

Messing around with microcontrollers is really my first venture back into the embedded world in, like mentioned, probably around 17 years.

I know that I knew more than I’ve forgotten -because I remember bits and pieces- but I still feel that there is a wealth of information and experience hiding away in my brain somewhere, I just need to find it :rofl:

This forum, however, I think I will find invaluable whilst I take the journey.