slowtec / tokio-modbus

A tokio-based modbus library
Apache License 2.0
405 stars 120 forks source link

Fix panic on mismatching request/response function codes #254

Closed uklotzde closed 6 months ago

uklotzde commented 6 months ago

Fixes #252.

uklotzde commented 6 months ago

The seconds commit introduces another error variant that replaces std::io::Error in case of mismatching headers between request and response. All error variants include the actual response result.

flosse commented 6 months ago

I'm sorry, where exactly is the fix?

uklotzde commented 6 months ago

I am still unsure if it would be more suitable to replace the outer std::io::Error with a new error type or the inner type as done here. Where do protocol errors belong?

uklotzde commented 6 months ago

The alternative approach of replacing the outer std::io::error with a custom error type and preserving Exception as the inner error type turned out to be much more elegant.

The different sources of errors are now separated more consistently, i.e. client/server communication vs. server-side processing.

flosse commented 6 months ago

I am still unsure if it would be more suitable to replace the outer std::io::Error with a new error type or the inner type as done here. Where do protocol errors belong?

In general I'd look on the layers.

Application -> Modbus -> Transport

So my error design would be

enum MyAppError {
  Modbus(ModbusError),
  // other app specific
}
enum ModbusError {
  Transport(TransportError).
  // other modbus specific
}
enum TransportError {
  // ...
}

So the top level error in this library would be a very specific error where io::Error is just one variant.

flosse commented 6 months ago

The alternative approach of replacing the outer std::io::error with a custom error type and preserving Exception as the inner error type turned out to be much more elegant.

:joy: it looks like you had the same idea :smile:

:100:

uklotzde commented 6 months ago

Keeping the Exception separate as a not-really-an-error without wrapping it justifies the nested Result pattern.