Do not return or modify values that do not exist

Author: epsilonrt

src/Modbus.cpp

@@ -124,23 +124,6 @@ bool  Modbus::regOutOfBounds (word address, word value) {
   return false;
 }
 
-//-------------------------------------------------------------------------------
-int Modbus::setAdditionalServerData (const char data[]) {
-
-  free (_additional_data);
-  if (data) {
-    size_t l = strlen (data) + 1;
-
-    _additional_data = (char *) malloc (l);
-    if (_additional_data) {
-
-      strcpy (_additional_data, data);
-      return l;
-    }
-  }
-  return 0;
-}
-
 //-------------------------------------------------------------------------------
 // protected
 void Modbus::receivePDU (byte *frame) {
@@ -204,6 +187,7 @@ void Modbus::receivePDU (byte *frame) {
 }
 
 //-------------------------------------------------------------------------------
+// private
 void Modbus::exceptionResponse (byte fcode, byte excode) {
   //Clean frame buffer
   free (_frame);
@@ -215,6 +199,40 @@ void Modbus::exceptionResponse (byte fcode, byte excode) {
   _reply = MB_REPLY_NORMAL;
 }
 
+/* Func 06: Write Single Register
+    Request
+      Function code     1 Byte  0x06
+      Register Address  2 Bytes 0x0000 to 0xFFFF
+      Register Value    2 Bytes 0x0000 to 0xFFFF
+    Response
+      Function code     1 Byte  0x06
+      Register Address  2 Bytes 0x0000 to 0xFFFF
+      Register Value    2 Bytes 0x0000 to 0xFFFF
+*/
+//-------------------------------------------------------------------------------
+// private
+void Modbus::writeSingleRegister (word reg, word value) {
+
+  if (hregOutOfBounds (reg, value)) {
+    exceptionResponse (MB_FC_WRITE_REG, MB_EX_ILLEGAL_VALUE);
+    return;
+  }
+
+  //Check Address and execute (reg exists?)
+  if (!setHreg (reg, value)) {
+    exceptionResponse (MB_FC_WRITE_REG, MB_EX_ILLEGAL_ADDRESS);
+    return;
+  }
+
+  //Check for failure ? Why ???
+  /*
+    if (hreg (reg) != value) {
+    exceptionResponse (MB_FC_WRITE_REG, MB_EX_SLAVE_FAILURE);
+    return;
+    }
+  */
+  _reply = MB_REPLY_ECHO; // reply with received frame
+}
 
 /*
   Func 03: Read Holding Registers
@@ -228,21 +246,23 @@ void Modbus::exceptionResponse (byte fcode, byte excode) {
       Register value        N* x 2 Bytes
      N = Quantity of Registers
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::readRegisters (word startreg, word numregs) {
   //Check value (numregs)
   if (numregs < 1 || numregs > 125) {
     exceptionResponse (MB_FC_READ_REGS, MB_EX_ILLEGAL_VALUE);
     return;
   }
 
-  //Check Address
-  //*** See comments on readCoils method.
-  if (!searchRegister (startreg + TRegister::HregOffset)) {
-    exceptionResponse (MB_FC_READ_REGS, MB_EX_ILLEGAL_ADDRESS);
-    return;
+  //Check Address (startreg...startreg + numregs - 1)
+  for (word k = 0; k < numregs; k++) {
+    if (!searchRegister (startreg + TRegister::HregOffset + k)) {
+      exceptionResponse (MB_FC_WRITE_REGS, MB_EX_ILLEGAL_ADDRESS);
+      return;
+    }
   }
 
-
   //Clean frame buffer
   free (_frame);
   _len = 0;
@@ -275,39 +295,6 @@ void Modbus::readRegisters (word startreg, word numregs) {
   _reply = MB_REPLY_NORMAL;
 }
 
-/* Func 06: Write Single Register
-    Request
-      Function code     1 Byte  0x06
-      Register Address  2 Bytes 0x0000 to 0xFFFF
-      Register Value    2 Bytes 0x0000 to 0xFFFF
-    Response
-      Function code     1 Byte  0x06
-      Register Address  2 Bytes 0x0000 to 0xFFFF
-      Register Value    2 Bytes 0x0000 to 0xFFFF
-*/
-void Modbus::writeSingleRegister (word reg, word value) {
-
-  if (hregOutOfBounds (reg, value)) {
-    exceptionResponse (MB_FC_WRITE_REG, MB_EX_ILLEGAL_VALUE);
-    return;
-  }
-
-  //Check Address and execute (reg exists?)
-  if (!setHreg (reg, value)) {
-    exceptionResponse (MB_FC_WRITE_REG, MB_EX_ILLEGAL_ADDRESS);
-    return;
-  }
-
-  //Check for failure ?
-  /*
-    if (hreg (reg) != value) {
-    exceptionResponse (MB_FC_WRITE_REG, MB_EX_SLAVE_FAILURE);
-    return;
-    }
-  */
-  _reply = MB_REPLY_ECHO; // reply with received frame
-}
-
 /* Func 16: Write Multiple registers
     Request
       Function code         1 Byte        0x10
@@ -321,14 +308,16 @@ void Modbus::writeSingleRegister (word reg, word value) {
       Starting Address      2 Bytes       0x0000 to 0xFFFF
       Quantity of Registers 2 Bytes       1 to 123 (0x7B)
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::writeMultipleRegisters (byte *frame, word startreg, word numoutputs, byte bytecount) {
   //Check value
   if (numoutputs < 1 || numoutputs > 123 || bytecount != 2 * numoutputs) {
     exceptionResponse (MB_FC_WRITE_REGS, MB_EX_ILLEGAL_VALUE);
     return;
   }
 
-  //Check Address (startreg...startreg + numregs)
+  //Check Address (startreg...startreg + numregs - 1)
   for (word k = 0; k < numoutputs; k++) {
     if (!searchRegister (startreg + TRegister::HregOffset + k)) {
       exceptionResponse (MB_FC_WRITE_REGS, MB_EX_ILLEGAL_ADDRESS);
@@ -380,21 +369,26 @@ void Modbus::writeMultipleRegisters (byte *frame, word startreg, word numoutputs
       Coil Status       n Byte  n = N or N+1
      N = Quantity of Outputs / 8, if the remainder is different of 0 -> N = N+1
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::readCoils (word startreg, word numregs) {
   //Check value (numregs)
   if (numregs < 1 || numregs > 2000) {
     exceptionResponse (MB_FC_READ_COILS, MB_EX_ILLEGAL_VALUE);
     return;
   }
 
-  //Check Address
-  //Check only startreg. Is this correct?
   //When I check all registers in range I got errors in ScadaBR
   //I think that ScadaBR request more than one in the single request
   //when you have more then one datapoint configured from same type.
-  if (!searchRegister (startreg + TRegister::CoilOffset)) {
-    exceptionResponse (MB_FC_READ_COILS, MB_EX_ILLEGAL_ADDRESS);
-    return;
+  // epsilonrt -> We absolutely must not return values that do not exist !!
+
+  //Check Address (startreg...startreg + numregs - 1)
+  for (word k = 0; k < numregs; k++) {
+    if (!searchRegister (startreg + TRegister::CoilOffset + k)) {
+      exceptionResponse (MB_FC_WRITE_REGS, MB_EX_ILLEGAL_ADDRESS);
+      return;
+    }
   }
 
   //Clean frame buffer
@@ -452,18 +446,21 @@ void Modbus::readCoils (word startreg, word numregs) {
      N = Quantity of Inputs / 8 if the remainder is different of 0  N = N+1
   Error
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::readInputStatus (word startreg, word numregs) {
   //Check value (numregs)
   if (numregs < 0x0001 || numregs > 0x07D0) {
     exceptionResponse (MB_FC_READ_INPUT_STAT, MB_EX_ILLEGAL_VALUE);
     return;
   }
 
-  //Check Address
-  //*** See comments on readCoils method.
-  if (!searchRegister (startreg + TRegister::IstsOffset)) {
-    exceptionResponse (MB_FC_READ_COILS, MB_EX_ILLEGAL_ADDRESS);
-    return;
+  //Check Address (startreg...startreg + numregs - 1)
+  for (word k = 0; k < numregs; k++) {
+    if (!searchRegister (startreg + TRegister::IstsOffset + k)) {
+      exceptionResponse (MB_FC_WRITE_REGS, MB_EX_ILLEGAL_ADDRESS);
+      return;
+    }
   }
 
   //Clean frame buffer
@@ -520,18 +517,21 @@ void Modbus::readInputStatus (word startreg, word numregs) {
       Input Registers             N* x 2 Bytes
      N = Quantity of Input Registers
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::readInputRegisters (word startreg, word numregs) {
   //Check value (numregs)
   if (numregs < 0x0001 || numregs > 0x007D) {
     exceptionResponse (MB_FC_READ_INPUT_REGS, MB_EX_ILLEGAL_VALUE);
     return;
   }
 
-  //Check Address
-  //*** See comments on readCoils method.
-  if (!searchRegister (startreg + TRegister::IregOffset)) {
-    exceptionResponse (MB_FC_READ_COILS, MB_EX_ILLEGAL_ADDRESS);
-    return;
+  //Check Address (startreg...startreg + numregs - 1)
+  for (word k = 0; k < numregs; k++) {
+    if (!searchRegister (startreg + TRegister::IregOffset + k)) {
+      exceptionResponse (MB_FC_WRITE_REGS, MB_EX_ILLEGAL_ADDRESS);
+      return;
+    }
   }
 
   //Clean frame buffer
@@ -576,6 +576,8 @@ void Modbus::readInputRegisters (word startreg, word numregs) {
       Output Address  2 Bytes 0x0000 to 0xFFFF
       Output Value    2 Bytes 0x0000 or 0xFF00
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::writeSingleCoil (word reg, word status) {
   //Check value (status)
   if (status != 0xFF00 && status != 0x0000) {
@@ -584,16 +586,18 @@ void Modbus::writeSingleCoil (word reg, word status) {
   }
 
   //Check Address and execute (reg exists?)
-  if (!setCoil (reg, (bool) status)) {
+  if (!setCoil (reg, status != 0)) {
     exceptionResponse (MB_FC_WRITE_COIL, MB_EX_ILLEGAL_ADDRESS);
     return;
   }
 
-  //Check for failure
+  //Check for failure ?? Why ?
+  /*
   if (coil (reg) != (bool) status) {
     exceptionResponse (MB_FC_WRITE_COIL, MB_EX_SLAVE_FAILURE);
     return;
   }
+  */
 
   _reply = MB_REPLY_ECHO; // reply with received frame
 }
@@ -610,6 +614,8 @@ void Modbus::writeSingleCoil (word reg, word status) {
       Starting Address    2 Bytes 0x0000 to 0xFFFF
       Quantity of Outputs 2 Bytes 0x0001 to 0x07B0
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::writeMultipleCoils (byte *frame, word startreg, word numoutputs, byte bytecount) {
   //Check value
   word bytecount_calc = numoutputs / 8;
@@ -673,6 +679,8 @@ void Modbus::writeMultipleCoils (byte *frame, word startreg, word numoutputs, by
       Run Indicator Status  1 Byte            0x00 = OFF, 0xFF = ON
       Additional Data
 */
+//-------------------------------------------------------------------------------
+// private
 void Modbus::reportServerId() {
   //Clean frame buffer
   free (_frame);
@@ -695,5 +703,22 @@ void Modbus::reportServerId() {
   _reply = MB_REPLY_NORMAL;
 }
 
+//-------------------------------------------------------------------------------
+int Modbus::setAdditionalServerData (const char data[]) {
+
+  free (_additional_data);
+  if (data) {
+    size_t l = strlen (data) + 1;
+
+    _additional_data = (char *) malloc (l);
+    if (_additional_data) {
+
+      strcpy (_additional_data, data);
+      return l;
+    }
+  }
+  return 0;
+}
+
 //-------------------------------------------------------------------------------
 #endif // USE_HOLDING_REGISTERS_ONLY not defined