Proposal for callback mechanism in modbus_reply()

[Bodo Meißner]

Hello Stephane,

I would like to use libmodbus as a server/slave in a device where the
data is already stored in several separate data structures. Both the
range of Modbus regiaster addresses and the memory containing the data
is not contiguous.
That means I cannot easily use the arrays in modbus_mapping_t.

In the thread "Raw Data in libmodbus"
<http://groups.google.com/group/libmodbus/browse_thread/thread/5b9099836e17569c>
I found a suggestion of a general callback mechanism.

A callback mechanism in modbus_reply() could be used to handle access to
data outside the mapping structure in my device and similar situations.

In my first sample implementation I used 2 callback functions, one for
reading and one for writing data.

Depending on the function code modbus_reply() checks if callback
functions are defined and calls the corresponding function(s) instead of
accessing the data in mb_mapping.

I uploaded my sample implementation to github:

https://github.com/bomm/libmodbus/compare/callback1

What do you think about this?


Other useful callback functions (not included in my current
implementation) might be:
- user defined access control checks before the actual data access
- copy data to the arrays in mb_mapping before a read access
- copy data from the arrays in mb_mapping after a write access


Bodo

[Daniel Kirkham]

Bodo,

This is the sort of functionality I had in mind when writing my recent email (subj: Validation and Reacting to ModBus Indications, on 13 June) as my code will get the chance to validate values being written to registers, and act on changes to register values.

I'm hoping to find time to code my server/slave this coming weekend, and will attempt to use your code. I will report back once I've made progress or if I run into a problem.

Regards,

Daniel

--

[Bodo Meißner]

Am 27.06.2012 15:21, schrieb Daniel Kirkham:

> I'm hoping to find time to code my server/slave this coming weekend, and
> will attempt to use your code. I will report back once I've made
> progress or if I run into a problem.

Hello Daniel,

this code is work in progress. My implementation was intended as a base
for discussion. I did not test it, I checked that it can be compiled
without errors or warnings.
Maybe this version will never get included into libmodbus, but only used
as an inspiration for a different implementation.

Of course you are free to use it under the original license of
libmodbus, but you should be prepared to change your code when a
different solution will be added to libmodbus.


Bodo

[Bodo Meißner]

Am 26.06.2012 16:05, schrieb Bodo Meiï¿œner:

> A callback mechanism in modbus_reply() could be used to handle access to
> data outside the mapping structure in my device and similar situations.

I refined my proposed implementation of a callback mechanism:
https://github.com/bomm/libmodbus/tree/callback1

See functions modbus_reply(), default_read() and default_write() in
https://github.com/bomm/libmodbus/blob/callback1/src/modbus.c

My changes are still untested.


Bodo

[Bodo Meißner]

Am 27.06.2012 15:21, schrieb Daniel Kirkham:

> I'm hoping to find time to code my server/slave this coming weekend, and
> will attempt to use your code. I will report back once I've made progress
> or if I run into a problem.

Hello Daniel,

I continued my implementation of the callback mechanism today. I tried
to remove duplicate code, renamed the default callback functions and
fixed some bugs in my code. Until now I tested only MODBUS functions 3
and 4 with default callback. (My server will only use one or both of
these functions.) I did not yet try to add user-defined callback functions.

I also considered working without this extension by manipulating the
pointers in the mapping structure and the request between
modbus_receive() and modbus_reply(), but today I found out that this is
not sufficient for my purpose.

I messed up the "callback1" branch on Github because I wanted to fix
wrong author information in old commits. That means it is probably best
to delete the branch and pull again.


Bodo

[Daniel Kirkham]

Bodo,

At this stage I haven't done any work with your code - I've been getting other parts of my code properly structured, and have now hit what appears to be a kernel bug affecting the hardware I'm trying to control. This has slowed my progress somewhat.

But regarding the modbus code, my initial thoughts on any callback functions (before I saw your code) is that they should be designed so they don't have to be aware of the modbus message coding. That is:

* libmodbus decodes and encodes the messages

* user callback functions are passed or return native uint16_t values (for registers) and uint8_t (for bits) 

* user callback functions do not have access to the modbus message buffers (both req and rsp)

I'm not sure of the best way to do this at the moment, but perhaps you can give it some thought as you make your changes.

With luck, I'll have a closer look this coming weekend.

Regards,

Daniel
--

[Bodo Meißner]

Am 04.07.2012 14:04, schrieb Daniel Kirkham:

