Skip to content

Commit

Permalink
SerialCommHub: several improvements (#731)
Browse files Browse the repository at this point in the history 
* SerialCommHub: make retry count on error configurable

At the moment, an automatic retry up to 2 times is done in case the
Modbus operation fails. With some devices or in same use-cases, this
is not desired, so let us make the retry count configurable via
manifest.

So mimic the current behavior and to retain backwards compatibility,
let's use a default value of 2.

While at, we slightly change the debug messages and unify the
loop counting code.

Signed-off-by: Michael Heimpold <[email protected]>

* SerialCommHub: fix fd usage on invalid values

open returns -1 on error, any other value has to be considered a
valid fd. So we have to initialize with -1 instead of zero
(which is stdin usually).
This must also be considered in the destructor to not close stdin
by accident.

Signed-off-by: Michael Heimpold <[email protected]>

* SerialCommHub: refactor error handling to prevent log flooding

When devices do not respond or some other problems occur,
then the the log output might be flooded with messages.

To have some more central control over the log output, let's
refactor the module:
- introduce various exceptions and use then in places where
  previously the log output was directly created
- introduce a common helper function which performs the
  Modbus function which also increases code sharing between
  different high-level callbacks
- emit only a warning in case the last trial of all configured
  retries fails finally (so in case the call can be completed
  successfully within the configure retries, only debug
  messages are created, but no usual user visible output)

Signed-off-by: Michael Heimpold <[email protected]>

* SerialCommHub: improve hexdump of received packet

Signed-off-by: Michael Heimpold <[email protected]>

* SerialCommHub: optimize memory pre-allocation for result

For registers, the byte_cnt should always be even.
But for coils/inputs, the byte_cnt could be odd, so let's
cover this by ensuring that we round upwards.

Signed-off-by: Michael Heimpold <[email protected]>

* SerialCommHub: check for even byte count in response

Since Modbus functions which operates on registers (which are defined
as 16-bit wide) are expected to return full registers, we can and
should check the received response for this.

Signed-off-by: Michael Heimpold <[email protected]>
Signed-off-by: Moritz Barsnick <[email protected]>

* SerialCommHub: silence warning about unused return value of write

Ignoring the return code of write results in a compiler warning
as theoretically the write could return without having written
the complete packet. So handle it accordingly.

In case of error, we can throw an exception.

Signed-off-by: Michael Heimpold <[email protected]>

* SerialCommHub: catch system error for failure to write

This is assumed to be fatal and non-recoverable, therefore logging is
protected against flooding. A code path for possible recovery is
implemented nevertheless.

This should be replaced by a proper error reporting and handling solution,
yet the error interfaces and handling for this are not in place yet.

Signed-off-by: Moritz Barsnick <[email protected]>

* SerialCommHub: fix reading beyond end of buffer

When converting a reply with an odd number of bytes of data ("read coils" or
"read discrete inputs"), the byte-to-word conversion was reading beyond the
end of the response message buffer.

This is fixed by checking for the number of remaining bytes, and copying
only what is left.

Signed-off-by: Moritz Barsnick <[email protected]>

---------

Signed-off-by: Michael Heimpold <[email protected]>
Signed-off-by: Moritz Barsnick <[email protected]>
Co-authored-by: Michael Heimpold <[email protected]>
Co-authored-by: Moritz Barsnick <[email protected]>
  • Loading branch information
@mhei @barsnick
mhei and barsnick authored Jul 8, 2024
1 parent 59269a7 commit bae72ef
Show file tree
Hide file tree
Showing 5 changed files with 243 additions and 177 deletions.
164 changes: 62 additions & 102 deletions modules/SerialCommHub/main/serial_communication_hubImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
#include "serial_communication_hubImpl.hpp"

#include <chrono>
#include <cstdint>
#include <date/date.h>
#include <date/tz.h>
#include <fmt/core.h>
#include <mutex>
#include <typeinfo>

namespace module {
Expand Down Expand Up @@ -38,6 +40,8 @@ void serial_communication_hubImpl::init() {
rxtx_gpio_settings.line_number = config.rxtx_gpio_line;
rxtx_gpio_settings.inverted = config.rxtx_gpio_tx_high;

system_error_logged = false;

if (!modbus.open_device(config.serial_port, config.baudrate, config.ignore_echo, rxtx_gpio_settings,
static_cast<tiny_modbus::Parity>(config.parity), config.rtscts,
milliseconds(config.initial_timeout_ms), milliseconds(config.within_message_timeout_ms))) {
Expand All @@ -48,146 +52,102 @@ void serial_communication_hubImpl::init() {
void serial_communication_hubImpl::ready() {
}

// Commands

types::serial_comm_hub_requests::Result
serial_communication_hubImpl::handle_modbus_read_holding_registers(int& target_device_id, int& first_register_address,
int& num_registers_to_read) {

serial_communication_hubImpl::perform_modbus_request(uint8_t device_address, tiny_modbus::FunctionCode function,
uint16_t first_register_address, uint16_t register_quantity,
bool wait_for_reply, std::vector<uint16_t> request) {
std::scoped_lock lock(serial_mutex);
types::serial_comm_hub_requests::Result result;
std::vector<uint16_t> response;

{
std::scoped_lock lock(serial_mutex);

auto retry_counter = this->num_resends_on_error;
while (retry_counter > 0) {

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);
if (response.size() > 0) {
break;
auto retry_counter = config.retries + 1;

while (retry_counter > 0) {
auto current_trial = config.retries + 1 - retry_counter + 1;

EVLOG_debug << fmt::format("Trial {}/{}: calling {}(id {} addr {}({:#06x}) len {})", current_trial,
config.retries + 1, tiny_modbus::FunctionCode_to_string_with_hex(function),
device_address, first_register_address, first_register_address, register_quantity);

try {
response = modbus.txrx(device_address, function, first_register_address, register_quantity,
config.max_packet_size, wait_for_reply, request);
} catch (const tiny_modbus::TinyModbusException& e) {
auto logmsg = fmt::format("Modbus call {} for device id {} addr {}({:#06x}) failed: {}",
tiny_modbus::FunctionCode_to_string_with_hex(function), device_address,
first_register_address, first_register_address, e.what());

if (retry_counter != 1)
EVLOG_debug << logmsg;
else
EVLOG_warning << logmsg;
} catch (const std::logic_error& e) {
EVLOG_warning << "Logic error in Modbus implementation: " << e.what();
} catch (const std::system_error& e) {
// FIXME: report this to the infrastructure, as soon as an error interface for this is available
// Log this only once, as we are convinced this will not go away
if (not system_error_logged) {
EVLOG_error << "System error in accessing Modbus: [" << e.code() << "] " << e.what();
system_error_logged = true;
}
retry_counter--;
}

if (response.size() > 0)
break;

retry_counter--;
}

EVLOG_debug << fmt::format("Process response (size {})", response.size());
// process response
if (response.size() > 0) {
EVLOG_debug << fmt::format("Process response (size {})", response.size());
result.status_code = types::serial_comm_hub_requests::StatusCodeEnum::Success;
result.value = vector_to_int(response);
system_error_logged = false; // reset after success
} else {
result.status_code = types::serial_comm_hub_requests::StatusCodeEnum::Error;
}
return result;
}

types::serial_comm_hub_requests::Result
serial_communication_hubImpl::handle_modbus_read_input_registers(int& target_device_id, int& first_register_address,
int& num_registers_to_read) {
types::serial_comm_hub_requests::Result result;
std::vector<uint16_t> response;

{
std::scoped_lock lock(serial_mutex);
// Commands

uint8_t retry_counter{this->num_resends_on_error};
while (retry_counter-- > 0) {
types::serial_comm_hub_requests::Result
serial_communication_hubImpl::handle_modbus_read_holding_registers(int& target_device_id, int& first_register_address,
int& 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);
return perform_modbus_request(target_device_id, tiny_modbus::FunctionCode::READ_MULTIPLE_HOLDING_REGISTERS,
first_register_address, 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);
if (response.size() > 0) {
break;
}
}
}
types::serial_comm_hub_requests::Result
serial_communication_hubImpl::handle_modbus_read_input_registers(int& target_device_id, int& first_register_address,
int& num_registers_to_read) {

EVLOG_debug << fmt::format("Process response (size {})", response.size());
// process response
if (response.size() > 0) {
result.status_code = types::serial_comm_hub_requests::StatusCodeEnum::Success;
result.value = vector_to_int(response);
} else {
result.status_code = types::serial_comm_hub_requests::StatusCodeEnum::Error;
}
return result;
return perform_modbus_request(target_device_id, tiny_modbus::FunctionCode::READ_INPUT_REGISTERS,
first_register_address, num_registers_to_read);
}

types::serial_comm_hub_requests::StatusCodeEnum serial_communication_hubImpl::handle_modbus_write_multiple_registers(
int& target_device_id, int& first_register_address, types::serial_comm_hub_requests::VectorUint16& data_raw) {

types::serial_comm_hub_requests::Result result;
std::vector<uint16_t> response;

std::vector<uint16_t> data;
append_array<uint16_t, int>(data, data_raw.data);

{
std::scoped_lock lock(serial_mutex);

uint8_t retry_counter{this->num_resends_on_error};
while (retry_counter-- > 0) {

EVLOG_debug << fmt::format("Try {} Call modbus_client->write_multiple_registers(id {} addr {} len {})",
(int)retry_counter, (uint8_t)target_device_id, (uint16_t)first_register_address,
(uint16_t)data.size());

response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::WRITE_MULTIPLE_HOLDING_REGISTERS,
first_register_address, data.size(), config.max_packet_size, true, data);
if (response.size() > 0) {
break;
}
}
}
result = perform_modbus_request(target_device_id, tiny_modbus::FunctionCode::WRITE_MULTIPLE_HOLDING_REGISTERS,
first_register_address, data.size(), true, data);

EVLOG_debug << fmt::format("Done writing (size {})", response.size());
// process response
if (response.size() > 0) {
return types::serial_comm_hub_requests::StatusCodeEnum::Success;
} else {
return types::serial_comm_hub_requests::StatusCodeEnum::Error;
}
return result.status_code;
}

types::serial_comm_hub_requests::StatusCodeEnum
serial_communication_hubImpl::handle_modbus_write_single_register(int& target_device_id, int& register_address,
int& data) {
types::serial_comm_hub_requests::Result result;
std::vector<uint16_t> response;

{
std::scoped_lock lock(serial_mutex);

uint8_t retry_counter{this->num_resends_on_error};
while (retry_counter-- > 0) {
result = perform_modbus_request(target_device_id, tiny_modbus::FunctionCode::WRITE_SINGLE_HOLDING_REGISTER,
register_address, 1, true, {static_cast<uint16_t>(data)});

EVLOG_debug << fmt::format("Try {} Call modbus_client->write_single_register(id {} addr {} data 0x{:04x})",
(int)retry_counter, (uint8_t)target_device_id, (uint16_t)register_address,
(uint16_t)data);

response = modbus.txrx(target_device_id, tiny_modbus::FunctionCode::WRITE_SINGLE_HOLDING_REGISTER,
register_address, 1, config.max_packet_size, true, {static_cast<uint16_t>(data)});
if (response.size() > 0) {
break;
}
}
}
EVLOG_debug << fmt::format("Done writing (size {})", response.size());
// process response
if (response.size() > 0) {
return types::serial_comm_hub_requests::StatusCodeEnum::Success;
} else {
return types::serial_comm_hub_requests::StatusCodeEnum::Error;
}
return result.status_code;
}

void serial_communication_hubImpl::handle_nonstd_write(int& target_device_id, int& first_register_address,
Expand Down
9 changes: 8 additions & 1 deletion modules/SerialCommHub/main/serial_communication_hubImpl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// insert your custom include headers here
#include "tiny_modbus_rtu.hpp"
#include <chrono>
#include <cstdint>
#include <termios.h>
#include <utils/thread.hpp>
#include <vector>
Expand All @@ -36,6 +37,7 @@ struct Conf {
int max_packet_size;
int initial_timeout_ms;
int within_message_timeout_ms;
int retries;
};

class serial_communication_hubImpl : public serial_communication_hubImplBase {
Expand Down Expand Up @@ -80,10 +82,15 @@ class serial_communication_hubImpl : public serial_communication_hubImplBase {

// ev@3370e4dd-95f4-47a9-aaec-ea76f34a66c9:v1
// insert your private definitions here
static constexpr uint8_t num_resends_on_error{3};
types::serial_comm_hub_requests::Result
perform_modbus_request(uint8_t device_address, tiny_modbus::FunctionCode function, uint16_t first_register_address,
uint16_t register_quantity, bool wait_for_reply = true,
std::vector<uint16_t> request = std::vector<uint16_t>());

tiny_modbus::TinyModbusRTU modbus;

std::mutex serial_mutex;
bool system_error_logged{false};
// ev@3370e4dd-95f4-47a9-aaec-ea76f34a66c9:v1
};

Expand Down
6 changes: 6 additions & 0 deletions modules/SerialCommHub/manifest.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ provides:
description: Timeout in ms for subsequent packets.
type: integer
default: 100
retries:
description: Count of retries in case of error in Modbus query.
type: integer
minimum: 0
maximum: 10
default: 2
metadata:
license: https://opensource.org/licenses/Apache-2.0
authors:
Expand Down
Loading

0 comments on commit bae72ef

Please sign in to comment.