memcpy problems when switching from arduino to espIDF

[mtraven]

Been working on moving an arduino framed project into a native IDF (4.4.7), for the first time. I worked through my arduino version to try and make it as compatible as possible before moving it over....removing String** usage & such.

My program uses a linked list with a void *data pointer that points to different types of structures. The nodes store the type, so accessing is just a matter of reading *data into a structure of the appropriate type. To do this, I use memcpy. Worked fine in my arduino version, but in IDF I've got some 80 errors like:

Code: Select all

void* memcpy(void*, const void*, size_t)' writing to an object of type 'struct butData' with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Werror=class-memaccess]",

here is the node struct for the linked list:

Code: Select all

typedef struct fNode
{
    void *data;
    uint8_t type;
    uint16_t flags;
}fNode;

here is what one of the structs looks like:

Code: Select all

typedef struct butData
{
    uint8_t ID, type;
    uint8_t  mouseState:3;
    Rect absPos;
    int8_t txtStartOffset;
    std::string fieldText; 
    fabgl::FontInfo const* fieldFont;
    uint8_t bg0, fg0, bg1, fg1, hoverColor, clickColor;
}butData;

Not gonna pretend to understand the error, but it seems like its because I have "non-trival members" of that struct (Rect, FontInfo, std:string)

here are the memcpy's I use to read & right to the node data: (read node is a temp instance of fNode, the linked list struct)

Code: Select all

memcpy(&curButData, readNode.data, sizeof(curButData));  // read
 memcpy(readNode.data, &curButData, sizeof(curButData));	 //write

seems like its ok with my write function, but flags errors on all the reads (some 80+ of them).

So first of all, I really need to understand why this wasn't a problem in arduino, and is in IDF? I know arduino framework has its differences, but this seems like straight C++ stuff.

Second, I need to figure out how to correct this. Kinda spinning my wheels on that right now. Do I need to write copy constructors for each of the structures? Do I need to use a different command to copy? Do I actually have a code issue, or do I need to fix something with the compiler? I mean, it all worked fine in arduino....

I can write some code, but I am by no means a programmer. Please keep that in mind with your responses.

Thanks so much for your attention, without community support like this, I could never accomplish the things do.


**on a side note, I had been working to break my use of String(arduino), after I read up on the problems it can make. In replacing all the String references, I used a mix of c and c++ strings (char *str & std::string). After using c++ strings, I saw they can do most of the things a String(ard) can do. Do they come with the same pitfalls of String? If not, why was the adruino String ever made?

[MicroController]

here are the memcpy's I use to read & right to the node data:

Code: Select all

memcpy(&curButData, readNode.data, sizeof(curButData));  // read
memcpy(&curButData, readNode.data, sizeof(curButData));		 //write

seems like its ok with my write function, but flags errors on all the reads (some 80+ of them).

So first of all, I really need to understand why this wasn't a problem in arduino, and is in IDF? I know arduino framework has its differences, but this seems like straight C++ stuff.

You can tell the compiler which types of issues ("warnings") it should ignore, for which it should issue warnings, and which warnings should be regarded as errors. This can cause one environment to 'cleanly' compile and another one to error out on the same code.

Second, I need to figure out how to correct this. Kinda spinning my wheels on that right now. Do I need to write copy constructors for each of the structures? Do I need to use a different command to copy? Do I actually have a code issue, or do I need to fix something with the compiler? I mean, it all worked fine in arduino....

A little testing indicates that the problem (or at least one of them) is "fabgl::FontInfo const* fieldFont". This pointer is a const and cannot be modified as far as C++ is concerned. That's why the compiler cannot generate/use default or trivial assignment - the compiler is not allowed to assign a new value to 'fieldFont'.

A quick hack which seems to work is to cast to a void*:

Code: Select all

memcpy((void*)&curButData, readNode.data, sizeof(curButData)); 

However... I must strongly advise against memcpy'ing objects like std::strings. - It may work if&when the source object a) is immutable and b) lives at least as long as the copy, but it's definitely 'undefined behaviour' and can easily cause much suffering.
(Example: After the memcpy of a std::string, you modify the string by adding or removing a character. This may cause the string object to make a new (longer or shorter) copy of the string's memory and then free the 'old' memory, of which the memcopy of the object does not know, happily referencing the now-invalid memory.)

**on a side note, I had been working to break my use of String(arduino), after I read up on the problems it can make. In replacing all the String references, I used a mix of c and c++ strings (char *str & std::string). After using c++ strings, I saw they can do most of the things a String(ard) can do. Do they come with the same pitfalls of String? If not, why was the adruino String ever made?

Not sure which specific problems you read about, but std::string and presumably Arduino String may both suffer the same issues w.r.t. heap memory (fragmentation) and/or performance.
The Arduino String seems to have a bunch of 'convenience' functions which std::string does not have. These functions are useful for an Arduino but somewhat at odds with some design principles of the C++ standard. That's probably why a) Arduino came up with their own String and b) std::string will not adopt the Arduino functionality.

[mtraven]

thank you for your thorough response. yes the String issues I was talking about were heap frag. Just seems like its a bad practice to use them, especially when memory is tight.

However... I must strongly advise against memcpy'ing objects like std::strings. -

2 things: the std::string used to be Strings(ard), I had no (**known**) issues from it, surely std:string is an improvement? But I see the point you illustrated with the adding characters example and want to try to do it "right". So what is the right way to read in a structure from memory, addressed by a void* ? Seems like the only way to address the extra characters scenario is to make them of fixed size?

Before I got your response, I was playing around with some casting that seems similar to your work around (i simplified fNode.data prt to just a pointer, just to make it simpler to work out)

Code: Select all

	butData *curButDataPtr = (butData*)(dataPtr);
        curButData =*curButDataPtr;

not sure if this works yet or not, but the syntax is accepted by the compiler. I was trying to do it with out first making the pointer:

Code: Select all

curDroData = *(droData*)dataPtr

but I cant find any combination of pointers that allows for it. generally tells me there is no member of the struct to deal with void*&.

there is like 80 instances of operations like this, tedious as it may be, I dont mind changing them, I just have to know the change is correct.

so if you were me, how would you read & right these structures to memory?

[MicroController]

how would you read & right these structures to memory?

That's a tough one I don't know enough about your applications specific circumstances. And it's generally one of the more complicated problems in C/C++ programming.

But generally, I'd approach the problem like this:
1) Decide/find out who 'owns' the source objects at which point in time. By 'owns' I mean which code/object is responsible for ultimately destroying it/releasing memory.
2) Decide if you actually need a copy of the source object or if using a pointer to the original is viable. You need a copy if a) you want a copy of the object's data that lives independent of any successive changes to the original, or can be modified without affecting the original, or b) you need the lifetime of the copy to be independent of the original's, i.e. you want an object you 'own' which stays there even after the original object's owner destroys its copy.
3) The clean way to make a copy of an object in C++ is via copy constructor or assignment operator. Both of which the compiler will provide for you if it can. If it can't, you'd provide an implementation of either one or both.
4) In your specific case, once you created a copy, you'd need to make sure it also is properly destroyed when not needed anymore. Specifically, the compiler doesn't know what object if any is behind a 'void*' and hence will not call any destructors.

