TCP Server: How to return Modbus exception codes?

[uklotzde]

See also: #163

[benjamin-nw]

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.

1. Modify 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.

Pros

• Removes the burden of having a Result<Response, ExceptionResponse> in ResponsePDU.
• Relax the trait requirements for Response in the Service trait.

Cons

• Need to refacto a lot of Client code to handle the new Response and return a correct Error.

Conclusion

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 Errors in this crate API are std::io::Error.

2. Changing 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:

• modbus exception
• io::Error
• more if needed...

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).

Pros

• Allow the user to match on the specific error types
• Reduce misuse of the library error type

Cons

• This is a breaking change, we need to update the std::io::Error into something like crate::error::Error.
• Need to review some parts in Client and Server to update the logic accordingly.

Example

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
}

Conclusion

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 )

Feedback welcome

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).

[benjamin-nw]

I had another ideas that popped in my mind will trying to implement the server. I'm putting everything down to bring more possibilities.

3. New 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).

Example

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)
}

Pros

• We can keep the structure of Result<Response, std::io::Error>
• The user always needs to check if a Request has been properly handled when getting a Response

Cons

• We need to change a lot of the code in order to update the Response type
• A Result<Response, Exception> can also work in the same way.

Conclusion

I think it's more clear to do this, and force the user to always handle the Response type in a more elegant way.

4. Add new 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.

Example

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.

Pros

• One type to rule them all
• We might change the Error variant field with a generic

Cons

• This will require a lot of change in the lib
• Might be unclear to user because a Result is typically a 2 variant enum (Ok, Err)

Conclusion

This can be achieved with other solutions with a Result<Response, Error>, I don't know if it's a good path.

[uklotzde]

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.

[benjamin-nw]

I agree that nesting the error feels more natural.

That will requires changes in different traits in order to be doable.

Client

pub trait Client: SlaveContext + Send + Debug {
     async fn call(&mut self, request: Request<'_>) -> Result<Result<Response, ExceptionResponse>, Error>;
}

Reader/Writer

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.

Server

Thanks to your server trait, we don't need to change anything because From<Result<Response, ExceptionResponse>> is already implemented for OptionalResponsePdu.

Pending questions

How to handle the call function in src/service/{tcp/rtu}.rs in order to return the nested errors ?

Feedback

What do you think ?