Open uklotzde opened 1 year ago
Hello, I'm trying to use tokio-modbus
to create a Modbus Server implementation, and I've also hit this wall.
I took a look at the code and the issues/PR this morning.
I think #226 is a good start and I'll elaborate on that further.
But I think that the downcast is a short fix, and will only postpone the problem to more complex cases. And I also don't like to show more private types to the public API.
The public API should stay simple with Request
, Response
and Error
and nothing else.
Response
(breaking change)The first idea is to add a variant to Response
:
pub enum Reponse {
// [other variants]...
Exception(FunctionCode, Exception),
}
Technically, an exception is still a "valid" response from the Modbus Server.
Result<Response, ExceptionResponse>
in ResponsePDU
.Response
in the Service
trait.Response
and return a correct Error
.I think that while this solution is simple, it could lead to misinterpretation. This Response
variant should be well documented and clearly state that its an Error response.
For me, this solution highlights another issue.
The fact that all Error
s in this crate API are std::io::Error
.
std::io::Error
to an internal Error
type with more granularity (breaking change)This is a big change, but imo, it should ease the use, simplify the API and provide better error handling without modifying the Response
part.
The Error
type should allow the user to know what kind of error happened:
io::Error
The Error
type should also define if an error is recoverable (i.e: modbus exception), or not (i.e: server disconnected or others I/O errors).
std::io::Error
into something like crate::error::Error
.I'm taking the idea of @emcell here.
Because I think that's the way to go, I've only made a few small changes.
pub enum Error {
/// This is a error returned by the Modbus Server and relative to Modbus.
Exception(ModbusException),
/// This is an error in the Modbus Client side (can or can't be related to Modbus).
Io(std::io::Error),
}
If we take read_input_registers<'a>
, I think it'll look to something like:
let result = modbus.read_input_registers(0x1000, 6).await;
match result {
Ok(rsp) => todo!(),
Err(Error::Exception(exception) => log::error!(format!("failed to read input reg: {}", exception)), // drop error and continue execution
Err(e) => return Err(e), // the error is more important, let the caller handle the failure
}
I think this solution is better suited in order to fix the issue and provide a better API.
(I've started a POC to test if it's possible to update the Error
type without too much changes: #231 )
What do you think ? @uklotzde
I think we can find a good way to solve this and make a better API for version 0.10. (if you are open for breaking changes between 0.x).
I had another ideas that popped in my mind will trying to implement the server. I'm putting everything down to bring more possibilities.
Response
type with Valid
and Exception
vairant (breaking change)The goal is to replace the Response
type in order to be able to identify the type of the reponse (ie. valid or exception).
pub enum Response {
/// The Reponse from the modbus server is a success.
// NOTE: the name is open to discussion.
Valid(ModbusReponse),
/// The Response from the modbus server is an exception.
Exception(ModbusExceptionResponse),
}
ModbusReponse
is the actual Response
type.
ModbusExceptionResponse
is the actual ExceptionResponse
.
I feel this is a natural way of handling things in the server side:
fn call(req: Self::Request) -> Self::Future {
match req {
ReadCoils(addr, quantity) => future::ready(self.read_coils(addr, quantity).map(ModbusResponse::ReadCoils).map_err(|e| ExceptionResponse::new(req.function_code(), e)).into()),
others => future::ready(Ok(Response::Exception(ExceptionResponse::new(others.function_code(), Exception::IllegalFunction))),
}
}
fn read_coils(addr: Address, quantity: u16) -> Result<Vec<u16>, Exception> {
if /* addr not in range */ {
return Err(Exception::IllegalDataAddr);
}
// read coils
Ok(coils)
}
Result<Response, std::io::Error>
Request
has been properly handled when getting a Response
Response
typeResult<Response, Exception>
can also work in the same way.I think it's more clear to do this, and force the user to always handle the Response
type in a more elegant way.
tokio_modbus::Result
new type (breaking change)The goal is to have a specialization of a new Result
type in the tokio-modbus
crate like std::io
.
pub enum Result {
/// The result of the operation from the modbus server is a success.
Response(ModbusReponse),
/// The result of the operation from the modbus server is an exception.
Exception(ModbusExceptionResponse),
/// The result of the operation contains fatal failures.
Error(std::io::Error),
}
ModbusReponse
is the actual Response
type.
ModbusExceptionResponse
is the actual ExceptionResponse
.
Error
variant field with a genericResult
is typically a 2 variant enum (Ok, Err)This can be achieved with other solutions with a Result<Response, Error>
, I don't know if it's a good path.
Separating orthogonal classes of errors from different layers in the (network) stack is usually done by nesting results:
Result<Result<Response, ExceptionResponse>, Error>
The outer Result
contains the technical error.
I agree that nesting the error feels more natural.
That will requires changes in different traits
in order to be doable.
pub trait Client: SlaveContext + Send + Debug {
async fn call(&mut self, request: Request<'_>) -> Result<Result<Response, ExceptionResponse>, Error>;
}
pub trait Reader: Client {
async fn read_coils(&mut self, _: Address, _: Quantity) -> Result<Result<Vec<Coil>, Exception>, Error>;
}
pub trait Writer: Client {
async fn write_single_coil(&mut self, _: Address, _: Coil) -> Result<Result<(), Exception>, Error>;
}
I saw in the implementation (let's say read_discrete_inputs
) that you check if the Response
is the good type, isn't this case unreachable
? Or it can be seen as a wrong modbus server implementation ?
Because if we have the Result<Vec<Coil>, Exception>
, the implementation of read_discrete_inputs
would look like:
async fn read_discrete_inputs<'a>(&'a mut self, addr: Address, cnt: Quantity) -> Result<Result<Vec<Coil>, Exception>, Error> {
let rsp = self.client.call(Request::ReadDiscreteInputs(addr, cnt)).await?;
match rsp {
Ok(Response::ReadDiscreteInputs(mut coils)) => {
debug_assert!(coils.len() >= cnt.into());
coils.truncate(cnt.into());
Ok(Ok(coils))
},
Err(ExceptionResponse(_, exception)) => Ok(Err(exception)),
Ok(_) => unreachable!(), // fatal error, modbus-server is not returning valid response
}
}
I don't know if it's a better idea to abort if the response is invalid, or return an Error, but the Exception should come from the server side, not the client. But since we follow the spec, this use case "should" never arise.
Thanks to your server trait, we don't need to change anything because From<Result<Response, ExceptionResponse>>
is already implemented for OptionalResponsePdu
.
How to handle the call
function in src/service/{tcp/rtu}.rs
in order to return the nested errors ?
What do you think ?
See also: #163