From dc2a2a8ccd6b48b1f869f423043435b81c377863 Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sat, 11 Mar 2017 10:13:52 +0000 Subject: [PATCH 1/5] Don't include util/*.h files from ModbusMaster.h These files contain static functions that will cause unused function warnings when ModbusMaster.h is included by applications. --- examples/Basic/Basic.pde | 1 + examples/PhoenixContact_nanoLC/PhoenixContact_nanoLC.pde | 1 + examples/RS485_HalfDuplex/RS485_HalfDuplex.ino | 1 + src/ModbusMaster.cpp | 5 +++++ src/ModbusMaster.h | 5 ----- 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/examples/Basic/Basic.pde b/examples/Basic/Basic.pde index 74c199b..013dfeb 100644 --- a/examples/Basic/Basic.pde +++ b/examples/Basic/Basic.pde @@ -22,6 +22,7 @@ */ #include +#include // instantiate ModbusMaster object diff --git a/examples/PhoenixContact_nanoLC/PhoenixContact_nanoLC.pde b/examples/PhoenixContact_nanoLC/PhoenixContact_nanoLC.pde index e91fb33..5536237 100644 --- a/examples/PhoenixContact_nanoLC/PhoenixContact_nanoLC.pde +++ b/examples/PhoenixContact_nanoLC/PhoenixContact_nanoLC.pde @@ -23,6 +23,7 @@ */ #include +#include // discrete coils #define NANO_DO(n) (0x0000 + n) ///< returns nanoLC discrete output address diff --git a/examples/RS485_HalfDuplex/RS485_HalfDuplex.ino b/examples/RS485_HalfDuplex/RS485_HalfDuplex.ino index c19607f..8af8494 100644 --- a/examples/RS485_HalfDuplex/RS485_HalfDuplex.ino +++ b/examples/RS485_HalfDuplex/RS485_HalfDuplex.ino @@ -27,6 +27,7 @@ */ #include +#include /*! We're using a MAX485-compatible RS485 Transceiver. diff --git a/src/ModbusMaster.cpp b/src/ModbusMaster.cpp index 4169e58..bed458e 100644 --- a/src/ModbusMaster.cpp +++ b/src/ModbusMaster.cpp @@ -29,6 +29,11 @@ Arduino library for communicating with Modbus slaves over RS232/485 (via RTU pro /* _____PROJECT INCLUDES_____________________________________________________ */ #include "ModbusMaster.h" +// functions to calculate Modbus Application Data Unit CRC +#include "util/crc16.h" + +// functions to manipulate words +#include "util/word.h" /* _____GLOBAL VARIABLES_____________________________________________________ */ diff --git a/src/ModbusMaster.h b/src/ModbusMaster.h index 8c433e6..2191927 100644 --- a/src/ModbusMaster.h +++ b/src/ModbusMaster.h @@ -54,11 +54,6 @@ Set to 1 to enable debugging features within class: /* _____PROJECT INCLUDES_____________________________________________________ */ -// functions to calculate Modbus Application Data Unit CRC -#include "util/crc16.h" - -// functions to manipulate words -#include "util/word.h" /* _____CLASS DEFINITIONS____________________________________________________ */ From ab07d5c49d510ee14a41035e46ed33a30f4d5b3c Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sat, 11 Mar 2017 11:04:49 +0000 Subject: [PATCH 2/5] Add callback functions to log sent/received messages The library only returns an error code with no other explanation so it's impossible to debug unexpected errors in communication such as: - Receiving continuous noise on the line (missing ground wire) - Receiving additional zero bytes (missing bias resistors) Allow callback functions to be specified so that messages can be logged for debugging. --- src/ModbusMaster.cpp | 51 ++++++++++++++++++++++++++++++++++++++++++++ src/ModbusMaster.h | 6 ++++++ 2 files changed, 57 insertions(+) diff --git a/src/ModbusMaster.cpp b/src/ModbusMaster.cpp index bed458e..6b9221d 100644 --- a/src/ModbusMaster.cpp +++ b/src/ModbusMaster.cpp @@ -51,6 +51,8 @@ ModbusMaster::ModbusMaster(void) _idle = 0; _preTransmission = 0; _postTransmission = 0; + _logTransmit = 0; + _logReceive = 0; } /** @@ -221,6 +223,45 @@ void ModbusMaster::postTransmission(void (*postTransmission)()) _postTransmission = postTransmission; } +/** +Set transmit log callback function. + +This function gets called before a Modbus message is sent. + +Typical usage of this callback is for debugging the messages that are +sent on the bus. + +@see ModbusMaster::ModbusMasterTransaction() +@see ModbusMaster::logReceive() +*/ +void ModbusMaster::logTransmit(void (*logTransmit)(const uint8_t *data, + size_t length)) +{ + _logTransmit = logTransmit; +} + +/** +Set receive log callback function. + +This function gets called after a Modbus message is received. + +It will only be called with the data that has been read, this may be +shorter than the data on the bus if there was an error in the header. +The status is provided to allow this to be detected or to only log +messages where there was an error. + +Typical usage of this callback is for debugging the messages that are +received on the bus. + +@see ModbusMaster::ModbusMasterTransaction() +@see ModbusMaster::logReceive() +*/ +void ModbusMaster::logReceive(void (*logReceive)(const uint8_t *data, + size_t length, uint8_t status)) +{ + _logReceive = logReceive; +} + /** Retrieve data from response buffer. @@ -706,6 +747,11 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) u8ModbusADU[u8ModbusADUSize++] = highByte(u16CRC); u8ModbusADU[u8ModbusADUSize] = 0; + if (_logTransmit) + { + _logTransmit(u8ModbusADU, u8ModbusADUSize); + } + // flush receive buffer before transmitting request while (_serial->read() != -1); @@ -826,6 +872,11 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) } } + if (_logReceive) + { + _logReceive(u8ModbusADU, u8ModbusADUSize, u8MBStatus); + } + // disassemble ADU into words if (!u8MBStatus) { diff --git a/src/ModbusMaster.h b/src/ModbusMaster.h index 2191927..6ab008b 100644 --- a/src/ModbusMaster.h +++ b/src/ModbusMaster.h @@ -70,6 +70,8 @@ class ModbusMaster void idle(void (*)()); void preTransmission(void (*)()); void postTransmission(void (*)()); + void logTransmit(void (*)(const uint8_t *data, size_t length)); + void logReceive(void (*)(const uint8_t *data, size_t length, uint8_t status)); // Modbus exception codes /** @@ -255,6 +257,10 @@ class ModbusMaster void (*_preTransmission)(); // postTransmission callback function; gets called after a Modbus message has been sent void (*_postTransmission)(); + // logTransmit callback function; gets called before writing a Modbus message + void (*_logTransmit)(const uint8_t *data, size_t length); + // logReceive callback function; gets called after reading a Modbus message + void (*_logReceive)(const uint8_t *data, size_t length, uint8_t status); }; #endif From 1df4097f28ea84142b07cf47831779913ddcf9f0 Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sat, 11 Mar 2017 12:59:07 +0000 Subject: [PATCH 3/5] Fix potential buffer overflow receiving messages A slave device could indicate that there are 255 bytes in the response here: u8BytesLeft = u8ModbusADU[2]; There are only 256 bytes of space in ModbusADU so the additional 255 bytes would overflow the buffer. Increase the size of ModbusADUSize so that it is large enough to handle the byte at index 255 without wrapping around to 0, and use it to check that there is enough space for the response. When we run out of space, return a new error type ku8MBResponseTooLarge (0xE4). --- src/ModbusMaster.cpp | 97 ++++++++++++++++++++++++-------------------- src/ModbusMaster.h | 9 ++++ 2 files changed, 61 insertions(+), 45 deletions(-) diff --git a/src/ModbusMaster.cpp b/src/ModbusMaster.cpp index 6b9221d..1e34a03 100644 --- a/src/ModbusMaster.cpp +++ b/src/ModbusMaster.cpp @@ -646,16 +646,17 @@ Modbus transaction engine. uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) { uint8_t u8ModbusADU[256]; - uint8_t u8ModbusADUSize = 0; - uint8_t i, u8Qty; + uint16_t u16ModbusADUSize = 0; + uint16_t i; + uint8_t u8Qty; uint16_t u16CRC; uint32_t u32StartTime; - uint8_t u8BytesLeft = 8; + uint16_t u8BytesLeft = 8; uint8_t u8MBStatus = ku8MBSuccess; // assemble Modbus Request Application Data Unit - u8ModbusADU[u8ModbusADUSize++] = _u8MBSlave; - u8ModbusADU[u8ModbusADUSize++] = u8MBFunction; + u8ModbusADU[u16ModbusADUSize++] = _u8MBSlave; + u8ModbusADU[u16ModbusADUSize++] = u8MBFunction; switch(u8MBFunction) { @@ -664,10 +665,10 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) case ku8MBReadInputRegisters: case ku8MBReadHoldingRegisters: case ku8MBReadWriteMultipleRegisters: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16ReadAddress); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16ReadAddress); - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16ReadQty); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16ReadQty); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16ReadAddress); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16ReadAddress); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16ReadQty); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16ReadQty); break; } @@ -679,38 +680,38 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) case ku8MBWriteSingleRegister: case ku8MBWriteMultipleRegisters: case ku8MBReadWriteMultipleRegisters: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16WriteAddress); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16WriteAddress); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16WriteAddress); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16WriteAddress); break; } switch(u8MBFunction) { case ku8MBWriteSingleCoil: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16WriteQty); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16WriteQty); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16WriteQty); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16WriteQty); break; case ku8MBWriteSingleRegister: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16TransmitBuffer[0]); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16TransmitBuffer[0]); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16TransmitBuffer[0]); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16TransmitBuffer[0]); break; case ku8MBWriteMultipleCoils: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16WriteQty); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16WriteQty); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16WriteQty); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16WriteQty); u8Qty = (_u16WriteQty % 8) ? ((_u16WriteQty >> 3) + 1) : (_u16WriteQty >> 3); - u8ModbusADU[u8ModbusADUSize++] = u8Qty; + u8ModbusADU[u16ModbusADUSize++] = u8Qty; for (i = 0; i < u8Qty; i++) { switch(i % 2) { case 0: // i is even - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16TransmitBuffer[i >> 1]); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16TransmitBuffer[i >> 1]); break; case 1: // i is odd - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16TransmitBuffer[i >> 1]); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16TransmitBuffer[i >> 1]); break; } } @@ -718,38 +719,38 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) case ku8MBWriteMultipleRegisters: case ku8MBReadWriteMultipleRegisters: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16WriteQty); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16WriteQty); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16WriteQty << 1); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16WriteQty); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16WriteQty); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16WriteQty << 1); for (i = 0; i < lowByte(_u16WriteQty); i++) { - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16TransmitBuffer[i]); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16TransmitBuffer[i]); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16TransmitBuffer[i]); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16TransmitBuffer[i]); } break; case ku8MBMaskWriteRegister: - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16TransmitBuffer[0]); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16TransmitBuffer[0]); - u8ModbusADU[u8ModbusADUSize++] = highByte(_u16TransmitBuffer[1]); - u8ModbusADU[u8ModbusADUSize++] = lowByte(_u16TransmitBuffer[1]); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16TransmitBuffer[0]); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16TransmitBuffer[0]); + u8ModbusADU[u16ModbusADUSize++] = highByte(_u16TransmitBuffer[1]); + u8ModbusADU[u16ModbusADUSize++] = lowByte(_u16TransmitBuffer[1]); break; } // append CRC u16CRC = 0xFFFF; - for (i = 0; i < u8ModbusADUSize; i++) + for (i = 0; i < u16ModbusADUSize; i++) { u16CRC = crc16_update(u16CRC, u8ModbusADU[i]); } - u8ModbusADU[u8ModbusADUSize++] = lowByte(u16CRC); - u8ModbusADU[u8ModbusADUSize++] = highByte(u16CRC); - u8ModbusADU[u8ModbusADUSize] = 0; + u8ModbusADU[u16ModbusADUSize++] = lowByte(u16CRC); + u8ModbusADU[u16ModbusADUSize++] = highByte(u16CRC); + u8ModbusADU[u16ModbusADUSize] = 0; if (_logTransmit) { - _logTransmit(u8ModbusADU, u8ModbusADUSize); + _logTransmit(u8ModbusADU, u16ModbusADUSize); } // flush receive buffer before transmitting request @@ -760,12 +761,12 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) { _preTransmission(); } - for (i = 0; i < u8ModbusADUSize; i++) + for (i = 0; i < u16ModbusADUSize; i++) { _serial->write(u8ModbusADU[i]); } - u8ModbusADUSize = 0; + u16ModbusADUSize = 0; _serial->flush(); // flush transmit buffer if (_postTransmission) { @@ -776,12 +777,18 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) u32StartTime = millis(); while (u8BytesLeft && !u8MBStatus) { + if (u16ModbusADUSize >= sizeof(u8ModbusADU)) + { + u8MBStatus = ku8MBResponseTooLarge; + break; + } + if (_serial->available()) { #if __MODBUSMASTER_DEBUG__ digitalWrite(__MODBUSMASTER_DEBUG_PIN_A__, true); #endif - u8ModbusADU[u8ModbusADUSize++] = _serial->read(); + u8ModbusADU[u16ModbusADUSize++] = _serial->read(); u8BytesLeft--; #if __MODBUSMASTER_DEBUG__ digitalWrite(__MODBUSMASTER_DEBUG_PIN_A__, false); @@ -802,7 +809,7 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) } // evaluate slave ID, function code once enough bytes have been read - if (u8ModbusADUSize == 5) + if (u16ModbusADUSize == 5) { // verify response is for correct Modbus slave if (u8ModbusADU[0] != _u8MBSlave) @@ -855,18 +862,18 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) } // verify response is large enough to inspect further - if (!u8MBStatus && u8ModbusADUSize >= 5) + if (!u8MBStatus && u16ModbusADUSize >= 5) { // calculate CRC u16CRC = 0xFFFF; - for (i = 0; i < (u8ModbusADUSize - 2); i++) + for (i = 0; i < (u16ModbusADUSize - 2); i++) { u16CRC = crc16_update(u16CRC, u8ModbusADU[i]); } // verify CRC - if (!u8MBStatus && (lowByte(u16CRC) != u8ModbusADU[u8ModbusADUSize - 2] || - highByte(u16CRC) != u8ModbusADU[u8ModbusADUSize - 1])) + if (!u8MBStatus && (lowByte(u16CRC) != u8ModbusADU[u16ModbusADUSize - 2] || + highByte(u16CRC) != u8ModbusADU[u16ModbusADUSize - 1])) { u8MBStatus = ku8MBInvalidCRC; } @@ -874,7 +881,7 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) if (_logReceive) { - _logReceive(u8ModbusADU, u8ModbusADUSize, u8MBStatus); + _logReceive(u8ModbusADU, u16ModbusADUSize, u8MBStatus); } // disassemble ADU into words @@ -886,7 +893,7 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) case ku8MBReadCoils: case ku8MBReadDiscreteInputs: // load bytes into word; response bytes are ordered L, H, L, H, ... - for (i = 0; i < (u8ModbusADU[2] >> 1); i++) + for (i = 0; i < (uint8_t)(u8ModbusADU[2] >> 1); i++) { if (i < ku8MaxBufferSize) { @@ -912,7 +919,7 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) case ku8MBReadHoldingRegisters: case ku8MBReadWriteMultipleRegisters: // load bytes into word; response bytes are ordered H, L, H, L, ... - for (i = 0; i < (u8ModbusADU[2] >> 1); i++) + for (i = 0; i < (uint8_t)(u8ModbusADU[2] >> 1); i++) { if (i < ku8MaxBufferSize) { diff --git a/src/ModbusMaster.h b/src/ModbusMaster.h index 6ab008b..8ab0d52 100644 --- a/src/ModbusMaster.h +++ b/src/ModbusMaster.h @@ -185,6 +185,15 @@ class ModbusMaster */ static const uint8_t ku8MBInvalidCRC = 0xE3; + /** + ModbusMaster response too large exception. + + The response is too large to fit in the receive buffer. + + @ingroup constant + */ + static const uint8_t ku8MBResponseTooLarge = 0xE4; + uint16_t getResponseBuffer(uint8_t); void clearResponseBuffer(); uint8_t setTransmitBuffer(uint8_t, uint16_t); From ea3664cf8b59d0699c06ef58d366932e22f6d702 Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sat, 11 Mar 2017 13:02:21 +0000 Subject: [PATCH 4/5] Check that Stream::read() has not returned -1 (no data available) If Stream::read() returns -1 then 0xFF will be added to the buffer. Check the return value before using it as data. --- src/ModbusMaster.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/ModbusMaster.cpp b/src/ModbusMaster.cpp index 1e34a03..d56bcc8 100644 --- a/src/ModbusMaster.cpp +++ b/src/ModbusMaster.cpp @@ -788,8 +788,13 @@ uint8_t ModbusMaster::ModbusMasterTransaction(uint8_t u8MBFunction) #if __MODBUSMASTER_DEBUG__ digitalWrite(__MODBUSMASTER_DEBUG_PIN_A__, true); #endif - u8ModbusADU[u16ModbusADUSize++] = _serial->read(); - u8BytesLeft--; + int data = _serial->read(); + + if (data != -1) + { + u8ModbusADU[u16ModbusADUSize++] = data; + u8BytesLeft--; + } #if __MODBUSMASTER_DEBUG__ digitalWrite(__MODBUSMASTER_DEBUG_PIN_A__, false); #endif From c298f454faba6119758c12fee29a6a4e3dd124f9 Mon Sep 17 00:00:00 2001 From: Kaloyan Mihaylov Date: Wed, 10 Jul 2019 10:37:41 +0300 Subject: [PATCH 5/5] Fix uninitialized variable read in requestFrom --- src/ModbusMaster.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ModbusMaster.cpp b/src/ModbusMaster.cpp index d56bcc8..cdcc534 100644 --- a/src/ModbusMaster.cpp +++ b/src/ModbusMaster.cpp @@ -90,7 +90,7 @@ void ModbusMaster::beginTransmission(uint16_t u16Address) // eliminate this function in favor of using existing MB request functions uint8_t ModbusMaster::requestFrom(uint16_t address, uint16_t quantity) { - uint8_t read; + uint8_t read = 0; // clamp to buffer length if (quantity > ku8MaxBufferSize) {