With all that said, I think the easiest approach may be to use inheritance and compiler-generated copy constructors/assignment operators and destructors, like

Code: Select all

struct fNode
{
    const uint8_t type; // We must never change the type of an object.
    uint16_t flags {0};

    fNode(uint8_t type) : type {type} {
    }
};

struct ButtonNode : public fNode {
  butData button;
  ButtonNode() : fNode(BUTTON) {
  }
};

struct OtherNode : public fNode {
  otherData other;
  OtherNode() : fNode(OTHER) {
  }
};

ButtonNode* makeButtonNode(butData* data) {
  ButtonNode* node = new ButtonNode();
  // Let the compiler make a copy of the data object:
  node->button = *data; // <- Should actually be put into a constructor of ButtonNode...
  return node;
}

void freeNode(fNode* node) {
  // Destroy a node and release its memory.
  delete node; // Automatically destroys/frees any object inside.
}

void doSomehing(fNode* node) {
  if(node->type == BUTTON) {
    // The node is of type ButtonNode
    doButtonAction(&(((ButtonNode*)node)->button));
  } else
  if(node->type == OTHER) {
    // The node is of type OtherNode
    doOtherAction(&(((OtherNode*)node)->other));
  }
}

void setFlag(fNode* node) {
  // No need to inspect the node's type to access flags.
  node->flags = 1;
}

[mtraven]

1. boy, you weren't joking about not using memcpy on structs withe std::strings! Spent the last week or so removing every god forsaken std:string from my program! everything is fixed length, null terminated, c strings now, I like that much better.

2. I dont need to copy, like ever really. I never wanted to, I just hadn't figured out how to cast a void pointer into a struct. I didn't even know how to ask the question when I wrote the original memcpy version. copying it into a struct was the only way I could work out at the time. Once I had worked out the magic cast:

Code: Select all

droData *_ddata= (droData*)newNode.data

I replaced all 80+ instances of memcpy with direct addresses, so I'm not copying entire structures(in & out) to change 1 or 2 fields, as I was
code is MUCH cleaner, visually & how it runs.

Just wanted to say thanks again for your help.


PS I've got new problems, if ya wanna check out the thread I posted today(what can I say, I'm greedy for good help):
viewtopic.php?f=2&t=39273&p=130437#p130437

[MicroController]

Glad you could work out a clean solution

[ESP_Sprite]

Fwiw, the issue is indeed that you're copying C++ objects. Issue there is that you get a sort of 'siamese twin' if you do that. Say you have a string calles string1, and you set it to "hello". The string allocates memory for that 'hello' and stores a pointer to it inside the string object. Now you memcpy() the string object to, say, string2. Because the 'hello' memory is a region that memcpy knows nothing about, there still is only one of them, and both string1 and string2 point to that. Now you destroy string1; it dutifully frees its string memory, so "hello" is gone and then cleans up itself. Now string2 suddenly points to freed memory, and you have an issue.

The C++ solution is to not do a hard memory copy, but to let the objects figure out themselves how to make a duplicate of themselves. For instance, if you do 'String string2=string1', the C++ compiler will translate this into a call to the copy constructor of string1; in other words, it leaves the copying to some function in string1. As a string knows the details of how to produce a copy, including details like making a copy of the "hello" data, this will result in the thing you want.

I even think the default copy constructor of structs might simply call the copy constructor for all objects enclosed in it. So you might have gotten away with simply doing a cast and assign, so something like 'butData node=*(butData*)fnode.data;'.

I see you already resolved this in a different way, so this is only a fyi in general, you may want to read up on casting, because the memcpy in itself is not really necessary unless you insist on having a copy of the data (e.g. if you need to modify it while keeping the original data intact.)

[djixon]

Just to add some important thing to that already well explaied. In ESP-IDF, if you want to do dynamic type casting in your C++ code, you have to turn on generation of RTTI (in sdkconfig) to allow dynamic type casting at run-time.

That is not exactly related to your problem but keep it in mind.