> But regarding the modbus code, my initial thoughts on any callback
> functions (before I saw your code) is that they should be designed so
they
> don't have to be aware of the modbus message coding. That is:

Hello Daniel,

this matches my intentions only partially.

> * libmodbus decodes and encodes the messages

In my proposal this is done except for the data to be written or read.

> * user callback functions are passed or return native uint16_t values (for
> registers) and uint8_t (for bits)

In my use case accessing native uint16_t values is not what I need. It
would even make it more difficult

Only a small fraction of my data is uint16_t. There are many 32-bit
integers, 32- and 64-bit floats and string values. All numeric values
are already stored as big-endian. That means I simply have to copy the
data between my data structure and request/response buffer.

(This is what prevents me from using the unmodified library with
manipulating the address in the request and the pointers in the mapping
structure.)

For a user like you who wants to access native 16-bit integer values I
would add conversion functions that can be used in the callback functions.

Something like this:

myWriteCallback(... uint8_t *inputData, int inputByteCount, ...)
{
uint16_t wordData[MAX_SIZE];
int wordDataCount = sizeof(wordData) / sizeof(wordData[0]);

modbus_raw_to_uint16(inputData, inputByteCount, wordData,
&wordDataCount);

writeDataFromWordDataToSomewhere();
}

myWriteCallback(... uint8_t *outputData, int *outputByteCount, ...)
{
uint16_t wordData[MAX_SIZE];
int wordDataCount;

readDataFromSomewhereToWordDataAndFillWordDataCount();

modbus_uint16_to_raw(wordData, wordDataCount,
outputData, outputByteCount);
}

The conversion functions would check the buffer size, convert the data
using a htons/ntohs loop and return the resulting size.
As optimization the functions could use memcpy if htons/ntohs does not
modify the data.

> * user callback functions do not have access to the modbus message buffers
> (both req and rsp)

Currently my callback functions will get a pointer to the location
inside the message buffer where the data has to be written or read.
That means the callback functions don't have to (and should not) care
about header, checksum etc.


Bodo

[Daniel Kirkham]

Bodo,

I've finally found time to get back to this project.

Thanks for your comments, I think we are in substantial agreement, and to this end, I've taken your callback1 code and forked a change (see https://github.com/dkirkham/libmodbus/compare/bomm:callback1...dkirkham:callback2). Note that I've only added new functions to src/modbus.c to get and set registers and bits within the modbus request and response packets.

This method could be extended to encompass other types, but that might become a bit too application specific. They may not be particularly efficient (compared with your memcpy optimisation suggestions) but I have individual range checks to complete on each register, so the overhead is not my primary concern.

The problem I have now is that cb_read and cb_write callbacks in my application need to take the function argument (eg. _FC_WRITE_MULTIPLE_REGISTERS) to determine what to do, but these constants are not made available in the public header files, only the library specific src/modbus-private.h

Now, of course I could include modbus-private.h into my application code, but a cleaner solution will be to either insert the _FC* constants into the public modbus.h, or another file alongside it. But given the constants are no longer hidden, the underscore prefix is probably no longer appropriate.

I was wondering how you approached this problem, and if Stephane has any thoughts on the matter.

Regards,

Daniel

--

[Bodo Meissner]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Daniel,

although I already had to use the current version of my changes in my application, I think it is still work in progress.
I appreciate your comments very much, because this will help to make my specific solution useful for others as well.

Am 15.07.2012 13:59, schrieb Daniel Kirkham:

> https://github.com/dkirkham/libmodbus/compare/bomm:callback1...dkirkham:callback2). Note that I've only added new functions to src/modbus.c to get and set registers and bits within the modbus request and response packets.

I would be interested in seeing the piece of your code where you use the new functions in combination with the callback functions.
What you do in these functions is similar to the internal functions modbus_read_any_registers() and modbus_read_any_bits() which I invented to simplify modbus_default_read().
I created functions for read access only because there was nearly the same code for reading input/holding registers and coils/discrete inputs. There was no need to have similar functions for writing.
We could make these functions public, maybe adapt them to your needs and add similar functions for writing.

> This method could be extended to encompass other types, but that might become a bit too application specific. They may not be particularly efficient (compared with your memcpy optimisation suggestions) but I have individual range checks to complete on each register, so the overhead is not my primary concern.

I do range checks for the addresses in my application. For this I have a table which contains base address and size of every individual data structure. I have unused MODBUS addresses between the data structures for future extensions.
So my application compares the specified address and number of registers with a table to check the validity of the address and to find the corresponding data structure. Then it can use memcpy to copy the data to the response buffer.
In my case I don't need any checks for values because I implemented only read access.

If you want to optimize your code I suggest using pointer casts in combination with htons/ntohs.
Instead of
rsp_buf[index++] = value >> 8;
rsp_buf[index] = value & 0xFF;
I would use
*((uint16_t*)(rsp_buf + index)) = htons(value);

The C library might implement the byte swapping in an optimized way and will simply copy the unchanged value on big-endian machines.

> The problem I have now is that cb_read and cb_write callbacks in my application need to take the function argument (eg. _FC_WRITE_MULTIPLE_REGISTERS) to determine what to do, but these constants are not made available in the public header files, only the library specific src/modbus-private.h
>
> Now, of course I could include modbus-private.h into my application code, but a cleaner solution will be to either insert the _FC* constants into the public modbus.h, or another file alongside it. But given the constants are no longer hidden, the underscore prefix is probably no longer appropriate.
>
> I was wondering how you approached this problem, and if Stephane has any thoughts on the matter.

In my case I simply compare the function code with numeric values 3 and 4. (The MODBUS specification defines numbers as function codes anyway.)
I agree that including the constants in a public header file would be a clean solution.


Bodo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAlADHYwACgkQnMz9fgzDSqfVQwCfVH1FKpl/A2bJyL84TNrhpqnq
YtoAoIpHiEajW9ocyxeGFf6dneFCl9w3
=aOUo
-----END PGP SIGNATURE-----

[Daniel Kirkham]

Bodo,

The code example I have is probably a little too long for an email, so look for it here:

https://github.com/dkirkham/examples/blob/master/modbus_callback.c

You can see I'm making use of coils, discrete inputs and registers, and these are both read and written.

This is essentially my first cut of the code, only partially tested, so there are likely to be bugs. There are also optimisations I could make - eg tables with register value bounds, to reduce the need for switch statements.

Regards,

Daniel

--

[Bodo Meißner]

Am 16.07.2012 03:44, schrieb Daniel Kirkham:

> https://github.com/dkirkham/examples/blob/master/modbus_callback.c
>
> You can see I'm making use of coils, discrete inputs and registers, and
> these are both read and written.

Thank you, Daniel!
Now I understand better the purpose of your functions.

I have to think a bit about this.

It is not trivial, but it should be possible to combine your function
modbus_response_set_register() with my function
modbus_read_any_registers(), the same with modbus_response_set_bit() /
modbus_read_any_bits() and add similar functions for write access.


Bodo

[Daniel Kirkham]

Bodo,

I've been completing my modbus application and in the process, found a problem with the value placed in the size variable, a pointer to which is passed to the cb_read callback. While your code correctly finds the size of the buffer available for the response, this is not useful for doing a bounds check in the two set functions I wrote - and since the size of the response packet is known from the modbus specification, size might as well reflect this. (Perhaps, at this same point, there should be a test in server side of the library to ensure the number of requested inputs/coils/registers does not exceed those specified in the modbus specification - presently there are only such tests in the client side of the library).

I've pushed the change to my repo, see https://github.com/dkirkham/libmodbus/compare/bomm:callback1...dkirkham:callback2 for the diffs. I also found a problem with my modbus_response_set_bit function (a line of code that shouldn't have been there) which has been corrected.

At this stage, the callback functionality I have does all that I need it to do, so I'm moving on to other parts of my project. If you or Stephane incorporate any of this code into the main libmodbus code, and the same general functionality is retained, I'll be happy to modify my application code to suit and test.

Regards,

Daniel

--

[Bodo Meissner]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 22.07.2012 05:45, Daniel Kirkham wrote:

> If you or Stephane incorporate any of this code into the main libmodbus code, and the same general functionality is retained, I'll be happy to modify my application code to suit and test.

Hello Daniel,

I'm in the same situation as you. My application needs to support Modbus _now_ and the server code in libmodbus was not sufficient, so I modified it to fit my needs.
I don't have access to the "official" libmodbus code. After my first mail about my proposal for callback fucntions, Stephane wrote that he will think about this and might implement a different approach.
Since I cannot wait for this, my application uses my patched version of libmodbus now.

Bodo

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAlALyEsACgkQnMz9fgzDSqcfagCePRCVyCI8s1lumeUqhmkE6L7s
HRcAnjXXX3AhTs29ubNAiXVOvdEz97wL
=FC3r
-----END PGP SIGNATURE-----