rsmp-nordic / rsmp_core

RSMP core specification
MIT License
6 stars 1 forks source link

Do we need MessageAck if another response is send? #83

Open emiltin opened 3 years ago

emiltin commented 3 years ago

If a response is send, maybe we don't need to send a MessageAck? A response should be enough confirmation that the message was received.

For example, according to the current spec, the exchange should be:

send StatusRequest
receive MessageAck (for the StatusRequest)
receive StatusResponse
send MessageAck (for the StatusResponse)

But maybe this could be simplified to:

send StatusRequest
receive StatusResponse (which is also an implicit ack of the StatusRequest)
send MessageAck (for the StatusResponse)

In other words, all messages must be acknowledged, but if you respond with e.g. a StatusResponse it works as an ack, you can skip the explicit MessageAck.

A MesageAck contains the message id of the original message, which is very useful. If you can skip the MessageAck, all response types, like StatusResponse, CommandResponse, etc. should include the original message id, so you know what message was acknowledged, see also #60.

Thanks to SER (Syndicat des Equipements de la Route) for suggesting this.

otterdahl commented 3 years ago

It is an interesting proposal. There is some written in the faq that partly touches this subject (the faq should be reworked an integrated in the spec by the way...).

I don't see any problem doing this with Status messages. But command messages is a different story. The intent is that the MessageAck should be sent immediately upon receiving a CommandRequest, but the CommandResponse should wait until the command has been executed, which in most case happens very quickly, but could take 1-2 minutes in some cases in a real controller. I can think of a number of commands that takes a while to complete. (switching time plan, order to dark mode, as so on...)

However, in reality most equipment doesn't implement CommandResponse in this way. They send CommandResponse immediately anyway - requiring the supervisor to monitor the corresponding status values in controller in order to tell when the command finishes. I can sort of understand why the suppliers have done it this way since it would mean quite a lot extra work for the RSMP module in the controller to keep track on.

Given this background, perhaps it doesn't matter if we make this change in the spec. It would mean quite an performance increase given the lower overhead. But I think it is a good idea to ask the suppliers what they think.

emiltin commented 3 years ago

Regarding CommandResponses, I think we should make sure the spec and implementations agree. Either the spec should be clarified so that a CommandResponse must be send immediately. In that case it should be clear form the response that it's just an acknowledgement, and no action has been taken yet. Or we change the spec so that an Ack must be send immediately, and the CommandResponse can be sent later.

marcgarba commented 1 year ago

Hello the team,

In fact at the SER group when discussing about it, for the 2 pairs : StatusRequest/StatusResponse and also CommandRequest/CommandResponse, we were thinking of optimizing a little more to just have : send StatusRequest receive StatusResponse (which is also an implicit ack of the StatusRequest) and send CommandRequest receive CommandResponse (which is also an implicit ack of the CommandRequest)

At the end, the site doesn't need to know if the supervision received or not the response. Even if not received, it will not send a copy back. And if not received, anyway the supervision will send again a request.

In the responses StatusResponse and CommandResponse we would replace the "mId" tag per the "oMid" tag to be able to retreive the corresponding request message to implicit ack.

Last point, instead of modifying the existing messages (perhaps a little complicated for compatibilty reasons when a lot of sites installed), we were thinking adding new messages types would be more convenient ? StatusRequestAckIncluded / StatusResponseAckIncluded CommandRequestAckIncluded / CommandResponseAckIncluded (something like that for new names...)

In case of a bad request received (for status or command), we can still receive a not-ack message.

marcgarba commented 1 year ago

To come back on compatibility, by "adding" new messages types (and not modifying current ones) AND using the version field of RSMP Core, we can have :

If new messages types are available since a X.Y "Core" version, when a site connect to a supervision, possible cases :

So is the label"breaking" relevant for this development ?

emiltin commented 1 year ago

During the handshake, the latest version that both ends support is chosen. So they always use the same version. "Breaking change" for this spec means that existing code would not work, but must be changed to work with the latest spec.

For this reason I also think it's better to retire old messages, instead of having both an old and a new version, which will bloat both code, documentation and test tools. If a device is not updated yet, it will use an older version. The supervisor should of course support this older version, as well as the newer.

emiltin commented 1 year ago

You might be interested in the work we do in the "version 4" working group, where we're investigation ideas for a potential version 4 of RSMP, see https://github.com/rsmp-nordic/rsmp_core_v4/discussions. Currently we're looking in to whether we could build RSMP 4 on top of MQTT. It solves a lot of issues like handshake, acknowledgement, buffering, request/response identifiers, etc. Even if we end up now building RSMP 4 on MQTT there's a lot of inspiration in MQTT. S