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

Memory leak? #57

Open
TXswe opened this issue Apr 13, 2021 · 2 comments
Open

Memory leak? #57

TXswe opened this issue Apr 13, 2021 · 2 comments

Comments

@TXswe
Copy link

TXswe commented Apr 13, 2021

I am not the most experienced coder, so i could have something wrong, but I have tried to minimize the code to replicate the issue. The code i do now is simple create a single modbus registry, and just re-add data to it.

I get less then 200 loops (often around 175), then the Arduino crashes.
If I modify to add more/less variables, just changes when it crashes.
If I use a mega, instead of an uno, it just takes longer as it has more memory, but same result.

#include <Ethernet.h>
#include <Modbus.h>
#include <ModbusIP.h>


//Modbus Registers Offsets (0-9999)
const int reg01CurXPos = 1;  // Current X Position

//ModbusIP object
ModbusIP mb;

//Variables
unsigned int looper = 0;


void setup() {
    Serial.begin(9600);
    // The media access control (ethernet hardware) address for the shield
    byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };
    // The IP address for the shield
    byte ip[] = { 192, 168, 1, 175};
    //Config Modbus IP
    mb.config(mac, ip);
    
    Serial.println("Setup done");
    delay(1000);
}

void loop() {
  mb.addHreg(reg01CurXPos, 1);
  delay(10);
  mb.task(); // Call update on registrys for Modbus.

  Serial.print("Loop - ");
  Serial.println(looper);
  looper++;

  delay(10);
}

Any ideas on what is going on?

@percramer
Copy link

why would you be adding modbus registers in the main loop? The first line (mb.addHreg(reg01CurXPos, 1);)? you would probably do that once in your setup for the needed registers.

@TXswe
Copy link
Author

TXswe commented Apr 14, 2021

I see your thinking and i ended up solving the issue in a different way.
So this is not a critical issue for me atm, it was more a info that there is a potential serious memory hole in that command.

I did need to add some more registries in the main loop, on certain situations.
As I looked in the modbus.h i saw:
#define MAX_REGS 32
... and it fooled me into thinking i would at max get 32 registries anyways.
And ofc, if you add one registry with the same number as an existing one, its good practice (of what i have learned on my little coding time) to check if an existing registry is there, or to remove the old when the new one is added, so u dont get data a memory leak.

I never expected the "add" command to simply just add, until you crash the hardware, regardless of limits, and that is the bug i reported here.

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

No branches or pull requests

2 participants