SerialCommHub: support longer transactions and chunking #441
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,10 @@ | |
| #include <sys/select.h> | ||
| #include <sys/time.h> | ||
|
|
||
| #include <algorithm> | ||
| #include <fmt/core.h> | ||
| #include <iterator> | ||
| #include <ostream> | ||
|
|
||
| #include "crc16.hpp" | ||
|
|
||
|
|
@@ -174,6 +177,8 @@ static std::vector<uint16_t> 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; | ||
| } | ||
|
|
@@ -185,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, | ||
| 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); | ||
|
|
@@ -262,19 +271,28 @@ bool TinyModbusRTU::open_device(const std::string& device, int _baud, bool _igno | |
| } | ||
|
|
||
| int TinyModbusRTU::read_reply(uint8_t* rxbuf, int rxbuf_len) { | ||
| // Lambda to convert std::chrono to timeval. | ||
| auto to_timeval = [](const auto& time) { | ||
|
There was a problem hiding this comment. This should be a stand alone function in the anonymous namespace in this file. It does not capture anything from its environment. There was a problem hiding this comment. I disagree - We can only use auto as a function argument in c++20. The the extracted function therefore would either need to pin down the exact chrono type (milliseconds) making the code slightly less generic or be a template function making the code less readable There was a problem hiding this comment. @dorezyuk do you mean, that function template<T>
timeval to_timeval(const T& time) {
// existing code without change
}would make it less readable? |
||
| using namespace std::chrono; | ||
| struct timeval timeout; | ||
| auto sec = duration_cast<seconds>(time); | ||
| timeout.tv_sec = sec.count(); | ||
| timeout.tv_usec = duration_cast<microseconds>(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); | ||
|
|
||
| int bytes_read_total = 0; | ||
| while (true) { | ||
| int rv = select(fd + 1, &set, NULL, NULL, &timeout); | ||
| timeout = within_message_timeval; | ||
| 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 | ||
|
|
@@ -289,20 +307,57 @@ 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_debug << "RECVD: " << hexdump(rxbuf, bytes_read_total); | ||
|
|
||
| } | ||
| } | ||
| } | ||
| return bytes_read_total; | ||
| } | ||
|
|
||
| std::vector<uint16_t> 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<uint16_t> request) { | ||
| // This only supports chunking of the read-requests. | ||
| std::vector<uint16_t> out; | ||
|
|
||
| const uint16_t register_chunk = (max_packet_size - MODBUS_MIN_REPLY_SIZE) / 2; | ||
|
There was a problem hiding this comment. If register_chunk is zero because max_packet_size is < MODBUS_MIN_REPLY_SIZE this loop never ends. Add input validation here and increase the min setting for max_packet_size in manifest to > MIN_REPLY_SIZE |
||
| size_t written_elements = 0; | ||
| while (register_quantity) { | ||
| const auto current_register_quantity = std::min(register_quantity, register_chunk); | ||
| std::vector<uint16_t> current_request; | ||
| if (request.size() > written_elements + current_register_quantity) { | ||
| current_request = std::vector<uint16_t>(request.begin() + written_elements, | ||
| request.begin() + written_elements + current_register_quantity); | ||
| written_elements += current_register_quantity; | ||
| } else { | ||
| current_request = std::vector<uint16_t>(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<uint16_t> TinyModbusRTU::txrx_impl(uint8_t device_address, FunctionCode function, | ||
| uint16_t first_register_address, uint16_t register_quantity, | ||
| bool wait_for_reply, std::vector<uint16_t> request) { | ||
| { | ||
| // size of request | ||
| int req_len = (request.size() == 0 ? 0 : 2 * request.size() + 1) + MODBUS_BASE_PAYLOAD_SIZE; | ||
|
|
@@ -331,7 +386,7 @@ std::vector<uint16_t> TinyModbusRTU::txrx(uint8_t device_address, FunctionCode f | |
| // set checksum in the last 2 bytes | ||
| append_checksum(req.get(), req_len); | ||
|
|
||
| EVLOG_debug << "SEND: " << hexdump(req.get(), req_len); | ||
|
|
||
|
|
||
| // clear input and output buffer | ||
| tcflush(fd, TCIOFLUSH); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug needs to be removed before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #535