Design question on tokenized request/response pairs

[Miq1]

I am about to generate a pull request for the token feature I introduced. In short:

• If you have several clients doing requests, you need to keep track which response belonged to what request.
• I added a uint32_t token to the requests that is set by the requester and will be delivered again with the response. Thus the requesting client can tell which request caused the response

The requests are easily modified, the token is added as a new (last) calling parameter with a default value of 0, so the current function signature for the requests will continue working (readHoldingRegister etc.).

The problem to be discussed arises with the onData and onError handlers. Existing implementations will not work with changed onData and onError function signatures that have the token parameter added.

I see three different ways to implement the feature:

1. add the token parameter to onData and onError handlers, knowingly accepting old implementations will break.

2. add two more handlers onDataToken and onErrorToken, that will support the new signatures. The library will keep track of requests tokenized (token!=0) and classic and call the respective handlers.

3. Supply the token at the end of the getData block, extending it by 4 bytes. Implementations using the token feature must take care themselves to get the excess 4 bytes, as getLength will not be changed to not break the old interface.

I have implemented 1., as I could start from scratch, but solution 2. is sounding best to me.

What would you suggest?

[bertmelis]

How about adding a void* as parameter? The same principle is used in several other libraries (also in core, see Ticker).

[Miq1]

I see - the Ticker hint was enlightening. I happen to have used Ticker several times without realizing it has been written by yourself :)

We would lose the signature checks at compilation time, though, and maybe even could cause a problem with memory overruns, if someone erroneously registered a non-token callback, but has the request token set for some reason.

I still am preferring the second option - additional safety for the price of two new callback pointers to be maintained.

[bertmelis]

For ESP32 I only made the implementation (.cpp). The header comes from ESP8266. Having 2 callback pointers is fine by me. Less complexity and more safety.

[Miq1]

Okay, this is set then. I will try to have it ready over the weekend.

[Miq1]

I set up a pull request now and experimentally tried to use the Token branch for my device firmware. I get a compile error there for a missing uint16_t parameter right after the FunctionCode:

src\main.cpp: In function 'void setup()':
src\main.cpp:1333:31: error: no matching function for call to 'esp32ModbusRTU::onDataToken(void (&)(uint8_t, esp32Modbus::FunctionCode, uint8_t*, size_t, uint32_t))'
   RS485.onDataToken(handleData);
                               ^
In file included from src\main.cpp:64:0:
.pio/libdeps/az-delivery-devkit-v4/esp32ModbusRTU/src/esp32ModbusRTU.h:62:8: note: candidate: void esp32ModbusRTU::onDataToken(esp32Modbus::MBRTUOnDataToken)
   void onDataToken(esp32Modbus::MBRTUOnDataToken handler);
        ^
.pio/libdeps/az-delivery-devkit-v4/esp32ModbusRTU/src/esp32ModbusRTU.h:62:8: note:   no known conversion for argument 1 from 'void(uint8_t, esp32Modbus::FunctionCode, uint8_t*, size_t, 
uint32_t) {aka void(unsigned char, esp32Modbus::FunctionCode, unsigned char*, unsigned int, unsigned int)}' to 'esp32Modbus::MBRTUOnDataToken {aka std::function<void(unsigned char, esp32Modbus::FunctionCode, short unsigned int, unsigned char*, short unsigned int, unsigned int)>}'

The handleData function is defined as follows (and was like it since I downloaded your library:

// RS485 Response data handler
void handleData(uint8_t serverAddress, esp32Modbus::FunctionCode fc, uint8_t* data, size_t length, uint32_t token) 
{

The typedef in esp32ModbusTypeDefs.h is this:

typedef std::function<void(uint8_t, esp32Modbus::FunctionCode, uint16_t, uint8_t*, uint16_t, uint32_t)> MBRTUOnDataToken;

The uint16_t parameter after the FunctionCode was brought in with PR #7, breaking the interface. Since I did not pull fresh copies in between, I ran into this issue.

As I see it, the address was meant to serve a similar, but more limited purpose of identifying responses from multiple callers.

I would suggest to leave it in for compatibility reasons, but it looks a bit odd to me.

[bertmelis]

yes, you're right: f85417c

[Miq1]

OK, my edited post and your answer crossed. In case you did not see the edit: I will change my code to cope with the (for me redundant) address parameter.

[Miq1]

Reconsidering the case, I would like to remove again the address parameter in a further pull request. It simply makes no sense with function codes not using addresses at all and it only doubles the more general token functionality in those cases where it will work.

[bertmelis]

I agree offcourse as I didn't implement it myself.

I'm more a fan of the traditional "C" way of passing a void pointer. Then you can pass a ptr-to-struct and have it containing everything you want. I used to do this to access meaningful names or mqtt-topics.

So I agree in dropping the address in exchange of something else. This "something else" has to be generic enough to serve multiple usage cases though. On esp this means something with a size of 32 bits.

[Miq1]

Excellent. The "something else" already is there with the 32 bit token the "Token" PR introduces.

[Miq1]

Done with #42. Closing this issue as finished.