From 34812e69bfa6002b149a291eb619b078ae2a81c7 Mon Sep 17 00:00:00 2001 From: Dima Dorezyuk Date: Mon, 20 Nov 2023 10:55:51 +0100 Subject: [PATCH 1/4] SerialCommHub: support longer transactions and chunking Signed-off-by: Dima Dorezyuk --- .../main/serial_communication_hubImpl.cpp | 6 +- .../main/serial_communication_hubImpl.hpp | 1 + modules/SerialCommHub/manifest.yaml | 6 ++ modules/SerialCommHub/tiny_modbus_rtu.cpp | 59 ++++++++++++++++--- modules/SerialCommHub/tiny_modbus_rtu.hpp | 6 +- 5 files changed, 67 insertions(+), 11 deletions(-) diff --git a/modules/SerialCommHub/main/serial_communication_hubImpl.cpp b/modules/SerialCommHub/main/serial_communication_hubImpl.cpp index af6135e6c..db2ed4f39 100644 --- a/modules/SerialCommHub/main/serial_communication_hubImpl.cpp +++ b/modules/SerialCommHub/main/serial_communication_hubImpl.cpp @@ -66,7 +66,7 @@ serial_communication_hubImpl::handle_modbus_read_holding_registers(int& target_d // (uint16_t)first_register_address, (uint16_t)num_registers_to_read); response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::READ_MULTIPLE_HOLDING_REGISTERS, - first_register_address, num_registers_to_read); + first_register_address, num_registers_to_read, config.max_packet_size); if (response.size() > 0) { break; } @@ -102,7 +102,7 @@ serial_communication_hubImpl::handle_modbus_read_input_registers(int& target_dev // (uint16_t)first_register_address, (uint16_t)num_registers_to_read); response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::READ_INPUT_REGISTERS, - first_register_address, num_registers_to_read); + first_register_address, num_registers_to_read, config.max_packet_size); if (response.size() > 0) { break; } @@ -140,7 +140,7 @@ types::serial_comm_hub_requests::StatusCodeEnum serial_communication_hubImpl::ha (uint16_t)data.size()); response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::WRITE_MULTIPLE_HOLDING_REGISTERS, - first_register_address, data.size(), true, data); + first_register_address, data.size(), config.max_packet_size, true, data); if (response.size() > 0) { break; } diff --git a/modules/SerialCommHub/main/serial_communication_hubImpl.hpp b/modules/SerialCommHub/main/serial_communication_hubImpl.hpp index 6f10b7e10..eacdc51ec 100644 --- a/modules/SerialCommHub/main/serial_communication_hubImpl.hpp +++ b/modules/SerialCommHub/main/serial_communication_hubImpl.hpp @@ -33,6 +33,7 @@ struct Conf { std::string rxtx_gpio_chip; int rxtx_gpio_line; bool rxtx_gpio_tx_high; + int max_packet_size; }; class serial_communication_hubImpl : public serial_communication_hubImplBase { diff --git a/modules/SerialCommHub/manifest.yaml b/modules/SerialCommHub/manifest.yaml index b3fd10986..4e605738c 100644 --- a/modules/SerialCommHub/manifest.yaml +++ b/modules/SerialCommHub/manifest.yaml @@ -36,6 +36,12 @@ provides: description: GPIO direction, false means low for TX, true means high for TX type: boolean default: false + max_packet_size: + description: Maximum size of a packet to read/write in bytes. Payload exceeding the size will be chunked. + type: integer + minimum: 0 + maximum: 65536 + default: 256 metadata: license: https://opensource.org/licenses/Apache-2.0 authors: diff --git a/modules/SerialCommHub/tiny_modbus_rtu.cpp b/modules/SerialCommHub/tiny_modbus_rtu.cpp index 3a5cfe2fd..e930a7b99 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.cpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.cpp @@ -21,7 +21,10 @@ #include #include +#include #include +#include +#include #include "crc16.hpp" @@ -174,6 +177,9 @@ static std::vector decode_reply(const uint8_t* buf, int len, uint8_t e case 0x0B: EVLOG_error << "Modbus exception: Gateway target device failed to respond"; break; + default: { + EVLOG_error << "Modbus exception: Unknown"; + } } return result; } @@ -263,8 +269,8 @@ bool TinyModbusRTU::open_device(const std::string& device, int _baud, bool _igno int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { struct timeval timeout; - timeout.tv_sec = 0; - timeout.tv_usec = MODBUS_RX_INITIAL_TIMEOUT_MS * 1000; // 500msec intial timeout until device responds + timeout.tv_sec = 6; + timeout.tv_usec = MODBUS_RX_INITIAL_TIMEOUT_MS * 1000; // 500msec initial timeout until device responds fd_set set; FD_ZERO(&set); FD_SET(fd, &set); @@ -272,8 +278,9 @@ int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { int bytes_read_total = 0; while (true) { int rv = select(fd + 1, &set, NULL, NULL, &timeout); - timeout.tv_usec = MODBUS_RX_WITHIN_MESSAGE_TIMEOUT_MS * - 1000; // reduce timeout after first chunk, no uneccesary waiting at the end of the message + // reduce timeout after first chunk, no unnecessary waiting at the end of the message + timeout.tv_sec = 0; + timeout.tv_usec = MODBUS_RX_WITHIN_MESSAGE_TIMEOUT_MS * 1000; if (rv == -1) { // error in select function call perror("txrx: select:"); break; @@ -296,13 +303,51 @@ int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { return bytes_read_total; } +std::vector TinyModbusRTU::txrx(uint8_t device_address, FunctionCode function, + uint16_t first_register_address, uint16_t register_quantity, + uint16_t max_packet_size, bool wait_for_reply, + std::vector request) { + // This only supports chunking of the read-requests. + std::vector out; + + const uint16_t register_chunk = (max_packet_size - MODBUS_MIN_REPLY_SIZE) / 2; + size_t written_elements = 0; + while (register_quantity) { + + const auto current_register_quantity = std::min(register_quantity, register_chunk); + std::vector current_request; + if (request.size() > written_elements + current_register_quantity) { + current_request = std::vector(request.begin() + written_elements, + request.begin() + written_elements + current_register_quantity); + written_elements += current_register_quantity; + } else { + current_request = std::vector(request.begin() + written_elements, request.end()); + written_elements = request.size(); + } + + const auto res = txrx_impl(device_address, function, first_register_address, current_register_quantity, + wait_for_reply, current_request); + + // We failed to read/write. + if (res.empty()) { + return res; + } + + out.insert(out.end(), res.begin(), res.end()); + first_register_address += current_register_quantity; + register_quantity -= current_register_quantity; + } + + return out; +} + /* This function transmits a modbus request and waits for the reply. Parameter request is optional and is only used for writing multiple registers. */ -std::vector TinyModbusRTU::txrx(uint8_t device_address, FunctionCode function, - uint16_t first_register_address, uint16_t register_quantity, - bool wait_for_reply, std::vector request) { +std::vector TinyModbusRTU::txrx_impl(uint8_t device_address, FunctionCode function, + uint16_t first_register_address, uint16_t register_quantity, + bool wait_for_reply, std::vector request) { { // size of request int req_len = (request.size() == 0 ? 0 : 2 * request.size() + 1) + MODBUS_BASE_PAYLOAD_SIZE; diff --git a/modules/SerialCommHub/tiny_modbus_rtu.hpp b/modules/SerialCommHub/tiny_modbus_rtu.hpp index 669c9cb04..c8926f48c 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.hpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.hpp @@ -58,8 +58,12 @@ class TinyModbusRTU { bool open_device(const std::string& device, int baud, bool ignore_echo, const Everest::GpioSettings& rxtx_gpio_settings, const Parity parity); + std::vector txrx_impl(uint8_t device_address, FunctionCode function, uint16_t first_register_address, + uint16_t register_quantity, bool wait_for_reply = true, + std::vector request = std::vector()); + std::vector txrx(uint8_t device_address, FunctionCode function, uint16_t first_register_address, - uint16_t register_quantity, bool wait_for_reply = true, + uint16_t register_quantity, uint16_t chunk_size, bool wait_for_reply = true, std::vector request = std::vector()); private: From febd5ec2fcc73670071dc73b9a95a6f7f19f1c23 Mon Sep 17 00:00:00 2001 From: Dima Dorezyuk Date: Mon, 20 Nov 2023 11:16:32 +0100 Subject: [PATCH 2/4] fix formatting Signed-off-by: Dima Dorezyuk --- modules/SerialCommHub/tiny_modbus_rtu.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/SerialCommHub/tiny_modbus_rtu.cpp b/modules/SerialCommHub/tiny_modbus_rtu.cpp index e930a7b99..3e638ccb1 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.cpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.cpp @@ -281,7 +281,7 @@ int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { // reduce timeout after first chunk, no unnecessary waiting at the end of the message timeout.tv_sec = 0; timeout.tv_usec = MODBUS_RX_WITHIN_MESSAGE_TIMEOUT_MS * 1000; - if (rv == -1) { // error in select function call + if (rv == -1) { // error in select function call perror("txrx: select:"); break; } else if (rv == 0) { // no more bytes to read within timeout, so transfer is complete From 0c2b0d62e5221edaccc1f92da5c9100d105b288f Mon Sep 17 00:00:00 2001 From: Dima Dorezyuk Date: Tue, 28 Nov 2023 14:48:51 +0100 Subject: [PATCH 3/4] make the timeout configurable Signed-off-by: Dima Dorezyuk --- .../main/serial_communication_hubImpl.cpp | 4 ++- .../main/serial_communication_hubImpl.hpp | 2 ++ modules/SerialCommHub/manifest.yaml | 8 +++++ modules/SerialCommHub/tiny_modbus_rtu.cpp | 30 ++++++++++++------- modules/SerialCommHub/tiny_modbus_rtu.hpp | 9 +++--- 5 files changed, 38 insertions(+), 15 deletions(-) diff --git a/modules/SerialCommHub/main/serial_communication_hubImpl.cpp b/modules/SerialCommHub/main/serial_communication_hubImpl.cpp index db2ed4f39..7c318ef26 100644 --- a/modules/SerialCommHub/main/serial_communication_hubImpl.cpp +++ b/modules/SerialCommHub/main/serial_communication_hubImpl.cpp @@ -31,6 +31,7 @@ static std::vector vector_to_int(const std::vector& response) { // Implementation void serial_communication_hubImpl::init() { + using namespace std::chrono; Everest::GpioSettings rxtx_gpio_settings; rxtx_gpio_settings.chip_name = config.rxtx_gpio_chip; @@ -38,7 +39,8 @@ void serial_communication_hubImpl::init() { rxtx_gpio_settings.inverted = config.rxtx_gpio_tx_high; if (!modbus.open_device(config.serial_port, config.baudrate, config.ignore_echo, rxtx_gpio_settings, - static_cast(config.parity))) { + static_cast(config.parity), milliseconds(config.initial_timeout_ms), + milliseconds(config.within_message_timeout_ms))) { EVLOG_AND_THROW(Everest::EverestConfigError(fmt::format("Cannot open serial port {}.", config.serial_port))); } } diff --git a/modules/SerialCommHub/main/serial_communication_hubImpl.hpp b/modules/SerialCommHub/main/serial_communication_hubImpl.hpp index eacdc51ec..32ac13ee5 100644 --- a/modules/SerialCommHub/main/serial_communication_hubImpl.hpp +++ b/modules/SerialCommHub/main/serial_communication_hubImpl.hpp @@ -34,6 +34,8 @@ struct Conf { int rxtx_gpio_line; bool rxtx_gpio_tx_high; int max_packet_size; + int initial_timeout_ms; + int within_message_timeout_ms; }; class serial_communication_hubImpl : public serial_communication_hubImplBase { diff --git a/modules/SerialCommHub/manifest.yaml b/modules/SerialCommHub/manifest.yaml index 4e605738c..4acca1b99 100644 --- a/modules/SerialCommHub/manifest.yaml +++ b/modules/SerialCommHub/manifest.yaml @@ -42,6 +42,14 @@ provides: minimum: 0 maximum: 65536 default: 256 + initial_timeout_ms: + description: Timeout in ms for the first packet. + type: integer + default: 500 + within_message_timeout_ms: + description: Timeout in ms for subsequent packets. + type: integer + default: 100 metadata: license: https://opensource.org/licenses/Apache-2.0 authors: diff --git a/modules/SerialCommHub/tiny_modbus_rtu.cpp b/modules/SerialCommHub/tiny_modbus_rtu.cpp index 3e638ccb1..3d14c43d7 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.cpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.cpp @@ -177,10 +177,9 @@ static std::vector decode_reply(const uint8_t* buf, int len, uint8_t e case 0x0B: EVLOG_error << "Modbus exception: Gateway target device failed to respond"; break; - default: { + default: EVLOG_error << "Modbus exception: Unknown"; } - } return result; } } @@ -191,8 +190,12 @@ TinyModbusRTU::~TinyModbusRTU() { } bool TinyModbusRTU::open_device(const std::string& device, int _baud, bool _ignore_echo, - const Everest::GpioSettings& rxtx_gpio_settings, const Parity parity) { + const Everest::GpioSettings& rxtx_gpio_settings, const Parity parity, + std::chrono::milliseconds _initial_timeout, + std::chrono::milliseconds _within_message_timeout) { + initial_timeout = _initial_timeout; + within_message_timeout = _within_message_timeout; ignore_echo = _ignore_echo; rxtx_gpio.open(rxtx_gpio_settings); @@ -268,9 +271,19 @@ bool TinyModbusRTU::open_device(const std::string& device, int _baud, bool _igno } int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { - struct timeval timeout; - timeout.tv_sec = 6; - timeout.tv_usec = MODBUS_RX_INITIAL_TIMEOUT_MS * 1000; // 500msec initial timeout until device responds + // Lambda to convert std::chrono to timeval. + auto to_timeval = [](const auto& time) { + using namespace std::chrono; + struct timeval timeout; + auto sec = duration_cast(time); + timeout.tv_sec = sec.count(); + timeout.tv_usec = duration_cast(time - sec).count(); + return timeout; + }; + + auto timeout = to_timeval(initial_timeout); + const auto within_message_timeval = to_timeval(within_message_timeout); + fd_set set; FD_ZERO(&set); FD_SET(fd, &set); @@ -278,9 +291,7 @@ int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { int bytes_read_total = 0; while (true) { int rv = select(fd + 1, &set, NULL, NULL, &timeout); - // reduce timeout after first chunk, no unnecessary waiting at the end of the message - timeout.tv_sec = 0; - timeout.tv_usec = MODBUS_RX_WITHIN_MESSAGE_TIMEOUT_MS * 1000; + timeout = within_message_timeval; if (rv == -1) { // error in select function call perror("txrx: select:"); break; @@ -313,7 +324,6 @@ std::vector TinyModbusRTU::txrx(uint8_t device_address, FunctionCode f const uint16_t register_chunk = (max_packet_size - MODBUS_MIN_REPLY_SIZE) / 2; size_t written_elements = 0; while (register_quantity) { - const auto current_register_quantity = std::min(register_quantity, register_chunk); std::vector current_request; if (request.size() > written_elements + current_register_quantity) { diff --git a/modules/SerialCommHub/tiny_modbus_rtu.hpp b/modules/SerialCommHub/tiny_modbus_rtu.hpp index c8926f48c..e3a221abd 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.hpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.hpp @@ -7,6 +7,7 @@ #ifndef TINY_MODBUS_RTU #define TINY_MODBUS_RTU +#include #include #include @@ -31,9 +32,6 @@ constexpr int MODBUS_MAX_REPLY_SIZE = 255 + 6; constexpr int MODBUS_MIN_REPLY_SIZE = 5; constexpr int MODBUS_BASE_PAYLOAD_SIZE = 8; -constexpr int MODBUS_RX_INITIAL_TIMEOUT_MS = 500; -constexpr int MODBUS_RX_WITHIN_MESSAGE_TIMEOUT_MS = 100; - enum class Parity : uint8_t { NONE = 0, ODD = 1, @@ -57,7 +55,8 @@ class TinyModbusRTU { ~TinyModbusRTU(); bool open_device(const std::string& device, int baud, bool ignore_echo, - const Everest::GpioSettings& rxtx_gpio_settings, const Parity parity); + const Everest::GpioSettings& rxtx_gpio_settings, const Parity parity, + std::chrono::milliseconds initial_timeout, std::chrono::milliseconds within_message_timeout); std::vector txrx_impl(uint8_t device_address, FunctionCode function, uint16_t first_register_address, uint16_t register_quantity, bool wait_for_reply = true, std::vector request = std::vector()); @@ -73,6 +72,8 @@ class TinyModbusRTU { int read_reply(uint8_t* rxbuf, int rxbuf_len); Everest::Gpio rxtx_gpio; + std::chrono::milliseconds initial_timeout; + std::chrono::milliseconds within_message_timeout; }; } // namespace tiny_modbus From 644e88da491be56e1034085cd1cf26f4228762aa Mon Sep 17 00:00:00 2001 From: Dima Dorezyuk Date: Wed, 6 Dec 2023 10:55:16 +0100 Subject: [PATCH 4/4] reviewer remarks Signed-off-by: Dima Dorezyuk --- .../main/serial_communication_hubImpl.cpp | 12 ++++++------ modules/SerialCommHub/manifest.yaml | 5 ++++- modules/SerialCommHub/tiny_modbus_rtu.cpp | 4 ++-- modules/SerialCommHub/tiny_modbus_rtu.hpp | 8 +++++--- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/modules/SerialCommHub/main/serial_communication_hubImpl.cpp b/modules/SerialCommHub/main/serial_communication_hubImpl.cpp index 7c318ef26..e3145c1ba 100644 --- a/modules/SerialCommHub/main/serial_communication_hubImpl.cpp +++ b/modules/SerialCommHub/main/serial_communication_hubImpl.cpp @@ -63,9 +63,9 @@ serial_communication_hubImpl::handle_modbus_read_holding_registers(int& target_d auto retry_counter = this->num_resends_on_error; while (retry_counter > 0) { - // EVLOG_info << fmt::format("Try {} Call modbus_client->read_holding_register(id {} addr {} len {})", - // (int)retry_counter, (uint8_t)target_device_id, - // (uint16_t)first_register_address, (uint16_t)num_registers_to_read); + EVLOG_debug << fmt::format("Try {} Call modbus_client->read_holding_register(id {} addr {} len {})", + (int)retry_counter, (uint8_t)target_device_id, (uint16_t)first_register_address, + (uint16_t)num_registers_to_read); response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::READ_MULTIPLE_HOLDING_REGISTERS, first_register_address, num_registers_to_read, config.max_packet_size); @@ -99,9 +99,9 @@ serial_communication_hubImpl::handle_modbus_read_input_registers(int& target_dev uint8_t retry_counter{this->num_resends_on_error}; while (retry_counter-- > 0) { - // EVLOG_info << fmt::format("Try {} Call modbus_client->read_input_register(id {} addr {} len {})", - // (int)retry_counter, (uint8_t)target_device_id, - // (uint16_t)first_register_address, (uint16_t)num_registers_to_read); + EVLOG_debug << fmt::format("Try {} Call modbus_client->read_input_register(id {} addr {} len {})", + (int)retry_counter, (uint8_t)target_device_id, (uint16_t)first_register_address, + (uint16_t)num_registers_to_read); response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::READ_INPUT_REGISTERS, first_register_address, num_registers_to_read, config.max_packet_size); diff --git a/modules/SerialCommHub/manifest.yaml b/modules/SerialCommHub/manifest.yaml index 4acca1b99..435020304 100644 --- a/modules/SerialCommHub/manifest.yaml +++ b/modules/SerialCommHub/manifest.yaml @@ -37,7 +37,10 @@ provides: type: boolean default: false max_packet_size: - description: Maximum size of a packet to read/write in bytes. Payload exceeding the size will be chunked. + description: >- + Maximum size of a packet to read/write in bytes. Payload exceeding the size will be chunked. + The APU size according to [wikipedia](https://en.wikipedia.org/wiki/Modbus) is 256 bytes, + which is used as default here. type: integer minimum: 0 maximum: 65536 diff --git a/modules/SerialCommHub/tiny_modbus_rtu.cpp b/modules/SerialCommHub/tiny_modbus_rtu.cpp index 3d14c43d7..f2904c0fc 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.cpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.cpp @@ -307,7 +307,7 @@ int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { int bytes_read = read(fd, rxbuf + bytes_read_total, rxbuf_len - bytes_read_total); if (bytes_read > 0) { bytes_read_total += bytes_read; - // EVLOG_info << "RECVD: " << hexdump(rxbuf, bytes_read_total); + EVLOG_debug << "RECVD: " << hexdump(rxbuf, bytes_read_total); } } } @@ -386,7 +386,7 @@ std::vector TinyModbusRTU::txrx_impl(uint8_t device_address, FunctionC // set checksum in the last 2 bytes append_checksum(req.get(), req_len); - // EVLOG_info << "SEND: " << hexdump(req.get(), req_len); + EVLOG_debug << "SEND: " << hexdump(req.get(), req_len); // clear input and output buffer tcflush(fd, TCIOFLUSH); diff --git a/modules/SerialCommHub/tiny_modbus_rtu.hpp b/modules/SerialCommHub/tiny_modbus_rtu.hpp index e3a221abd..bb4066a60 100644 --- a/modules/SerialCommHub/tiny_modbus_rtu.hpp +++ b/modules/SerialCommHub/tiny_modbus_rtu.hpp @@ -57,9 +57,6 @@ class TinyModbusRTU { bool open_device(const std::string& device, int baud, bool ignore_echo, const Everest::GpioSettings& rxtx_gpio_settings, const Parity parity, std::chrono::milliseconds initial_timeout, std::chrono::milliseconds within_message_timeout); - std::vector txrx_impl(uint8_t device_address, FunctionCode function, uint16_t first_register_address, - uint16_t register_quantity, bool wait_for_reply = true, - std::vector request = std::vector()); std::vector txrx(uint8_t device_address, FunctionCode function, uint16_t first_register_address, uint16_t register_quantity, uint16_t chunk_size, bool wait_for_reply = true, @@ -69,6 +66,11 @@ class TinyModbusRTU { // Serial interface int fd{0}; bool ignore_echo{false}; + + std::vector txrx_impl(uint8_t device_address, FunctionCode function, uint16_t first_register_address, + uint16_t register_quantity, bool wait_for_reply = true, + std::vector request = std::vector()); + int read_reply(uint8_t* rxbuf, int rxbuf_len); Everest::Gpio rxtx_gpio;