andresarmento / modbus-arduino

A library that allows your Arduino to communicate via Modbus protocol, acting as a slave (master in development). Supports serial (RS-232, RS-485) and IP via Ethernet (Modbus IP).
BSD 3-Clause "New" or "Revised" License
453 stars 267 forks source link

Memory leak? #57

Open TXswe opened 3 years ago

TXswe commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.