fix tcp retransmission by reading up to 10 frames #172
Conversation
Signed-off-by: chenxinghua <[email protected]>
|
when verify_response_header failed, try reading more frames |
| if retries >= MAX_RETRIES { | ||
| return Err(e); | ||
| } | ||
| retries += 1; |
There was a problem hiding this comment.
The error should at least be logged instead of silently ignoring it.
There was a problem hiding this comment.
| retries += 1; | |
| retries += 1; | |
| log::info!("Try to recover from header mismatch: expected/request = {:?}, actual/response = {:?}, retries = {:?}", req_hdr, res_adu.hdr, retries); |
log like this is acceptable?
There was a problem hiding this comment.
Please modify the code directly instead of replying to comments. Commits can be squashed later before merging.
src/service/tcp.rs
Outdated
| @@ -18,6 +18,7 @@ use crate::{ | |||
| }; | |||
|
|
|||
| const INITIAL_TRANSACTION_ID: TransactionId = 0; | |||
| const MAX_RETRIES: usize = 10; | |||
There was a problem hiding this comment.
Max retries of what? Why 10? If this is an arbitrary number then it should better be configurable instead of a hard-coded constant.
A comment with references why this retry algorithm is needed is also required. Either by documenting the constant or the corresponding configuration option.
There was a problem hiding this comment.
Let MAX_RETRIES be configurable is more suitable. Can you give me some advice? I have not found how to add configurable code in this crate.
The retry algorithm is inspired by libmodbus's error recovery mode, see https://libmodbus.org/reference/modbus_set_error_recovery/ for details.
When MODBUS_ERROR_RECOVERY_PROTOCOL is set, a sleep and flush sequence will be used to clean up the ongoing communication, this can occurs when the message length is invalid, the TID is wrong or the received function code is not the expected one. The response timeout delay will be used to sleep.
There was a problem hiding this comment.
Please add all information that is required reason about design decision to the code. No one will read this PR later after it has been merged.
There was a problem hiding this comment.
If this parameter should be configurable then make a proposal. Otherwise use a constant and comment (in the code) why.
Signed-off-by: chenxinghua <[email protected]>
Signed-off-by: chenxinghua <[email protected]>
| /// response frame can be discarded. This strategy is inspired by | ||
| /// libmodbus's error recovery: | ||
| /// https://libmodbus.org/reference/modbus_set_error_recovery/ | ||
| pub async fn connect_slave_recover(socket_addr: SocketAddr, slave: Slave, max_recover_retries: usize) -> Result<Context, Error> { |
There was a problem hiding this comment.
The number of factory functions for creating a client context gets confusing with all the possible combinations. I guess switching to a builder pattern would be more appropriate, although this is bigger tasks apart from this PR.
@flosse Your thoughts?
src/client/tcp.rs
Outdated
| @@ -26,6 +26,20 @@ pub async fn connect_slave(socket_addr: SocketAddr, slave: Slave) -> Result<Cont | |||
| Ok(context) | |||
| } | |||
|
|
|||
| /// Connect to a physical, broadcast, or custom Modbus device, | |||
There was a problem hiding this comment.
- Please try to use short sentences
- The initial line or sentence should be the summary, followed by one or more paragraphs with details.
But let's wait until we decided if you use an implicit constant for the time being before we are switching to a configurable parameter when introducing the builder pattern.
src/client/tcp.rs
Outdated
| /// reponse frame will arrived, with this strategy the mismatched | ||
| /// response frame can be discarded. This strategy is inspired by | ||
| /// libmodbus's error recovery: | ||
| /// https://libmodbus.org/reference/modbus_set_error_recovery/ |
There was a problem hiding this comment.
When following the link I don't see any connection between the recovery modes offered by libmodus and this parameter. They don't even mention any retires? This is more confusing than it helps.
There was a problem hiding this comment.
I quickly checked the rodbus implementation and didn't find anything similar.
Please elaborate why this retry is needed and desired. We should not introduce common parameters that only solve a particular use case.
There was a problem hiding this comment.
In libmodbus, discard ready frames when transcation id mismatch and error recover strategy enabled.
rc = ctx->backend->pre_check_confirmation(ctx, req, rsp, rsp_length);
if (rc == -1) {
if (ctx->error_recovery & MODBUS_ERROR_RECOVERY_PROTOCOL) {
_sleep_response_timeout(ctx);
modbus_flush(ctx);
}
return -1;
}I assume that, first several (1 or 2) frames are mismatch, the ok frame may in the remaining.
For situation in #111 discard one frame, the remaining can work.
|
I am concerned that this might only be a quick band aid for a more general issue, namely that response messages could arrive out of order. Is this even possible or allowed over a single connection? I am not deeply familiar with the Modbus specifications. |
Signed-off-by: chenxinghua <[email protected]>
|
any further suggestion? should I let MAX_RETRIES constant hardcoded for accepted. @uklotzde |
|
Please re-open when done. |
No description provided.