w5500 no mem for receive buffer (IDFGH-7516) #9083
Comments
github-actions
bot
changed the title
MadDogMayCry0
changed the title
|
UPD. ... What does it mean. This is an absurd piece of code. UPD UPD |
|
Ok, a crunch to prevent memory leaks when packets too intencive or data is too big. static void emac_w5500_task(void *arg)
{
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
uint32_t length = 0;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
do {
ESP_LOGE("toys.c","%d",heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
length = ETH_MAX_PACKET_SIZE;
//Prevent memory leak
if(heap_caps_get_free_size(MALLOC_CAP_INTERNAL)<120000){
buffer = heap_caps_malloc(length, MALLOC_CAP_DMA);
while (emac->packets_remain){
//flush buffer
if(emac->parent.receive(&emac->parent, buffer, &length)){
buffer = NULL;
}
}
free(buffer);
}
buffer = heap_caps_malloc(length, MALLOC_CAP_DMA);
if(!buffer){
//Memory leak
w5500_reset(emac);
ESP_LOGE(TAG, "no mem for receive buffer");
break;
} else if (emac->parent.receive(&emac->parent, buffer, &length) == ESP_OK) {
/* pass the buffer to stack (e.g. TCP/IP layer) */
if (length) {
emac->eth->stack_input(emac->eth, buffer, length);
} else {
free(buffer);
}
} else {
free(buffer);
}
}
while (emac->packets_remain);
}
}
vTaskDelete(NULL);
}If you have a better way pls remind me! |
|
@suda-morris any particular reason to allocate max ethernet frame sized buffers? |
I think becuse of this peace (wtf???) static void emac_w5500_task(void *arg)
{
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
uint32_t length = 0;
//rxBuffer length
uint16_t rxLen = 0;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
//get rx length
w5500_get_rx_received_size(emac, &rxLen);
printf("rxLen:%d\r\n",rxLen);
int iter = 0;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
do {
//Wtf???????????
printf("iter:%d rxLen:%d\r\n",iter,rxLen);
iter++;
// ESP_LOGE("toys.c","%d",heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
length = ETH_MAX_PACKET_SIZE;
//Prevent memory leak
// if(heap_caps_get_free_size(MALLOC_CAP_INTERNAL)<120000){
// buffer = heap_caps_malloc(length, MALLOC_CAP_DMA);
// while (emac->packets_remain){
// //flush buffer
// if(emac->parent.receive(&emac->parent, buffer, &length)){
// buffer = NULL;
// }
// }
// free(buffer);
// }
buffer = heap_caps_malloc(length, MALLOC_CAP_DMA);
if(!buffer){
//Memory leak - reboot module
w5500_reset(emac);
ESP_LOGE(TAG, "no mem for receive buffer");
break;
} else if (emac->parent.receive(&emac->parent, buffer, &length) == ESP_OK) {
/* pass the buffer to stack (e.g. TCP/IP layer) */
if (length) {
emac->eth->stack_input(emac->eth, buffer, length);
} else {
free(buffer);
}
} else {
free(buffer);
}
}
while (emac->packets_remain);
}
}
vTaskDelete(NULL);
}50 peaces of "\r\n" with 98 bytes of length was sended I think all of this code have to be redisigned. |
|
And this variant show something strange static void emac_w5500_task(void *arg)
{
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
uint32_t length = 0;
//rxBuffer length
uint16_t rxLen = 0;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
//get rx length
int iter = 0;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
do {
w5500_get_rx_received_size(emac, &rxLen);
//Wtf???????????
printf("iter:%d rxLen:%d\r\n",iter,rxLen);
iter++;
// ESP_LOGE("toys.c","%d",heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
length = ETH_MAX_PACKET_SIZE;
//Prevent memory leak
// if(heap_caps_get_free_size(MALLOC_CAP_INTERNAL)<120000){
// buffer = heap_caps_malloc(length, MALLOC_CAP_DMA);
// while (emac->packets_remain){
// //flush buffer
// if(emac->parent.receive(&emac->parent, buffer, &length)){
// buffer = NULL;
// }
// }
// free(buffer);
// }
buffer = heap_caps_malloc(length, MALLOC_CAP_DMA);
if(!buffer){
//Memory leak - reboot module
w5500_reset(emac);
ESP_LOGE(TAG, "no mem for receive buffer");
break;
} else if (emac->parent.receive(&emac->parent, buffer, &length) == ESP_OK) {
/* pass the buffer to stack (e.g. TCP/IP layer) */
if (length) {
emac->eth->stack_input(emac->eth, buffer, length);
} else {
free(buffer);
}
} else {
free(buffer);
}
}
while (emac->packets_remain);
}
}
vTaskDelete(NULL);
}50 peaces of "\r\n" with 98 bytes of length was sended |
|
UPD printf("iter:%d rxLen:%d\r\n",iter,__builtin_bswap16( rxLen ) - 2); |
|
So, this works as i see. But bug is not desapears. Have to flush it when leak. This variant can recive more data btw spending much less memory and effort... I think I've proven the block of code wrong. It requires a complete redesign and rethinking. static void emac_w5500_task(void *arg){
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
uint32_t length = 0;
//rxBuffer length
uint16_t rxLen = 0;
// int iter = 0;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
//get rx length
w5500_get_rx_received_size(emac, &rxLen);
if(rxLen<1){
continue;
}
buffer = heap_caps_malloc(rxLen, MALLOC_CAP_DMA);
// printf("rxLen:%d heap:%d\r\n",rxLen,heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
//flush buffer
if(heap_caps_get_free_size(MALLOC_CAP_INTERNAL)<120000){
while (emac->packets_remain){
emac->parent.receive(&emac->parent, buffer, &length);
}
}
if(!buffer){
ESP_LOGE(TAG, "no mem for receive buffer");
w5500_reset(emac);
continue;
}
if (emac->parent.receive(&emac->parent, buffer, &length) == ESP_OK) {
/* pass the buffer to stack (e.g. TCP/IP layer) */
emac->eth->stack_input(emac->eth, buffer, length);
}
}
}
vTaskDelete(NULL);
} |
|
The problem as I see it is some strange behavior of the buffer. Either the buffer is incorrectly calculated, or the buffer itself is somehow difficult to distribute frames within itself. But it's not possible to have 5642 bytes for 5st iteration and for only 10 bytes data that was sended before. And as expected, if we manually allocate something like 256 bytes per line break, we see that all 50 iterations will work very well. It turns out that the value of the incoming buffer is incorrectly calculated. Or it is calculated correctly, but something does not have time to reset the previous values before a new calculation, and as a result, under certain conditions, the result is incorrect. |
espressif-bot
assigned kostaond and euripedesrocha and unassigned david-cermak, kostaond and euripedesrocha
|
Ok, size function must be something like so uint16_t offset = 0;
uint16_t len = 0;
w5500_read(emac, W5500_REG_SOCK_RX_RD(0), &offset, sizeof(offset));
offset = __builtin_bswap16(offset);
w5500_read_buffer(emac, &len, sizeof(len), offset);
rxLen = __builtin_bswap16(len) - 2;but i think better way is to have static sized uint8_t arr[16384] and after we recive buffer and know about length in this arr, we can then allocate memory to put this data in stack. static void emac_w5500_task(void *arg){
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
uint32_t length = 0;
//rxBuffer length
uint16_t rxLen = 0;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
//get rx length
uint16_t offset = 0;
uint16_t len = 0;
w5500_read(emac, W5500_REG_SOCK_RX_RD(0), &offset, sizeof(offset));
offset = __builtin_bswap16(offset);
w5500_read_buffer(emac, &len, sizeof(len), offset);
rxLen = __builtin_bswap16(len) - 2;
if(rxLen < 1){
continue;
}
//mem allocation
buffer = heap_caps_malloc(rxLen, MALLOC_CAP_DMA);
printf("rxLen:%d heap:%d\r\n",rxLen,heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
if(!buffer){
ESP_LOGE(TAG, "no mem for receive buffer");
w5500_reset(emac);
continue;
}
if (emac->parent.receive(&emac->parent, buffer, &length) == ESP_OK) {
/* pass the buffer to stack (e.g. TCP/IP layer) */
emac->eth->stack_input(emac->eth, buffer, length);
continue;
}
free(buffer);
}
}
vTaskDelete(NULL);
}UPD and about this function. static esp_err_t w5500_get_rx_received_size(emac_w5500_t *emac, uint16_t *size)
{
esp_err_t ret = ESP_OK;
uint16_t received0, received1 = 0;
do {
ESP_GOTO_ON_ERROR(w5500_read(emac, W5500_REG_SOCK_RX_RSR(0), &received0, sizeof(received0)), err, TAG, "read RX RSR failed");
ESP_GOTO_ON_ERROR(w5500_read(emac, W5500_REG_SOCK_RX_RSR(0), &received1, sizeof(received1)), err, TAG, "read RX RSR failed");
} while (received0 != received1);
*size = __builtin_bswap16(received0);
err:
return ret;
}i think it's not possible to have some data inbeetween requests, and even so, you become this data later in another frame. static esp_err_t w5500_get_rx_received_size(emac_w5500_t *emac, uint16_t *size){
esp_err_t ret = ESP_OK;
uint16_t res = 0;
ESP_GOTO_ON_ERROR(w5500_read(emac, W5500_REG_SOCK_RX_RSR(0), &res, sizeof(res)), err, TAG, "read RX RSR failed");
*size = __builtin_bswap16(res);
err:
return ret;
}tests Happy end? :) |
I think LwIP assumes the buffer is dynamically allocated. This means it attempts to
Up to your application. In my driver, by default, I chose a 1 tick delay.
Then you should read the datasheet.
The benefit of polling RX_RSR for a stable value is reading the entire ring buffer in one go, which is more efficient, at least on its own. |
Thats right, thats why i told about allocation after we get length of data in buffer
So, then you have to copy data from static array to dynamic block of memory and then put it to stack. But anyway this is not good way. Faster as any time request for buffer size, but bad. I don't like this way.
If you are ready to demonstrate that such a situation is practically possible, then I would like to reproduce it.
There is an extra loop in the code, which does not lead to the desired result, but only creates an unnecessary load. But even under such conditions, you would not get intermediate data into the buffer. Without this cycle, receiving intermediate data is excluded. The code described in the library does not achieve the expected behavior. There is too much theory that is not reproduced in practice. Banal tests instantly reveal all the flaws in the logic that were implemented by its author. By the way, this test demonstrates that frames are not able to stick together even if it is sent one after another. The module never sees more than one block at a time there, and the next block cannot in any way wedge into the previous one. We would need a delay between these two probes before we see another block. But taking into account this delay, we would quickly consider the block in the next frame and would have time to connect significantly more blocks already in the stack, and not in the procedure for checking glued frames in this function. UPD Yes, you are right. But.. uint16_t res_a, res_b = 0;
int trg = 0;
do{
w5500_read(emac, W5500_REG_SOCK_RX_RSR(0), &res_a, sizeof(res_a));
w5500_read(emac, W5500_REG_SOCK_RX_RSR(0), &res_b, sizeof(res_b));
trg++;
}
while(res_a != res_b);
printf("- %d -\r\n",trg);
uint16_t size = __builtin_bswap16(res_a);
buffer = heap_caps_malloc(4096, MALLOC_CAP_DMA);
printf("res:%d heap:%d\r\n",size,heap_caps_get_free_size(MALLOC_CAP_INTERNAL));sending data result but as i said it's cost much more time, and we need a delay between probe to get it to work correclty. In any case, this code have to be redisigned! All or huge part of it. UPD :) |
|
UPD |
|
UPD Ok, then. |
|
UPD. static void emac_w5500_task(void *arg){
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
uint16_t offset = 0;
uint16_t rx_len = 0;
uint16_t remain_bytes = 0;
w5500_get_rx_received_size(emac, &remain_bytes);
printf("rx_size:%d heap:%d\r\n",remain_bytes,heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
while(remain_bytes){
// get current read pointer
w5500_read(emac, W5500_REG_SOCK_RX_RD(0), &offset, sizeof(offset));
offset = __builtin_bswap16(offset);
// read head first
w5500_read_buffer(emac, &rx_len, sizeof(rx_len), offset);
rx_len = __builtin_bswap16(rx_len) - 2; // data size includes 2 bytes of header
offset += 2;
//Memory allocation
buffer = heap_caps_malloc(rx_len, MALLOC_CAP_DMA);
if(!buffer){
break;
}
// read the payload
w5500_read_buffer(emac, buffer, rx_len, offset);
// update read pointer
offset += rx_len;
offset = __builtin_bswap16(offset);
w5500_write(emac, W5500_REG_SOCK_RX_RD(0), &offset, sizeof(offset));
/* issue RECV command */
w5500_send_command(emac, W5500_SCR_RECV, 100);
remain_bytes -= rx_len + 2;
// stacking
emac->eth->stack_input(emac->eth, buffer, rx_len);
}
}
}
vTaskDelete(NULL);
} |
|
UPD static void emac_w5500_task(void *arg){
emac_w5500_t *emac = (emac_w5500_t *)arg;
uint8_t status = 0;
uint8_t *buffer = NULL;
while (1) {
// check if the task receives any notification
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(1000)) == 0 && // if no notification ...
gpio_get_level(emac->int_gpio_num) != 0) { // ...and no interrupt asserted
continue; // -> just continue to check again
}
/* read interrupt status */
w5500_read(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
/* packet received */
if (status & W5500_SIR_RECV) {
status = W5500_SIR_RECV;
// clear interrupt status
w5500_write(emac, W5500_REG_SOCK_IR(0), &status, sizeof(status));
uint16_t offset = 0;
uint16_t rx_len = 0;
uint16_t remain_bytes = 0;
w5500_get_rx_received_size(emac, &remain_bytes);
printf("size:%d heap:%d\r\n",remain_bytes,heap_caps_get_free_size(MALLOC_CAP_INTERNAL));
//Memory allocation
buffer = heap_caps_malloc(remain_bytes, MALLOC_CAP_DMA);
if(!buffer){
continue;
}
uint16_t res = 0;
if(remain_bytes){
while(remain_bytes){
// get current read pointer
w5500_read(emac, W5500_REG_SOCK_RX_RD(0), &offset, sizeof(offset));
offset = __builtin_bswap16(offset);
// read head first
w5500_read_buffer(emac, &rx_len, sizeof(rx_len), offset);
rx_len = __builtin_bswap16(rx_len) - 2; // data size includes 2 bytes of header
offset += 2;
// read the payload
w5500_read_buffer(emac, buffer+res, rx_len, offset);
// update read pointer
offset += rx_len;
offset = __builtin_bswap16(offset);
w5500_write(emac, W5500_REG_SOCK_RX_RD(0), &offset, sizeof(offset));
/* issue RECV command */
w5500_send_command(emac, W5500_SCR_RECV, 100);
remain_bytes -= rx_len + 2;
res += rx_len;
}
printf("stacking\r\n");
emac->eth->stack_input(emac->eth, buffer, res);
}
}
}
vTaskDelete(NULL);
}works bad, but shows how stack works, ans why we cant just get a whole buffer from module. |
|
You now know there 2 ways from getting the size of the data to be read from the chip. Personally, I went with reading the 2 byte header of each packet because I found it to be simpler. I actually got to write and test the RX_RSR polling method, but gave up on it because in order to be superior to the other method, it had to be paired up up with passing managed buffers to LwIP. Since it's not possible due to ESP-NETIF demanding ownership of the buffer by the fact that it frees it up by itself, it didn't make much sense to go with this method. |
Clear! So, i do use this one #9083 (comment) last days, and it's seems to works stable. Need add some additionals checks to get it to work in final. This on is sux #9083 (comment) and i don't know why it's not works as expected. Something strange here. :) |
espressif-bot
added
the
Status: Selected for Development
espressif-bot
added
Resolution: Duplicate
vscode @ last esp-idf
w5500 module
WROOM32
AsyncTCP Server example
Putty as a tcp client
When i send 4027 bytes over tcp/ip i have "w5500.mac: no mem for receive buffer"
the problem is here.
The text was updated successfully, but these errors were encountered: