Closed creberust closed 6 months ago
Hi,
Thanks for the feedback !
I need to also update the examples with the new client API to correctly assert
on error.
// Now we try to read with an invalid register address.
// This should return a Modbus exception response with the code
// IllegalDataAddress.
println!("CLIENT: Reading nonexisting holding register address... (should return IllegalDataAddress)");
let response = ctx.read_holding_registers(0x100, 1).await;
println!("CLIENT: The result is '{response:?}'");
assert!(response.is_err());
// TODO: How can Modbus client identify Modbus exception responses? E.g. here we expect IllegalDataAddress
// Question here: https://github.com/slowtec/tokio-modbus/issues/169
Like this part with an assert
on the Exception
being returned.
I'll add the CHANGELOG
entry when I'm ready with the PR 👍🏻 .
I need to also update the examples with the new client API to correctly
assert
on error.
Updating the examples will hopefully also reveal if this approach is usable in practice.
I tried it with our modbus
client wrapper around tokio_modbus
that does automatic re-connection on io::Error
, and it works great ! 🎉 I can match on io::Error
and spawn a task to reconnect, or let pass the modbus Response
with the data or an exception.
I need to try with a more complex use case with multiple functions called asynchronously and in parallel. But things are looking great already !
I have just one small question regarding the use of unreachable
in the client side.
Can you confirm that the paths with unreachable!
are indeed not supposed to happen ? And if it's reached, it's a bug in the client codec ?
I have just one small question regarding the use of
unreachable
in the client side.Can you confirm that the paths with
unreachable!
are indeed not supposed to happen ? And if it's reached, it's a bug in the client codec ?
Paths marked as unreachable!()
must never executed. Otherwise it is a bug. It must not be used for handling unexpected data that might occur, even if the spec disallows it.
Sometimes you simply cannot proof statically that certain paths are never executed. In this case unreachable!()
is used for documentation purposes.
Agreed,
For me, going down the unreachable
path in this case:
self.client
.call(Request::ReadCoils(addr, cnt))
.await
.map(|modbus_rsp| {
modbus_rsp.map(|rsp| match rsp {
Response::ReadCoils(mut coils) => {
debug_assert!(coils.len() >= cnt.into());
coils.truncate(cnt.into());
coils
}
others => {
unreachable!(
"unexpected response code: {} (request code: {})",
others.function_code(),
FunctionCode::ReadCoils
)
}
})
})
Is a bug in the tokio_modbus
lib and should never happen.
Are we ok with that ? If so, I think the PR is ready 👍🏻
For me, going down the
unreachable
path in this case:
Could you add a comment that points the reader where this is handled? Because at first sight it looks like the code would panic on invalid inputs.
Moved verify_response_header
function in the parent module to use the same function in rtu
and tcp
impl.
Added dev doc comments to explain why the unreachable!
macro is used here.
Improved error message in unreachable!
to ease the bug report handling.
Is it ok to have a link directly to the github issue page in the error message ?
Sorry for the delay. I hope to review your PR again soon. Please be patient :see_no_evil:
Feedback from other users of this library would be greatly appreciated. Those are the ones that are most affected by this substantial API refactoring.
The new
Client
API enables the user to have 2 errors returned when calling a Modbus function.std::io::Error
: An error occured on the network side.tokio_modbus::Exception
: An error occured on the Modbus server.Tasks
Result
type alias.Client
trait.tcp-server.rs
tls-server.rs
rtu-server.rs
rtu-server-address.rs
rtu-over-tcp-server.rs