Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential memory leak #14

Open
sreekants opened this issue Nov 29, 2022 · 8 comments
Open

Potential memory leak #14

sreekants opened this issue Nov 29, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@sreekants
Copy link

In LoRa_E22.cpp a buffer is allocated in each call

...
ResponseStructContainer LoRa_E22::receiveMessageComplete(const uint8_t size, bool rssiEnabled){
ResponseStructContainer rc;

rc.data = malloc(size);
   ...

However, in all of the samples, you appear not to release the memory, for example esp32_e22_04_SendFixedTransmission.ino:

...
if (e22ttl.available()>1) {
// read the String message
#ifdef ENABLE_RSSI
ResponseContainer rc = e22ttl.receiveMessageRSSI();
#else
ResponseContainer rc = e22ttl.receiveMessage();
#endif
...

Isn't rc.close() required to release the memory? Also, is there a way to avoid malloc() every time you receive a message to avoid fragmentation?

@strud
Copy link

strud commented Nov 29, 2022

Hi sreekants,

I had this issue recently and yes you must call rc.close() every cycle to release the memory.

I have also suggested it be added to the example code.

Maybe a better approach is to allocate the maximum allowable per message ie 256 bytes and then leave it allocated for ever?

@sreekants
Copy link
Author

Hi sreekants,

I had this issue recently and yes you must call rc.close() every cycle to release the memory.

I have also suggested it be added to the example code.

Maybe a better approach is to allocate the maximum allowable per message ie 256 bytes and then leave it allocated for ever?

I like the idea of a pre-allocated buffer managed by the library. That would be a good fix with backward compatibility if the close() function has an empty body.

@xreef
Copy link
Owner

xreef commented Dec 1, 2022

Hi all,
no, the ResponseContainer doesn't need a close function because there isn't a pointer but directly a complex variable.
Bye Renzo

@xreef xreef closed this as completed Dec 1, 2022
@sreekants
Copy link
Author

Hi Renzo,
Where is the corresponding free() to the buffer allocated on line 780? The pointer rc.data holds a pointer to this buffer, but the only call releasing this buffer is ResponseContainer::close(). However, I do not see it being called.

rc.data = malloc(size);

Can you explain how the heap is managed correctly with the library?

(We arrived at this problem because we were transmitting 220 byte frame over LoRa E22 in our software and when we monitored the heap it fell from 180Kb, steadily to 70kb as our stress tests ran through a few hours.)

@xreef
Copy link
Owner

xreef commented Dec 1, 2022

Hi @sreekants,
you select the ResponseStructContainer that needs close

ResponseStructContainer LoRa_E22::receiveMessageComplete(const uint8_t size, bool rssiEnabled){
ResponseStructContainer rc;
rc.data = malloc(size);
rc.status.code = this->receiveStruct((uint8_t *)rc.data, size);
if (rc.status.code!=E22_SUCCESS) {
return rc;
}
if (rssiEnabled){
char rssi[1];
this->serialDef.stream->readBytes(rssi, 1);
rc.rssi = rssi[0];
}
this->cleanUARTBuffer();
return rc;
}

and It had It

struct ResponseStructContainer {
void *data;
byte rssi;
ResponseStatus status;
void close() {
free(this->data);
}
};

but the ResponseContainer doesn't have and doesn't need a close.

struct ResponseStructContainer {
void *data;
byte rssi;
ResponseStatus status;
void close() {
free(this->data);
}
};
struct ResponseContainer {
String data;
byte rssi;
ResponseStatus status;
};

Bye Renzo

@sreekants
Copy link
Author

Yes, I understand now. I stand corrected. Your samples use ResponseContainer. So they do not need a close(). You are correct there.

In our usecase, however, we are sending binary data over LoRa. The ResponseContainer is suitable only for string data. This is why we end up calling receiveMesssageComplete(). As stated, each call allocates a buffer, fragmenting memory, even if the close() function is called. Is there a better way to allow the use of a preallocated buffer so we don't have to malloc() every loop?

@xreef
Copy link
Owner

xreef commented Dec 1, 2022

Ahhh ok, now I understand,
mmm... no I don't think about that, but It's an interesting feature and can be a good enhancement.
I think of a solution.
If you have some suggestions write here.
Bye Renzo

@xreef xreef reopened this Dec 1, 2022
@xreef xreef added the enhancement New feature or request label Dec 1, 2022
@fixajteknik
Copy link

I think I asked that question and you made some update and then I share a video about this issue 2 years ago #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants