Hi guys,
I have a mysteries problem around a delete call.
What I am doing: I have a loop, that is checks elements in a List and deletes them if they meet a certain condition.
This looks like this (it is already full with debug prints):
debug->OUT->println(F("check_pending_out_events(a)"));
//go through all pending events
for(int idx = pending_serial_out_events->size()-1; idx >= 0; idx--) {
SerialOutEvent *soe = pending_serial_out_events->get(idx);
if(soe->gotAck()) {
//[...]
} else {
//was not acknowlaged -> check if its time to retry
if(soe->retry_due()) {
//we reached the retry limit?
if(soe->try_counter >= EVENT_RETRYS) {
//we tried already to often -> time to give up...
//remove from list
pending_serial_out_events->remove(idx);
debug->print_module_name(MODULE_SERIAL_COM);
debug->OUT->print(F("we didn't received and ack for event / sequence: "));
debug->OUT->println(soe->squence_number);
debug->OUT->println(F("check_pending_out_events(b)"));
delete soe; //HERE WE HANG
debug->OUT->println(F("check_pending_out_events(c)"));
} else {
[...]
}
}
}
}
debug->OUT->println(F("check_pending_out_events(d)"));
It looks like the program never returns from the delete call of the class "SerialOutEvent". I have no idea why...
This happens always, not after some time.
This is the class:
class SerialOutEvent {
public:
//generates the squence numbers for the serial out events
static byte squence_number_generator;
//the sequence number of this event
byte squence_number;
//the payload. we save the payload here, in case we have to send this event again
byte *payload;
//the length of the payload this object carries
byte payload_length;
//number of times, we tried to send this package and didn't got an answer
byte try_counter;
//our flags
SlimBitArray flags;
//timer for measure the acknowlage time time
FF_Timer timer;
SerialOutEvent(byte payload_length) {
update_squence_number();
payload = new byte[payload_length];
this->payload_length = payload_length;
//we wait two seconds for the answer, then we send the event again
try_counter = 0;
}
~SerialOutEvent() {
delete[] payload;
}
//[...]
};
byte SerialOutEvent::squence_number_generator = 0;
My Serial Monitor looks like this:
...
check_pending_out_events(a)
SerialCom: we didn't received and ack for event / sequence: 0
check_pending_out_events(b)
"check_pending_out_events(c)" is never reached...
I would appreciate if you guys have a quick look over my code mess, maybe you see something that I overlooked.
I also checked already if there is enough SRAM available and I still have 6274bytes free...
Never, never, EVER use new, malloc, etc. without checking the return value. These calls CAN fail.
Never, never, EVER use delete, free, etc. without ensuring the thing you're deleting, freeing, etc., actually WERE allocated.
Either of the above will leave you with a pointer that points to somewhere you REALLY don't want to read or especially write.
In nearly every case, a delete call failing is the result of attempting to delete an object that was never created.
In nearly every case, a free call failing is the result of attempting to free a block of memory that was never allocated.
Either of the above can blow up the free memory list and crash your program.
Pointers should ALWAYS be initialized to NULL, so you can tell whether or not they are set, and should be reset to NULL after being deleted or freed, so you can tell whether or not they have been deleted or freed.
You initialize your itemPtr to NULL initially, and from then on, resizeBuff() will return true or false whether or not it was successful in allocating your memory. You can call it on pre-allocated memory blocks and it will resize them for you. Calling it with 0 bytes leaves you with a NULL pointer again. I find it very very handy.
What happens to that object's "place in line" after you delete it? You just call delete on the pointer, I don't see any cleaning up of the internal list or queue that was storing it. My mind is screaming in agony looking at this.
Memory management in C++ requires strict discipline. When you're allocating heap memory, it is necessary to determine who "owns" the new object. Ownership is not a language feature, but a higher level concept that is necessary because of C++'s lack of automatic memory management.
Since you're getting this pointer from a get() function, that looks like the object that "owns" the pointer. Give this object a remove() function that takes the index or the pointer and deletes the object, along with reconfiguring whatever internal structure is keeping track of these objects. Deleting it outside the structure like this is just asking for trouble. Never do this.
First, thank you for your replies
What I want to do, boils down to this two functions:
The first one creates the Objects in question and put them into an List. The second one is removing them from the list and is supposed to delete them, since the only pointers to the objects are kept in the list. My entire program (which is huge) runs just fine without this one delete call, it just crashes after some minutes when the SRAM is eventually full, since I don't call delete. Bit with delete it crashes at the first delete call...
/**
* Send out event from the digital inputs
*/
void send_di_events() {
//leave if we don't have DI list
if(di_list == NULL) {
return;
}
//check all inputs for events
for(int idx_di = 0; idx_di < di_list->size(); idx_di++) {
DI *di = di_list->get(idx_di);
//get event byte
byte di_events = di->getEvents();
//check if a event bit is set
if(di_events != 0) {
SerialOutEvent *soe = new SerialOutEvent(3);
soe->payload[0] = COM_INPUT_EVENT;
soe->payload[1] = (byte) idx_di;
soe->payload[2] = di_events; //send all event bits
pending_serial_out_events->add(soe);
pending_serial_out_events->trim();
//send out the event
sendSerialOutEvent(soe);
if(debug->LEVEL >= 3) {
debug->print_module_name(MODULE_SERIAL_COM);
debug->OUT->print(F("sent out an event: DI: "));
debug->OUT->print(idx_di);
debug->OUT->print(F(" / event bits: "));
debug->OUT->print(di_events, BIN);
debug->OUT->print(F(" / sequence: "));
debug->OUT->print(soe->squence_number);
debug->OUT->println();
}
}
}
}
/**
* Checks for unacknowlaged and acknowlaged SerialOutEvents
*/
void check_pending_out_events() {
debug->OUT->println(F("check_pending_out_events(a)"));
//go through all pending events
for(int idx = pending_serial_out_events->size()-1; idx >= 0; idx--) {
SerialOutEvent *soe = pending_serial_out_events->get(idx);
if(soe->gotAck()) {
//was acknowledged -> remove it from the list
pending_serial_out_events->remove(idx);
delete soe;
} else {
//was not acknowlaged -> check if its time to retry
if(soe->retry_due()) {
//we reached the retry limit?
if(soe->try_counter >= EVENT_RETRYS) {
//we tried already to often -> time to give up...
//remove from list
pending_serial_out_events->remove(idx);
debug->print_module_name(MODULE_SERIAL_COM);
debug->OUT->print(F("we didn't received and ack for event / sequence: "));
debug->OUT->println(soe->squence_number);
debug->OUT->println(F("check_pending_out_events(b)"));
delete soe;
debug->OUT->println(F("check_pending_out_events(c)"));
} else {
//time for retry -> send data again and hope for the best :-)
sendSerialOutEvent(soe);
}
}
}
}
debug->OUT->println(F("check_pending_out_events(d)"));
}
This is just weird. It may compile but its not the standard way.. Try it like this..
byte* payload;
payload = (byte*) malloc(payload_length); // Have a go at allocating it.
if (payload) { // If you got it..
//do stuff..
}
// Then when you are done with it
free(payload); // recycle the RAM.
payload = NULL; // As a flag that its empty.
Will this help? Donno' but it is a tried and true method.
Assuming OP isn't programming for an AVR processor, it might be better to use one of the main Container classes available in the STL rather than trying to roll their own. Those classes handle dynamic allocation / deallocation properly.
malloc()
This function does not call constructors or initialize memory in any way. There are no ready-to-use smart pointers that could guarantee that the matching deallocation function is called. The preferred method of memory allocation in C++ is using RAII-ready functions std::make_unique, std::make_shared, container constructors, etc, and, in low-level library code, new-expression.
R.10: Avoid malloc() and free()
Reason: malloc() and free() do not support construction and destruction, and do not mix well with new and delete.
If you need dynamic memory in C++, use the following (in order):
Use standard containers like std::vector
Use smart pointers such as std::unique_ptr, created using std::make_unique
In low-level code for implementing your own custom containers, use new and delete if other containers don't suit your specific needsOn AVR, you don't have access to the standard library out of the box. There are third-party ports of the standard library, or other third-party libraries for dynamic containers. Abstracting away memory allocation in container classes (RAII) is always preferable to code with raw owning pointers and calls to new/delete mixed with the logic of the program.
Thank you so much for your help and suggestions. I just identified the problem and fixed it, now all is working fine. The Problem was not in the code that I posted, but it is this construction that let the program crash, no idea why?!
class Class2 {
public:
byte *pnt;
Class2() {
Class2(8);
}
Class2(byte len) {
Serial.println("Class2()");
pnt = new byte[len];
}
~Class2() {
Serial.println("~Class2()");
delete[] pnt;
}
};
class Class1 {
public:
//Class2 foo = Class2(8); //-> this works fine
Class2 foo; //-> this crashes
};
void loop() {
Serial.println('A');
delay(100);
Class1 *c1 = new Class1();
Serial.println('B');
delay(100);
delete c1;
Serial.println('C');
delay(100);
delay(1000);
}
This is not how you call another constructor. This is creating a local Class2 object that is immediately destroyed when constructor exits. The members of this object remain uninitialized. If you want to forward arguments to another constructor use this syntax:
Class2() : Class2(8) {}
Also, having a raw pointer like that exposed for the whole world to see is like flashing your junk in public. It's really bad.
. Too much Java biased...