feat: add `Exception` type to public api

slowtec / tokio-modbus

A tokio-based modbus library

Apache License 2.0

405 stars 120 forks source link

feat: add `Exception` type to public api #246

Closed creberust closed 8 months ago

creberust commented 8 months ago

Hi,

I've added the Exception type to the public API in order to be able to use it in the Service implementation first.

I've also modified the examples to show how to return an Exception from the Service, it requires to change the Service trait. Thus, if it's too much for this PR, I can split it up and keep this PR very short.

This is a duplicate of #218 but the author seems unavailable.

uklotzde commented 8 months ago

Thanks. All changes look reasonable. Does this PR resolve #169?

Would you mind adding a change log entry? We need to bump the version to v0.11.0 even though client-only code is unaffected. Not a big deal.

What's the reason for using two different GitHub accounts? I was confused and wondering why GitHub requires approval to run the CI builds again.

creberust commented 8 months ago

Hi,

this resolves only the Server side Exception handling of the #169 . But I think it's the easiest minimal change to keep the current code working without modifying too much, and yet adding simplicity to Exception handling in the Server side.

In order to resolve #169, we need to also update the Client trait to better handle return to the user, the correct Exception. I think this will be harder because we need to modify a lot of trait, and some of the logic with the Client handling of errors.

I'll add a changelog entry 👍🏻 .

Oh sorry for the confusion, this one is my personal account and the other one is my work account. I'll only be using this one in the future, sorry for the unexpected change.

creberust commented 8 months ago

I can also add tests to be sure that the FunctionCode for each Response in case of an Exception is the correct one.

creberust commented 8 months ago

Hello 👋🏻

I've added a change log entry for each of the changes.

I've also added integration tests for tcp-server, rtu-server and rtu-over-tcp-server in order to check that Exception's are correctly returned.

But I've run into 2 issues:

Integration test for rtu-server can't be run because we need an available SerialPort. We need to mock one or change the way to test.
I've found a hang when using Request::Custom with rtu-over-tcp-server. I've commented the code where the mistakes happens. It only hangs here with feature rtu-over-tcp-server.

We'll need to update the tests when the client will handle the Exception.

creberust commented 8 months ago

Oh I forgot to add pre-commit 😅 will do it now and commit the fix for the errors.