-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More possibilities with Modbus-Exceptions #226
Conversation
@@ -271,6 +392,30 @@ pub struct ExceptionResponse { | |||
pub exception: Exception, | |||
} | |||
|
|||
/// Convenience trait for downcasting `std::io::Error` to `ExceptionResponse` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to introduce a public trait? What is the use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an example to make this more clear.
Just an easy way to downcast the std::io::Error to the ExceptionResponse if it is one.
I'm also not really with this solution. The better way would be to introduce an Error type for the clients. Could look like this
pub enum Error {
ModbusException(ModbusException),
IoError(std::io::Error)
}
This this change it's clear for the api users that a ModbusException exists and can be extracted. On the other hand that could break a lot of builds.
What do you think?
Thanks for contributing! IMHO this PR contains too many unrelated changes. The |
any chance this is getting merged soon? |
Please give this PR a concise title that later becomes the Git commit message. We also need a changelog entry that summarizes the changes and consequences for users of this crate. Ideally a single sentence for every notable API change. As short as possible. Everyone is invited to do a review. We are managing this crate in our spare time without any monetary compensation. Please understand that paid projects might be prioritized. Thank you. |
Should be rebased and updated once #246 has been merged. |
I will close this PR after seeing no progress lately and because merge conflicts are pending. Please open a new PR whenever you have something ready. Thank you. |
This got a little bigger than I initially expected. I hope you're ok with my changes.
What I've done so far
frame
module publicFunctionCode
to an enum and replaced it in a lot of places in codec module. I've done this because I needed the function code in the service handlers to send the CorrectExceptionResponse
object.GetFunctionCode
trait and implemented it onRequest
andResponse
. This is also necessary to generate the correctExceptionResponse
in the handlers.ExtractExceptionResponse
trait. This is for an easy way to downcast thestd::io::Error
in the clients toExceptionResponse
futures::ready
I started this because I didn't find ways to specify Modbus-Exception and didn't find a way to extract those in the clients.
I Think this PR should fix #169 and #98.
What could be better here?
I think It would be better to be able to Just return a
Result<Response, Exception>
from the Service and figure the correct function_code out after the service handler and automatically convert theException
intoExceptionResult
.But I didn't want to make an even bigger change without speaking to to you in the first place.