refacto: new Client API

[creberust]

The new Client API enables the user to have 2 errors returned when calling a Modbus function.

1. std::io::Error: An error occured on the network side.
2. tokio_modbus::Exception: An error occured on the Modbus server.

Tasks

• Add Result type alias.
• Update the Client trait.
• Update the examples with new API.
  • tcp-server.rs
  • tls-server.rs
  • rtu-server.rs
  • rtu-server-address.rs
  • rtu-over-tcp-server.rs

[uklotzde]

Thanks for implementing the nested Result proposal. Looks good so far. It nicely separates technical from semantic/logic errors.

[creberust]

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 👍🏻 .

[uklotzde]

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.

[creberust]

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 !

[creberust]

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 ?

[uklotzde]

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.

[creberust]

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 👍🏻

[uklotzde]

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.

[creberust]

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 ?

[uklotzde]

Sorry for the delay. I hope to review your PR again soon. Please be patient 🙈

[uklotzde]

Feedback from other users of this library would be greatly appreciated. Those are the ones that are most affected by this substantial API refactoring.

[uklotzde]

creberust#1

[uklotzde]

Some suggestions for the wording in the changelog.