Allow server to handle slave address

[obayemi]

Hi I've made this small change on the server part of the library to allow it to be more useful

I did need to handle the slave address but it was neither passed to the service nor possible for the service to not respond to a valid modbus request on the bus, wich is a problem on a bus with a lot of devices with their own addresses.

I did not see any PR / issue so I made the fix in a fork that I am currently rolling in production.

It is a bit rough for now but I am interested to know if you would be open to merging it ?

Basically all my changes are to make the Request and Response types in the services types that need to implement respectively From<RequestAdu> and TryInto<ResponsePdu>. The change is 100% backward compatible since the original Request and Respone object implements the required traits, and allow to use any different request / response custom implementation in the service required in the service development that into/from can work with.

Two things I'm not really sure about are:

• having to make RequestAdu and ResponsePdu public for the interface to work though, but I'm not sure that there actually is a much better way.
• TryInto<ResponsePdu> vs Option<Into<ResponsePdu>> for the response type, because I think the try_into() is pretty obvious in rust that it is just doing a regular into(), but while also taking in account the fact that it might fail, like when no response is supposed to be sent because the message was not destined for that device on the bus, but Option<Into<ResponsePdu>> makes it more obvious / limiting that is is actually only designed to allow not sending a response as a success case.

I've not really wrote tests nor yet, but I'm really interested in your feedback.

[obayemi]

actually, I realise now that I have missed #97
its intended to do the same thing, but without changing Service::call signature, and also allowing to not reply to a request when not relevant.

[obayemi]

Added an example in examples/rtu-server-address.rs

[obayemi]

please @uklotzde @flosse can you take a look at it ?

[Kais-ben-youssef]

+1 Looking forward to this

[uklotzde]

Thanks for your contribution!

I am not familiar with the experimental server-side stuff. Maybe someone who actually uses it could spend some time and comment on the changes?

[uklotzde]

Hint (for your next PR, not this one): Don't use the main branch in your fork for feature development. Use a feature branch and always pull main from the upstream repo.

Reset your main branch to upstream after this PR has been finished.

[obayemi]

Hint (for your next PR, not this one): Don't use the main branch in your fork for feature development. Use a feature branch and always pull main from the upstream repo.

yeah it was a feature branch on my repo but I merged It when I considered it "done" for my use case (enough for use in my project) I'm doing PRs to review to my changes to myself before sending it here 😅

[obayemi]

The bulk of the changes is not "really" changes in the server part I just used generics and From/Into traits to actually make use of thing that were already there. It was mainly just making the Service Trait and the servers generic enough to make use of types already present in the project. there are basically only two actual code changes, and a lot of typing stuff to allow it to compile/

Request

The slaveId that was already received in process in the RequestPdu but just not sent in the service call handler that was a non-generic Request that didn't contain the id. so I made a new SlaveRequst containing the slave and made both SlaveRequest and Request implement From<RequestPdu> instead of having only the Request implementing From<RequestAdu> wich did not contain the slaveId. and then in process use

let response = service.call(request.into()).await.map_err(Into::into)?;

instead of

let response = service.call(request.pdu.0).await.map_err(Into::into)?;

Response

Then I had to make the response optional so the server is able to not repond to request not intended for it (in rtu bus there are lots of devices, on the same bus, the master sends a request with a slaveId, and only the targeted device should reply or it will create conflict on the bus and the message will be corrupted).

So I made the Response having to "optionally" need to be Into<ResponseAdu> by Using the TryInto trait. I think it was easier than having it be either Option<Into<ResponseAdu>> or better Into<Option<Into<ResponseAdu>>> I think that TryInto expresses very elegantly the fact that Response should turn into a ResponseAdu, but could also not be, even If the error type is only discarded.

I used ResponseAdu and not ResponsePdu to allow the service to still send pure Response payload if it does not care about the slave, and also to maintain backwards compatibility.

Then the only real change is

match response.try_into() {
    Ok(pdu) => {
        framed.send(rtu::ResponseAdu { hdr, pdu }).await?;
    }
    Err(_) => {
        debug!("skipping reponse");
    }
}

instead of

framed
    .send(rtu::ResponseAdu {
        hdr,
        pdu: response.into(),
    })
    .await?;

[uklotzde]

Thank you for the detailed explanations. Sounds reasonable to me.

During reviews I always try to add missing pieces and clarifications to the code. Either by making the code more expressive or adding comments and docs as needed. PR conversations are transient and all the efforts put into them get lost.

Please rebase and fix the failing checks and builds.

[obayemi]

I think that should fix all the CI errors
and I migrated to using an OptionalResponse(Option<Response>) as the Into target to make the response be truely optional and not a failure of turning it into an actual response.
also changed logs to use the log:: prefix

[uklotzde]

also changed logs to use the log:: prefix

Sorry, I didn't notice that it wasn't you who started importing symbols from the log module 🙈 Legacy code.

[obayemi]

np, good occasion to fix the legacy code :)

[obayemi]

and now that I finally figured out that the test suite was in pre-commit all along, this should fix the last tests

[uklotzde]

Thank you! I'll add a changelog entry.

[obayemi]

already on crates !
awesome thx :)