SerialCommHub: support longer transactions and chunking #441
Conversation
| timeout.tv_sec = 0; | ||
| timeout.tv_usec = MODBUS_RX_INITIAL_TIMEOUT_MS * 1000; // 500msec intial timeout until device responds | ||
| // 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?
| // (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); |
There was a problem hiding this comment.
Debug needs to be removed before merging
| @@ -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_info << "RECVD: " << hexdump(rxbuf, bytes_read_total); | |||
| EVLOG_debug << "RECVD: " << hexdump(rxbuf, bytes_read_total); | |||
| @@ -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_info << "SEND: " << hexdump(req.get(), req_len); | |||
| EVLOG_debug << "SEND: " << hexdump(req.get(), req_len); | |||
| // 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
| int read_reply(uint8_t* rxbuf, int rxbuf_len); | ||
|
|
||
| Everest::Gpio rxtx_gpio; | ||
| std::chrono::milliseconds initial_timeout; | ||
| std::chrono::milliseconds within_message_timeout; |
There was a problem hiding this comment.
Both could be const and initialized in the initializer list
There was a problem hiding this comment.
This would be a better design. But in the way the tiny_modbus implemented and used it would require quite a refactoring:
- there is no constructor defined for TinyModbusRTU
- the object
modbusis default-constructed in SerialComHub, and the device is opened later on.
So we will need to figure out when and with which parameters to call the TinyModbusRTU constructor
Do you think it makes sense to add this refactoring into this MR?
Signed-off-by: Dima Dorezyuk <[email protected]>
Signed-off-by: Dima Dorezyuk <[email protected]>
Signed-off-by: Dima Dorezyuk <[email protected]>
Signed-off-by: Dima Dorezyuk <[email protected]>
Pr adds support for maximum packet sizes to the SerialCommHub. The wikipedia says that the the packet size is between 253 and 256 bytes
This pr makes the packet size configurable. If the payload exceeds the packet size we will read/write it in chunks