Interest in changeable handlers for registers and coils

237 views
Skip to first unread message

Åke Forslund

unread,
Oct 7, 2013, 2:53:08 AM10/7/13
to libm...@googlegroups.com
Hi,

In a project I'm working on I need to connect modbus to an already existing structure with settings, inputs and outputs. To accomplish this I've split out the init/free/build response/set/ functionality of libmodbus into separate functions that can be exchanged for my own handler functions. The standard behaviour is very similar to the original code and will require minimum changes on an upgrading user's part.

My very rough proof of concept code is available at https://github.com/forslund/libmodbus/tree/custom-table-handlers

In my case this makes it much easier to connect libmodbus to my project's settings, inputs and outputs but I don't know how others use the project so it might be completely unnecessary. Is this a thing that could be considered to be merged into upstream or is it outside the scope of the project? I personally would like to have it included so I don't have to patch every new version of the library =)

/Åke

Bodo Meissner

unread,
Oct 7, 2013, 11:08:52 AM10/7/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07.10.2013 08:53, �ke Forslund wrote:

> In a project I'm working on I need to connect modbus to an already existing structure with settings, inputs and outputs.

Hello �ke,

I had a similar project last year. It was not possible to change the application to use data structures provided by libmodbus for several reasons.
That's why I added an option to specify callback functions for reading and writing the data. The default (without specifying callback functions) should behave exactly as the unmodified library.

My changes are here:
https://github.com/bomm/libmodbus/commits/callback1

This modified library is used in devices running with embedded Linux. These devices are being sold for more than a year, but I don't know how many users of the device actually use Modbus/TCP communication.


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

iEYEARECAAYFAlJSzoQACgkQnMz9fgzDSqfnhwCfXr4vxCsMmdSHV+EXdm/QxAbi
Q14An0fDZj4UFBBoPbkOE64v4vDW6Ytm
=GgPm
-----END PGP SIGNATURE-----

Bodo Meissner

unread,
Oct 7, 2013, 11:51:29 AM10/7/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07.10.2013 17:08, Bodo Meissner wrote:

> My changes are here: https://github.com/bomm/libmodbus/commits/callback1

I just noticed that two users on Github used my version as a starting point for their extensions.
The latest version seems to be here:
https://github.com/wdouglass/libmodbus


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

iEYEARECAAYFAlJS2IAACgkQnMz9fgzDSqdoLQCdG/h5uwO1EqODM8jiRARBXQpG
VPkAnAuAerl+6a76lZaegtdkntv8A/Ao
=n380
-----END PGP SIGNATURE-----

Åke Forslund

unread,
Oct 8, 2013, 1:41:11 AM10/8/13
to libm...@googlegroups.com
Nice knowing I'm not the only one facing this issue, I'll keep using my own version for the time being since it's updated against the latest libmodbus but I'll have a good read through of your version to see if I can learn anything from your solution.

Since we are a couple of people in need of this feature I'll create a feature request in the github issue-tracker.

Stéphane Raimbault

unread,
Oct 8, 2013, 4:21:55 AM10/8/13
to libm...@googlegroups.com
I want this feature too :)


2013/10/8 Åke Forslund <ake.fo...@gmail.com>
Nice knowing I'm not the only one facing this issue, I'll keep using my own version for the time being since it's updated against the latest libmodbus but I'll have a good read through of your version to see if I can learn anything from your solution.

Since we are a couple of people in need of this feature I'll create a feature request in the github issue-tracker.

--
Vous recevez ce message, car vous êtes abonné au groupe Google Groupes libmodbus.
Pour vous désabonner de ce groupe et ne plus recevoir d'e-mails le concernant, envoyez un e-mail à l'adresse libmodbus+...@googlegroups.com.
Pour plus d'options, visitez le site https://groups.google.com/groups/opt_out .

Bodo Meissner

unread,
Oct 8, 2013, 5:28:50 AM10/8/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08.10.2013 10:21, St�phane Raimbault wrote:
> I want this feature too :)

I read a bit more in the latest modifications by Github user Woodrow Douglass.
The commit https://github.com/wdouglass/libmodbus/commit/a43f97f1f44d08b43b42ac0f80ea6c5eeec4ba9e doesn't seem to fit my requirements.
If I understand it correct, this includes the following change of the behaviour:
In any case the default read or write function will be called first which accesses the data in the mapping structure. At the end of these functions the corresponding callback/notification function will be called.

In my application I have a very large existing data structure which has to be mapped to Modbus registers. To allow future extensions of the structure, the Modbus registers mapping is not contiguous.
That's why I don't want to use any data arrays in the mapping structure. My callback functions completely replace the data access.

To fit both requirements we probably need some flags to determine if the callback functions should should be called instead of or in addition to the default access functions. If the callback functions will be called additionally, it might be useful to specify if these will be called before or after the default functions. At least for reading values I would want to call the callback function before the default access function, not afterwards.


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

iEYEARECAAYFAlJT0FEACgkQnMz9fgzDSqer3wCfTL/OvhuY1HKfo9Bf5kdCMxPM
GNQAn3LIrO+ghY+l/nceqmTtTB8os0j+
=97Lo
-----END PGP SIGNATURE-----

Åke Forslund

unread,
Oct 9, 2013, 2:00:43 AM10/9/13
to libm...@googlegroups.com
I don't think a flag will be necessary, the read/write callback function can be implemented to read/write in any number of locations. I also think the default behaviour should be handled by a callback as well (what is selected by default) to make modbus_reply() simpler.

Do you mean that the modbus address is sparsely mapped or that the pointers aren't mapped after each other? I would personally like to have a non-continuous modbus map and would therefore like to move the "is-address-valid"-check to the callback functions.



Den tisdagen den 8:e oktober 2013 kl. 11:28:50 UTC+2 skrev Bodo Meissner:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Bodo Meissner

unread,
Oct 9, 2013, 2:51:50 AM10/9/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09.10.2013 08:00, �ke Forslund wrote:
> I don't think a flag will be necessary, the read/write callback function can be implemented to read/write in any number of locations. I also think the default behaviour should be handled by a callback as well (what is selected by default) to make modbus_reply() simpler.

I agree. A flag is not necessary if the user-defined callback function can call the default function. The only advantage of the flag is that the default functions don't have to be exposed to the official interface.

I implemented it as you suggested. My library code contains functions modbus_default_read() and modbus_default_write() which are indeed default callback functions.
If the user specified a non-NULL callback function for read and/or write access this user-defined function is called instead of the corresponding default function.

I distinguished between read and write access only because of different argument lists. The callback functions can do different things depending on the supplied function code. The default functions are good examples for this.

The mentioned change in Woodrow Douglass' version will always call the default function and the default function will call the user-defined callback.
I don't know his requirements. I only thought about a way to combining my approach with his one. That's why I came up with the idea of inventing a flag.

> Do you mean that the modbus address is sparsely mapped or that the pointers aren't mapped after each other? I would personally like to have a non-continuous modbus map and would therefore like to move the "is-address-valid"-check to the callback functions.

In my application the address space is sparsely mapped to a big structure that contains nearly 100 sub-structures. I need some reserved addresses in case any of the sub-structures has to be extended. All address checks are done in the callback function. In my callback function I use a table that maps a Modbus register start address to the address and size of a structure. A range of addresses will be mapped to the memory area of the sub-structure and my address check forbids accessing memory after the end of the sub-structure.
My table looks similar to this:
{
/* modbus address, offset, size */
{ 0, offsetof(bigStructure_t, subStruct0), sizeof(subStruct0_t) },
{ 100, offsetof(bigStructure_t, subStruct1), sizeof(subStruct1_t) },
{ 500, offsetof(bigStructure_t, subStruct2), sizeof(subStruct2_t) },
/* etc ... */
{ 0, 0, 0 }
}

To calculate the start address of every sub-structure I used a separate program that rounds up the actual size of every sub-structure to a nice value with an option to manually insert larger reserved blocks in between.


Unfortunately Git cannot automatically merge my changes with the latest default branch. If anyone wants to use my version I will invest the time and try to manually do the merge.


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

iEYEARECAAYFAlJU/QUACgkQnMz9fgzDSqc2rwCghLwnQDwtjpBMJZ/vAvKLo2AN
Qz4An1wHV+uBsAArqYAlHpRakEftndER
=iTjQ
-----END PGP SIGNATURE-----

Åke Forslund

unread,
Oct 9, 2013, 6:10:07 AM10/9/13
to libm...@googlegroups.com
I could pretty easily make my version allow a sparse modbus-address map by moving the invalid address check to the callback functions, as it is already based on the latest libmodbus master branch. I have separate functions for coils and holding registers but I _think_ your functions should be usable together with a couple of very small wrapper functions.

I'll make a pull request and then we can discuss what's good/bad and what needs to change.


Den onsdagen den 9:e oktober 2013 kl. 08:51:50 UTC+2 skrev Bodo Meissner:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Daniel Kirkham

unread,
Oct 9, 2013, 7:25:32 AM10/9/13
to libm...@googlegroups.com
Åke, Bodo,

As a bit of background, I forked Bodo's callback code to form mine at https://github.com/dkirkham/libmodbus/tree/callback2
This was forked again by wdouglass, as noted by Bodo.

I had two main motivations for a callback mechanism:
* For my code to know when a register or coil has been written, so it can actually control a device, and likewise, when a register or input is being read, to get the latest data from the device
* To allow for range checking when registers were being written, and reject incorrect write requests

Bodo's code did most of what I wanted, but I decided that the code would be better if libmodbus was purely responsible for decoding and encoding packets, which meant I needed to add functions to libmodbus to read and write the register and coil values - these functions would be called from the callback functions - hence the modbus_response_set_{register|bit}() and modbus_request_get_{register|bit}() functions.

In the process I found a problem with the calculation of the cb_read size variable, which affected the range checking.

The downside to my implementation is that the callback functions, in my application code, need access to some hitherto non-public constants (eg. _FC_WRITE_MULTIPLE_REGISTERS) which are defined in the library specific src/modbus-private.h, rather than a public header file. I haven't made this change in my repo, I just added them into my application code (not pretty, but expedient).

I'm using my forked code in my own device at the moment, and haven't taken the time to keep it up to date with Stéphane's releases.
 
I would commend that a callback mechanism like this is incorporated into the modbus library. If there is interest, I'm certainly happy to help with adapting the code and/or testing it in my device.

Regards,

Daniel
--

Bodo Meissner

unread,
Oct 9, 2013, 8:34:20 AM10/9/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09.10.2013 13:25, Daniel Kirkham wrote:

> I'm using my forked code in my own device at the moment, and haven't taken the time to keep it up to date with St�phane's releases.

Hello all,

it's similar with my version. I'm currently no longer involved in the project that uses my modified library. (I have to do some bugfixing or smaller work from time to time, but until now nothing related to Modbus.)
"My" device provides read-only acces to the data via Modbus. I only needed to implement register access and it does not distinguish between "input registers" and "holding registers". That's why my callback functions are very simple compared to the default functions.
I also don't know if any user who bought the device actually uses Modbus communication. (Maybe the implementation was only for marketing purposes, because the competitors' devices also have this in their data sheets.)

> I would commend that a callback mechanism like this is incorporated into the modbus library. If there is interest, I'm certainly happy to help with adapting the code and/or testing it in my device.

I agree that it would be good to have this in the "official" library. Unfortunately I can no longer adapt my application to use a new version of the library and I cannot test it.
That's why I propose �ke and St�phane should decide how to integrate this feature into the current version.
Feel free to ask me for information or to discuss the concept for the final solution.


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

iEYEARECAAYFAlJVTUwACgkQnMz9fgzDSqfr9wCfXsboURa6b+YwxlFP+cpGd2i2
x1cAnjWH4QvgJN0i78CikVPQjGLyiTw2
=icgI
-----END PGP SIGNATURE-----

Åke Forslund

unread,
Oct 15, 2013, 5:21:23 AM10/15/13
to libm...@googlegroups.com
Hi,

tried to get something generic working: I would be happy to receive comments on https://github.com/stephane/libmodbus/pull/148

It's not ready to be merged but is something to base further discussions on.

Best regards
/Åke

Bodo Meissner

unread,
Oct 17, 2013, 5:40:00 PM10/17/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 15.10.2013 11:21, �ke Forslund wrote:

> tried to get something generic working: I would be happy to receive comments on https://github.com/stephane/libmodbus/pull/148
>
> It's not ready to be merged but is something to base further discussions on.

Hello �ke,

I just started reading your changes. I did not analyze everything, so I have some general comments first

In your proposal you have multiple callback functions for different types of data. I think this is better than my idea of having a single callback function which internally switches between different function codes or data types. With your approach the callback functions are simpler.

Additionally I have specific comment for your commit "Move complexity from user handlers to internals" https://github.com/forslund/libmodbus/commit/185dfde19cf6c936ff1a5a7d54209b9c99af9645

Instead of implementing a request for multiple registers (or other data elements) by repeatedly calling a function for accessing a single register I prefer having a callback for multiple registers and to think of a single register access as a special case.
There are two reasons for this:
1. I would like to avoid the overhead of repeated function calls and address checks. If the client sends repeated requests for single registers we cannot do anything about it, but I want to optimize requests for multiple registers by doing a single address check for the whole range.
If the byte order of the existing data structures is big-endian the data transfer can be implemented using memcpy(). (as in my application)
2. In my application I have a lot of 32-bit or 64-bit data which should be read in an atomic operation. For these data I want to avoid a request for a single Modbus register. The client should preferably use a request for the whole set of registers to avoid inconsistent data.
If the user-defined callback sees only a single register access it has no chance to ensure consistent 32-bit or 64-bit data.

I think it is worth having the complexity of a loop and a data buffer in the user defined callback for the resulting efficiency and (option for) data consistency.


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

iEYEARECAAYFAlJgWTAACgkQnMz9fgzDSqdEUgCglYN6ohpnkWEjk4/6N6BdiB0n
AaAAn1VIxzKYekUt6vQtezhx7ahqL/8e
=NGaW
-----END PGP SIGNATURE-----

Åke Forslund

unread,
Oct 22, 2013, 7:26:14 AM10/22/13
to libm...@googlegroups.com
Thanks for the reply.

I get your point of view and I'll reintroduce the set/get_multiple_registers. Maybe I'll add a fallback function for them, so they aren't needed but if you add them you get the better performance?

I'll implement this and push the changes to the pull-request. It might not be until this weekend since I'm away on work.

/Åke



Den torsdagen den 17:e oktober 2013 kl. 23:40:00 UTC+2 skrev Bodo Meissner:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 15.10.2013 11:21, �ke Forslund wrote:

> tried to get something generic working: I would be happy to receive comments on https://github.com/stephane/libmodbus/pull/148
>
> It's not ready to be merged but is something to base further discussions on.

Hello �ke,

Bodo Meissner

unread,
Oct 22, 2013, 10:00:54 AM10/22/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 22.10.2013 13:26, �ke Forslund wrote:

> I get your point of view and I'll reintroduce the set/get_multiple_registers. Maybe I'll add a fallback function for them, so they aren't needed but if you add them you get the better performance?

Hallo �ke,

the fallback function is a good idea.

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

iEYEARECAAYFAlJmhRYACgkQnMz9fgzDSqcPdQCbBhOdR3EF4UU4HPHjr1dc7AQL
/TUAoJXBwG8Q7QyDRVxEwApanXlROPij
=L8hn
-----END PGP SIGNATURE-----

Daniel Kirkham

unread,
Oct 23, 2013, 8:05:52 AM10/23/13
to libm...@googlegroups.com
Åke, Bodo,

Although not useful in my application, being able to read and write multiple registers in one atomic operation is a useful feature.
I agree that the default functions for reading and writing multiple registers should be replaceable with a callback, and that
these multiple register functions call the single register versions, which can themselves be replaced with a callback.

The user code can then use either the multiple register callback or the single register callback as requirements dictate.


Having multiple callback functions for different types of data I think is also a good idea. If you have a look at the example
user code I shared a year of so back (see https://github.com/dkirkham/examples/blob/master/modbus_callback.c), you will
see I needed the MODBUS function constants (eg. _FC_WRITE_SINGLE_REGISTER) in that code - and as I commented
in this forum, the library didn't export these in the public include file, so these constants had to be repeated in my user code
- which is bad. Your solution gets around this problem.


There is a capability that your code doesn't have, which may be desirable for some users, and that is the ability to return
exception codes when invalid values are written into holding registers (ie. MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE)
or if the register map has holes in it (ie. MODBUS_EXCEPTION_ILLEGAL_DATA_ADDRESS). You will see how I've done this
in the callback code example referenced above.

The key difference, if you look at that code and my forked modbus.c function
(see https://github.com/dkirkham/libmodbus/blob/callback2/src/modbus.c) is that the callback functions return the exception
code which then goes into forming the exception reply. I think Bodo added this code originally. However, in your code, there
is the assumption that if the callback function returns a negative value, that the exception is
MODBUS_EXCEPTION_ILLEGAL_DATA_ADDRESS. I think having the callback functions return the exception code is a more
useful capability and is worth preserving.


I won't have the chance over the next couple of weeks (work commitments and travel) but if we can stabilise the callback interface, I could have a go at rewriting the example code referenced above and test it in my application.

Regards,

Daniel
--
On Wednesday, October 23, 2013 1:00:54 AM UTC+11, Bodo Meissner wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 22.10.2013 13:26, �ke Forslund wrote:

> I get your point of view and I'll reintroduce the set/get_multiple_registers. Maybe I'll add a fallback function for them, so they aren't needed but if you add them you get the better performance?

Hallo �ke,

Åke Forslund

unread,
Oct 23, 2013, 11:02:27 AM10/23/13
to libm...@googlegroups.com
Thanks Daniel,

you are right returning the exception code is definitely more useful. The solution I used was the minimum solution to work in my case. I'll look into your solution and move more error handling to the callbacks.

I'll update my pull request as I get along. It'll probably be 3 or 4 commits before I have handled all the issues raised here. And knowing myself I've probably missed something =)

Thanks for all the feedback, since I'm not a very experienced modbus user it's easy for me to miss obvious use-cases.

/Åke

Julien Blanc

unread,
Oct 23, 2013, 11:11:49 AM10/23/13
to libm...@googlegroups.com
Le 23/10/2013 17:02, �ke Forslund a �crit :
> Thanks Daniel,
>
> you are right returning the exception code is definitely more useful.
> The solution I used was the minimum solution to work in my case. I'll
> look into your solution and move more error handling to the callbacks.
>
> I'll update my pull request as I get along. It'll probably be 3 or 4
> commits before I have handled all the issues raised here. And knowing
> myself I've probably missed something =)
>
> Thanks for all the feedback, since I'm not a very experienced modbus
> user it's easy for me to miss obvious use-cases.
>
> /�ke

Hi,

I�m currently testing your code, and one thing i see that disturb me is
that callbacks are global functions. Would�nt it be better if they were
part of the context, so you can provide two modbus services inside the
same program ?

Since the context is an opaque structure, i think it won�t break the
current api/abi.

Also, the set_register_handlers and get_io_bits_handlers should be named
modbus_set_register_handlers and modbus_io_bits_handlers IMHO.

Regards,

Julien

Åke Forslund

unread,
Oct 23, 2013, 3:26:04 PM10/23/13
to libm...@googlegroups.com
Hi!

I considered that but I discarded it because at first I didn't want to mess with existing structures and reasoned that if you wanted many different servers you created separate programs. But you have a point, in embedded systems it would be a nice feature to be able to manage multiple servers in a single product.

It's an easy enough modification and as you say it's a cleaner solution. I'll add it to my todo list.

I'll better get started on these as soon as I get home since the list keeps growing =)

/Åke

Julien Blanc

unread,
Oct 24, 2013, 10:50:41 AM10/24/13
to libm...@googlegroups.com
Le 23/10/2013 21:26, Åke Forslund a écrit :
Hi!

I considered that but I discarded it because at first I didn't want to mess with existing structures and reasoned that if you wanted many different servers you created separate programs. But you have a point, in embedded systems it would be a nice feature to be able to manage multiple servers in a single product.

It's an easy enough modification and as you say it's a cleaner solution. I'll add it to my todo list.

I'll better get started on these as soon as I get home since the list keeps growing =)

Hi!

Just so that we don’t do the work twice, i’ve forked your github repo and started working on this.

I’ve run into a few problems, namely the context not being available everywhere in private functions, and these function working not on the mapping itself but rather directly on the arrays of the mappings (which imho could be changed).

The main concern is that there is not really the notion of « storage backend » in the library. The modbus_mapping_t structure gets this role, but has a few problems :
- it is not opaque
- its members are often used directly

I think my need is maybe a bit different from yours. You just need to customize the reading / writing, whereas in fact i simply need a completely different storage backend. In my case, this will mean :
- break the existing modbud_mapping_t structure,
- create an interface with a reference vfptable
- reimplement the modbus_mapping_t structure. I think i can keep source compatibility (but this will mean keeping it public, where it would be better opacified), but not binary one.

In your case, and contrary to what i said firstly, i think from a design point of view the callbacks should belong to the mapping object, not the context. However, this will break the ABI (although it can be done in such a way that will probably not arm most programs).

Note : i come from the c++ world, so tend to think in object and interface. There may be another way to achieve what i want, comments are welcome.

Regards,

Julien



/Åke

Den onsdagen den 23:e oktober 2013 kl. 17:11:49 UTC+2 skrev Julien Blanc:
Le 23/10/2013 17:02, �ke Forslund a �crit :
> Thanks Daniel,
>
> you are right returning the exception code is definitely more useful.
> The solution I used was the minimum solution to work in my case. I'll
> look into your solution and move more error handling to the callbacks.
>
> I'll update my pull request as I get along. It'll probably be 3 or 4
> commits before I have handled all the issues raised here. And knowing
> myself I've probably missed something =)
>
> Thanks for all the feedback, since I'm not a very experienced modbus
> user it's easy for me to miss obvious use-cases.
>
> /�ke

Hi,

I�m currently testing your code, and one thing i see that disturb me is
that callbacks are global functions. Would�nt it be better if they were
part of the context, so you can provide two modbus services inside the
same program ?

Since the context is an opaque structure, i think it won�t break the
current api/abi.

Also, the set_register_handlers and get_io_bits_handlers should be named
modbus_set_register_handlers and modbus_io_bits_handlers IMHO.

Regards,

Julien
--
Vous recevez ce message, car vous êtes abonné au groupe Google Groupes libmodbus.
Pour vous désabonner de ce groupe et ne plus recevoir d'e-mails le concernant, envoyez un e-mail à l'adresse libmodbus+...@googlegroups.com.
Pour plus d'options, visitez le site https://groups.google.com/groups/opt_out .


--
 

Julien BLANC
Co-Fondateur
Responsable Informatique

NMC Company
16,rue Isly 87000 Limoges
Tel: +33 637 675 306, +33 972 417 093
julien...@nmc-company.fr

Åke Forslund

unread,
Oct 24, 2013, 11:28:04 AM10/24/13
to libm...@googlegroups.com


Den torsdagen den 24:e oktober 2013 kl. 16:50:41 UTC+2 skrev Julien Blanc:
Le 23/10/2013 21:26, Åke Forslund a écrit :
Hi!

I considered that but I discarded it because at first I didn't want to mess with existing structures and reasoned that if you wanted many different servers you created separate programs. But you have a point, in embedded systems it would be a nice feature to be able to manage multiple servers in a single product.

It's an easy enough modification and as you say it's a cleaner solution. I'll add it to my todo list.

I'll better get started on these as soon as I get home since the list keeps growing =)

Hi!

Just so that we don’t do the work twice, i’ve forked your github repo and started working on this.

I’ve run into a few problems, namely the context not being available everywhere in private functions, and these function working not on the mapping itself but rather directly on the arrays of the mappings (which imho could be changed).

The main concern is that there is not really the notion of « storage backend » in the library. The modbus_mapping_t structure gets this role, but has a few problems :
- it is not opaque
- its members are often used directly

I think my need is maybe a bit different from yours. You just need to customize the reading / writing, whereas in fact i simply need a completely different storage backend. In my case, this will mean :
- break the existing modbud_mapping_t structure,
- create an interface with a reference vfptable
- reimplement the modbus_mapping_t structure. I think i can keep source compatibility (but this will mean keeping it public, where it would be better opacified), but not binary one.

In your case, and contrary to what i said firstly, i think from a design point of view the callbacks should belong to the mapping object, not the context. However, this will break the ABI (although it can be done in such a way that will probably not arm most programs).

Note : i come from the c++ world, so tend to think in object and interface. There may be another way to achieve what i want, comments are welcome.

Regards,

Julien


Hi Julien,

regarding the context, I came to the same conclusion when I looked through the code earlier today but I didn't get any further in my thoughts before my current work got in the way. the mapping-struct is probably a good place to put them but I haven't thought it through at any great length.

I don't quite understand how our needs are different? I need the mapping to work against an existing array of variables. I built a (global) struct from a config file and created a map from modbus address to a random memory location. It's an ugly solution and my solution is to add void *arg arguments to modbus_map_new() to be passed to the init function of the different handlers. This will change the API but in another pull-request practically the same thing was was proposed and the libmodbus maintainer was positive to that.

you have to explain the concept vfptable? since I'm not familiar with it.

what do you mean with reimplement the modbus_mapping-structure? extend the existing one or build a new from scratch?

I'll look into your changes and see if I can pull them directly into my branch or if it collides with code I've written but not yet pushed. What's your username on github?

Åke Forslund

unread,
Oct 24, 2013, 11:39:19 AM10/24/13
to libm...@googlegroups.com
Thinking out loud, maybe we should just pass a big backend-object to modbus_mapping_new() with everything we need already set up. add a

  backend_t *be;

to the modbus_mapping-structure and extend the modbus_mappig_new() to accept the backend as an argument, if the backend is NULL then it inits the default backend and stores it internally.

but would this be too complicated a solution since there is much more to set up from the user perspective? Please explain your thoughts more was it something along these lines or was it simpler? =)

It's late in the day and my brain hurts, I hope I've not written to incoherently =)


/Åke

Den måndagen den 7:e oktober 2013 kl. 08:53:08 UTC+2 skrev Åke Forslund:

Julien Blanc

unread,
Oct 24, 2013, 12:25:01 PM10/24/13
to libm...@googlegroups.com
Le 24/10/2013 17:39, �ke Forslund a �crit :
> Thinking out loud, maybe we should just pass a big backend-object to
> modbus_mapping_new() with everything we need already set up. add a
>
> backend_t *be;
>
> to the modbus_mapping-structure and extend the modbus_mappig_new() to
> accept the backend as an argument, if the backend is NULL then it
> inits the default backend and stores it internally.
>
> but would this be too complicated a solution since there is much more
> to set up from the user perspective? Please explain your thoughts more
> was it something along these lines or was it simpler? =)
>
> It's late in the day and my brain hurts, I hope I've not written to
> incoherently =)

That just seems to resemble a lot to what i�m currently doing.

I�ve defined the following structure :

typedef struct _modbus_storage_backend_vfptable
{
int version;
int (*read_coils)(void* backend, uint16_t starting_address, uint16_t
quantity, uint8_t* byte_count, uint8_t[] coils);
int (*read_inputs)(void* backend, uint16_t starting_address, uint16_t
quantity, uint16_t* byte_count, uint8_t[] inputs);
int (*read_holding_registers)(void* backend, uint16_t starting_address,
uint16_t quantity, int* byte_count, uint16_t[] values);
int (*read_input_registers)(void* backend, uint16_t starting_address,
uint16_t quantity, int* byte_count, uint16_t[] values);
int (*write_single_coil)(void* backend, uint16_t address, bool on);
int (*write_single_register)(void* backend, uint16_t address, uint16_t
value);
int (*write_multiple_coils)(void* backend, uint16_t starting_address,
uint16_t quantity, const uint8_t[] values);
int (*write_multiple_registers(void* backend, uint16_t starting_address,
uint16_t quantity, const uint16_t values);
} modbus_storage_backend_vfptable;


which contains the callbacks used to read/write data to the backend.
That�s basically a virtual function pointer table : a list of functions
that belongs to a specific class of objects. Usually, it is instantiated
once and thus resides in the data zone.

The modbus_mapping_t structure is modified as follows :
typedef struct {
const modbus_storage_backend_vfptable* vfp_table;
int nb_bits;
int nb_input_bits;
int nb_input_registers;
int nb_registers;
void *tab_bits;
void *tab_input_bits;
void *tab_input_registers;
void *tab_registers;
} modbus_mapping_t;

A private static structure will contain the default functions for
handling data (which, by the way, will be private). The
modbus_mapping_new function will take care that vfp_table correctly
points to it.

modbus_reply now has the following signature :
int modbus_reply(modbus_t *ctx, const uint8_t *req,
int req_length, modbus_storage_backend *mb_storage_backend)

Where modbus_storage_backend can be any structure having as a first
member a modbud_storage_backend_vfptable.

Thus, in modbus_reply, read / write are delegated to the functions
defined in modbus_storage_backend_vfptable, with calls like

((modbus_storage_backend_vfptable*)mb_storage_backend)->read_coils(mb_storage_backend,
� ).

This will ensure that :
- the correct function is called for the backend
- the backend object is passed back to the function.

I think this approach is clean in a number of ways :
- it clearly separates the protocol from the storage. The storage
backend does just that : store and give data, and the modbus part has no
idea about how data is stored.
- it makes the library really extensible in the terms of where the data
comes from
- it retains source compatibility with current library (binary
compatibility is broken, because of the changes in modbus_mapping_t),
thus doesn�t makes it harder to use if you don�t need the extra
functionality

It has the following drawbacks
- the extra dereference in the function has a cost
- writing a backend is not trivial (although with good exemples it can
be explained well).

I will commit my changes to my github account
(https://github.com/Julien-Blanc/libmodbus) as soon as i get something
working (hopefully tomorrow)

Regards,

Julien

Åke Forslund

unread,
Oct 24, 2013, 1:23:22 PM10/24/13
to libm...@googlegroups.com
This will ensure that :
- the correct function is called for the backend
- the backend object is passed back to the function.

I think this approach is clean in a number of ways :
- it clearly separates the protocol from the storage. The storage
backend does just that : store and give data, and the modbus part has no
idea about how data is stored.
- it makes the library really extensible in the terms of where the data
comes from
- it retains source compatibility with current library (binary
compatibility is broken, because of the changes in modbus_mapping_t),
thus doesn�t makes it harder to use if you don�t need the extra
functionality

It has the following drawbacks
- the extra dereference in the function has a cost
- writing a backend is not trivial (although with good exemples it can
be explained well).

I will commit my changes to my github account
(https://github.com/Julien-Blanc/libmodbus) as soon as i get something
working (hopefully tomorrow)

Regards,

Julien

Cool I look forward to looking at this when I get back home on saturday.

The thing I would have done differently is that I would have pointers to the data storage together with the callback-functions inside the modbus_map_t. This makes sure you don't mix set's of data-structures with callback-functions meant for other data-structures. And you don't need to modify the modbus_reply()-interface.

Also I'm not sure I like sending the backend_data_structure to each callback function since that adds unnecessary complexity to those functions. you could send a pointer to the coil-sub-data-structure if the single backend-object contains pointers to each of the data stores.

in modbus_reply something like
mb_map->store->set_coils(mb->map->coils, ...);

/Åke

Stéphane Raimbault

unread,
Oct 24, 2013, 2:17:47 PM10/24/13
to libm...@googlegroups.com
Hi Åke and Julien,

I'm please to see your interest on the subject and the way you try to achieve a clean and versatile solution.
I think about this new feature yesterday and my idea on the subject is a bit different.

We could define two very simple entry points to hook all libmodbus functions (server side):
bool (*read) (int function, int address, int nb, const void *data);
bool (*write) (int function, int address, int nb, void *data);

with two new libmodbus functions to set these hooks in a modbus_t context:
- modbus_set_read_backend(ctx, my_custom_read_function, my_custom_data). The last argument is used to give a pointer to any custom data and can be NULL if you just want a notification system. 
- ...

The two entry points will be set to NULL by default in libmodbus context and won't be used by default.

To illustrate how libmodbus will handle these new entry points, here is an example in modbus_reply with MODBUS_FC_READ_COILS:

            rsp_length = ctx->backend->build_response_basis(&sft, rsp);
            if (ctx->read != NULL) {
                 ctx->read(function, address, nb,  ctx->data_read);
            }
            rsp[rsp_length++] = (nb / 8) + ((nb % 8) ? 1 : 0);
            rsp_length = response_io_status(address, nb,
                                            mb_mapping->tab_bits,
                                            rsp, rsp_length);


with this solution we avoid overhead of the backends when not used.

For example to write a storage for a database. The goal of our example is to read values from the database only when required (it can be slower than reading everything once for a loop with many clients and high frequency, it depends on your environment...):

my_custom_read can fetch a result from a database (data_read can be a pointer to a struct with my connection, current mb_mapping, etc), copy the results of reading in mb_mapping->tab_bits as usual (no need to pack the bits for the reply, etc) and response_io_status will do the rest.

It's also easy to implement a notification system (for example, to be notified when a register is written) w/o altering libmodbus behaviour (but don't do anything stupid in the entry point which could block the reply).

For a notifcation system, write() could be (pseudo code):

if (function in (
            MODBUS_FC_WRITE_AND_READ_REGISTERS, 
            MODBUS_FC_WRITE_MULTIPLE_REGISTERS,
            MODBUS_FC_WRITE_SINGLE_REGISTER) and address <= 42 and 42 <= address + nb - 1) {
   alert('Great we have received a new temperature value')
}

With this solution, the current mapping keeps a central position, and a copy to mb_mapping is required but I think it's very simple to write callbacks this way.

What do you think?

Stéphane

PS: I have some patches on my disk to export MODBUS_FC_*


2013/10/24 Julien Blanc <julien...@nmc-company.fr>
Le 24/10/2013 17:39, Åke Forslund a écrit :

Thinking out loud, maybe we should just pass a big backend-object to modbus_mapping_new() with everything we need already set up. add a

backend_t *be;

to the modbus_mapping-structure and extend the modbus_mappig_new() to accept the backend as an argument, if the backend is NULL then it inits the default backend and stores it internally.

but would this be too complicated a solution since there is much more to set up from the user perspective? Please explain your thoughts more was it something along these lines or was it simpler? =)

It's late in the day and my brain hurts, I hope I've not written to incoherently =)

That just seems to resemble a lot to what i’m currently doing.

I’ve defined the following structure :


typedef struct _modbus_storage_backend_vfptable
{
int version;
int (*read_coils)(void* backend, uint16_t starting_address, uint16_t quantity, uint8_t* byte_count, uint8_t[] coils);
int (*read_inputs)(void* backend, uint16_t starting_address, uint16_t quantity, uint16_t* byte_count, uint8_t[] inputs);
int (*read_holding_registers)(void* backend, uint16_t starting_address, uint16_t quantity, int* byte_count, uint16_t[] values);
int (*read_input_registers)(void* backend, uint16_t starting_address, uint16_t quantity, int* byte_count, uint16_t[] values);
int (*write_single_coil)(void* backend, uint16_t address, bool on);
int (*write_single_register)(void* backend, uint16_t address, uint16_t value);
int (*write_multiple_coils)(void* backend, uint16_t starting_address, uint16_t quantity, const uint8_t[] values);
int (*write_multiple_registers(void* backend, uint16_t starting_address, uint16_t quantity, const uint16_t values);
} modbus_storage_backend_vfptable;


which contains the callbacks used to read/write data to the backend. That’s basically a virtual function pointer table : a list of functions that belongs to a specific class of objects. Usually, it is instantiated once and thus resides in the data zone.


The modbus_mapping_t structure is modified as follows :
typedef struct {
const modbus_storage_backend_vfptable* vfp_table;
int nb_bits;
int nb_input_bits;
int nb_input_registers;
int nb_registers;
void *tab_bits;
void *tab_input_bits;
void *tab_input_registers;
void *tab_registers;
} modbus_mapping_t;

A private static structure will contain the default functions for handling data (which, by the way, will be private). The modbus_mapping_new function will take care that vfp_table correctly points to it.

modbus_reply now has the following signature :
int modbus_reply(modbus_t *ctx, const uint8_t *req,
int req_length, modbus_storage_backend *mb_storage_backend)

Where modbus_storage_backend can be any structure having as a first member a modbud_storage_backend_vfptable.

Thus, in modbus_reply, read / write are delegated to the functions defined in modbus_storage_backend_vfptable, with calls like

((modbus_storage_backend_vfptable*)mb_storage_backend)->read_coils(mb_storage_backend, … ).


This will ensure that :
- the correct function is called for the backend
- the backend object is passed back to the function.

I think this approach is clean in a number of ways :
- it clearly separates the protocol from the storage. The storage backend does just that : store and give data, and the modbus part has no idea about how data is stored.
- it makes the library really extensible in the terms of where the data comes from
- it retains source compatibility with current library (binary compatibility is broken, because of the changes in modbus_mapping_t), thus doesn’t makes it harder to use if you don’t need the extra functionality


It has the following drawbacks
- the extra dereference in the function has a cost
- writing a backend is not trivial (although with good exemples it can be explained well).

I will commit my changes to my github account (https://github.com/Julien-Blanc/libmodbus) as soon as i get something working (hopefully tomorrow)

Regards,

Julien
--
Vous recevez ce message, car vous êtes abonné au groupe Google Groupes libmodbus.
Pour vous désabonner de ce groupe et ne plus recevoir d'e-mails le concernant, envoyez un e-mail à l'adresse libmodbus+unsubscribe@googlegroups.com.

Julien Blanc

unread,
Oct 25, 2013, 12:40:56 PM10/25/13
to libm...@googlegroups.com
Le 24/10/2013 20:17, St�phane Raimbault a �crit :
> Hi �ke and Julien,
>
> I'm please to see your interest on the subject and the way you try to
> achieve a clean and versatile solution.
> I think about this new feature yesterday and my idea on the subject is
> a bit different.

Hi St�phane, and first of all, thanks for having done such a good job on
the library !

>
> We could define two very simple entry points to hook all libmodbus
> functions (server side):
> bool (*read) (int function, int address, int nb, const void *data);
> bool (*write) (int function, int address, int nb, void *data);

That was in fact not far from my first idea, but Bodo�s remarks about
keeping hidden the library internals as well as modbus protocol details
is appealing.

[snipped the details]
> With this solution, the current mapping keeps a central position, and
> a copy to mb_mapping is required but I think it's very simple to write
> callbacks this way.

It�s simpler to understand how new callbacks are to be defined, but it�s
harder to write them, because they have to be more aware of modbus
internals, like function codes or how data is packed. Also, without an
opaque object from the user, it makes it harder to retain a context
without resorting to a global object, which is a Bad Thing� (in your
example, i understand that data refers to the request data, and not to a
specific object).

I�ve pushed my changes on my github repo
<https://github.com/Julien-Blanc/libmodbus/tree/custom_requests_handlers>,
so feel free to comment them. They probably still have lot of bugs (unit
tests don�t all pass), and i�ll probably change a bit the interface, but
the global mechanism is in place and working nice. The default_mapping
gives a good example of what is expected to write a storage context.

�ke wrote :
>
> The thing I would have done differently is that I would have pointers
> to the data storage together with the callback-functions inside the
> modbus_map_t. This makes sure you don't mix set's of data-structures
> with callback-functions meant for other data-structures. And you don't
> need to modify the modbus_reply()-interface.

My idea is that the storage backend is completely opaque in its
structure (only a public interface should be implemented) for the
library. This means that the library cannot resort on pointers to data
storage.

>
> Also I'm not sure I like sending the backend_data_structure to each
> callback function since that adds unnecessary complexity to those
> functions. you could send a pointer to the coil-sub-data-structure if
> the single backend-object contains pointers to each of the data stores.

Here again : this would mean that the library has some knowledge about
the storage backend, which comes from the outside world. I think the
lesser knowledge on the storage context the library has, the more it is
extensible.

Regards,

Julien

Åke Forslund

unread,
Oct 26, 2013, 3:27:11 AM10/26/13
to libm...@googlegroups.com
Hi Stéphane and Julien,

It's nice to have some input from the maintainer in this wild discussion =) As the thread length starts to indicate there are a lot of decisions to make and a lot of possible uses to accommodate. Se more answers below.


Den fredagen den 25:e oktober 2013 kl. 18:40:56 UTC+2 skrev Julien Blanc:
Le 24/10/2013 20:17, St�phane Raimbault a �crit :
> Hi �ke and Julien,
>
> I'm please to see your interest on the subject and the way you try to
> achieve a clean and versatile solution.
> I think about this new feature yesterday and my idea on the subject is
> a bit different.

Hi St�phane, and first of all, thanks for having done such a good job on
the library !

>
> We could define two very simple entry points to hook all libmodbus
> functions (server side):
> bool (*read) (int function, int address, int nb, const void *data);
> bool (*write) (int function, int address, int nb, void *data);

That was in fact not far from my first idea, but Bodo�s remarks about
keeping hidden the library internals as well as modbus protocol details
is appealing.
I agree with this, it feels like the read/write functions described here would duplicate a lot of modbus_reply(). IMHO the functions defined by the users should (in a perfect world) not need to know anything other than their backend. I would like the functions to
1. Get the data from the storage/hardware/whatever
2. switch the data from the system endianness to the modbus endianness
3. deliver data to modbus


[snipped the details]
> With this solution, the current mapping keeps a central position, and
> a copy to mb_mapping is required but I think it's very simple to write
> callbacks this way.

It�s simpler to understand how new callbacks are to be defined, but it�s
harder to write them, because they have to be more aware of modbus
internals, like function codes or how data is packed. Also, without an
opaque object from the user, it makes it harder to retain a context
without resorting to a global object, which is a Bad Thing� (in your
example, i understand that data refers to the request data, and not to a
specific object).

I�ve pushed my changes on my github repo
<https://github.com/Julien-Blanc/libmodbus/tree/custom_requests_handlers>,
so feel free to comment them. They probably still have lot of bugs (unit
tests don�t all pass), and i�ll probably change a bit the interface, but
the global mechanism is in place and working nice. The default_mapping
gives a good example of what is expected to write a storage context.
I'll see if I can pull your branch and go through the code.

�ke wrote :
>
> The thing I would have done differently is that I would have pointers
> to the data storage together with the callback-functions inside the
> modbus_map_t. This makes sure you don't mix set's of data-structures
> with callback-functions meant for other data-structures. And you don't
> need to modify the modbus_reply()-interface.

My idea is that the storage backend is completely opaque in its
structure (only a public interface should be implemented) for the
library. This means that the library cannot resort on pointers to data
storage.

>
> Also I'm not sure I like sending the backend_data_structure to each
> callback function since that adds unnecessary complexity to those
> functions. you could send a pointer to the coil-sub-data-structure if
> the single backend-object contains pointers to each of the data stores.

Here again : this would mean that the library has some knowledge about
the storage backend, which comes from the outside world. I think the
lesser knowledge on the storage context the library has, the more it is
extensible.
I think I understand and I've realised that this will be a much simpler solution than the one I was thinking of. The problem I see with this solution is that the modbus_mapping_t doesn't do much of mapping since all it will do is contain the function pointers. Or am I mistaken on this?

I'm looking forward to trying out your code

Regards
/Åke

Regards,

Julien

Åke Forslund

unread,
Oct 26, 2013, 4:28:04 AM10/26/13
to libm...@googlegroups.com
When looking at the code it all became a bit clearer =)

Bodo Meissner

unread,
Oct 26, 2013, 4:56:29 AM10/26/13
to libm...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26.10.2013 09:27, �ke Forslund wrote:
>
> Den fredagen den 25:e oktober 2013 kl. 18:40:56 UTC+2 skrev Julien Blanc:
>
> That was in fact not far from my first idea, but Bodo�s remarks about keeping hidden the library internals as well as modbus protocol details is appealing.
>
> I agree with this, it feels like the read/write functions described here would duplicate a lot of modbus_reply(). IMHO the functions defined by the users should (in a perfect world) not need to know anything other than their backend. I would like the functions to 1. Get the data from the storage/hardware/whatever 2. switch the data from the system endianness to the modbus endianness 3. deliver data to modbus

4. do some error checking if necessary, for example address range checking.

This was also my intention when I implemented my version.
See functions modbus_default_read and modbus_default_write in https://github.com/bomm/libmodbus/blob/callback1/src/modbus.c.
The main arguments to the function are function code, address, number of elements and the buffer address and size where to put or get the data. In my version the library simply passes the the data area in the request or reply buffer.
The context and mapping arguments are mainly for passing to other internal functions.
Today I would leave the function code switch in the main library code to keep the callback functiuons simpler. Anyway I would pass the function code to the callback function because I might want to do nearly the same for different fucntion codes, e.g. read input registers or read holding registers.

> �ke wrote :
>>
>> The thing I would have done differently is that I would have pointers to the data storage together with the callback-functions inside the modbus_map_t. This makes sure you don't mix set's of data-structures with callback-functions meant for other data-structures. And you don't need to modify the modbus_reply()-interface.
>
> My idea is that the storage backend is completely opaque in its structure (only a public interface should be implemented) for the library. This means that the library cannot resort on pointers to data storage.

I think it is a good idea to somehow pass a pointer to an opaque data structure (void*) to the callback functions. This pointer can be used by the callback fucntions if necessary.

>> Also I'm not sure I like sending the backend_data_structure to each callback function since that adds unnecessary complexity to those functions. you could send a pointer to the coil-sub-data-structure if the single backend-object contains pointers to each of the data stores.
>
> Here again : this would mean that the library has some knowledge about the storage backend, which comes from the outside world. I think the lesser knowledge on the storage context the library has, the more it is extensible.
>
> I think I understand and I've realised that this will be a much simpler solution than the one I was thinking of. The problem I see with this solution is that the modbus_mapping_t doesn't do much of mapping since all it will do is contain the function pointers. Or am I mistaken on this?

If you have a separate memory area, the actual mapping in the "mapping structure" is not used. In my application I created the mapping with zero-size data arrays and registered the callback functions.
It depends on the callback functions whether the mapping is used or not.


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

iEYEARECAAYFAlJrg70ACgkQnMz9fgzDSqf9dACfYbPYDBD0Dp4GPkz0KmTLDwJ0
Iz8An1E2F5+P+dNoSqV7z0vKQBSAIXOi
=QaCT
-----END PGP SIGNATURE-----

Åke Forslund

unread,
Oct 26, 2013, 6:11:01 AM10/26/13
to libm...@googlegroups.com
In Julien's solution (as far as I can understand) the mapping is replaced by a user created struct (in the default case this is the mapping structure from earlier with function table) . The struct that is created must have a table of functions at the top. Doing this the user creates an own struct with any variables needed and in the reply function the user mapping is typecast to a base struct with only the function-table, this makes the functions callable but modbus_reply doesn't have any information about the data in the struct.

So basically my complaint wasn't applicable at all and was based on my own lack of information.

It's a neat solution, I personally feel a bit uneasy about typecasting left and right like this but I can't see any reason why it shouldn't work.

My intention is to merge juliens changes into my pull request along with a change of the functions to return modbus error codes instead of just 0 or -1 and (I think I had something more on my todo list). Then I'll rebase it so it has a more sane commit history and Stéphane will have to decide what he thinks of the pull request and as his idea is quite different it might not get included at all and I'll accept his decision even if might not agree =)

Regards
/Åke


Den lördagen den 26:e oktober 2013 kl. 10:56:29 UTC+2 skrev Bodo Meissner:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26.10.2013 09:27, �ke Forslund wrote:
>
> Den fredagen den 25:e oktober 2013 kl. 18:40:56 UTC+2 skrev Julien Blanc:
>
> That was in fact not far from my first idea, but Bodo�s remarks about keeping hidden the library internals as well as modbus protocol details is appealing.
>
> I agree with this, it feels like the read/write functions described here would duplicate a lot of modbus_reply(). IMHO the functions defined by the users should (in a perfect world) not need to know anything other than their backend. I would like the functions to 1. Get the data from the storage/hardware/whatever 2. switch the data from the system endianness to the modbus endianness 3. deliver data to modbus

4. do some error checking if necessary, for example address range checking.

This was also my intention when I implemented my version.
See functions modbus_default_read and modbus_default_write in https://github.com/bomm/libmodbus/blob/callback1/src/modbus.c.
The main arguments to the function are function code, address, number of elements and the buffer address and size where to put or get the data. In my version the library simply passes the the data area in the request or reply buffer.
The context and mapping arguments are mainly for passing to other internal functions.
Today I would leave the function code switch in the main library code to keep the callback functiuons simpler. Anyway I would pass the function code to the callback function because I might want to do nearly the same for different fucntion codes, e.g. read input registers or read holding registers.

> �ke wrote :

Julien Blanc

unread,
Oct 28, 2013, 5:46:02 AM10/28/13
to libm...@googlegroups.com
Le 26/10/2013 12:11, �ke Forslund a �crit :
> In Julien's solution (as far as I can understand) the mapping is
> replaced by a user created struct (in the default case this is the
> mapping structure from earlier with function table) . The struct that
> is created must have a table of functions at the top. Doing this the
> user creates an own struct with any variables needed and in the reply
> function the user mapping is typecast to a base struct with only the
> function-table, this makes the functions callable but modbus_reply
> doesn't have any information about the data in the struct.
>
> So basically my complaint wasn't applicable at all and was based on my
> own lack of information.
>
> It's a neat solution, I personally feel a bit uneasy about typecasting
> left and right like this but I can't see any reason why it shouldn't work.
>
> My intention is to merge juliens changes into my pull request along
> with a change of the functions to return modbus error codes instead of
> just 0 or -1 and (I think I had something more on my todo list). Then
> I'll rebase it so it has a more sane commit history and St�phane will
> have to decide what he thinks of the pull request and as his idea is
> quite different it might not get included at all and I'll accept his
> decision even if might not agree =)

Thanks for taking in charge this part of the work. Also, before or after
it gets merged, some documentation should be written/updated.

I fixed a few bugs so now all unit tests pass OK.

As a side note, i�ve included a � version � field in the structure
within the function pointer table. Currently it is not used and must be
1. It�s here so that we can add new functions without breaking
compatibity. When we add a new function / set of functions, we increment
the version field. And in the code, before calling a function, we check
the version field: it will tell if the structure implements the
function. If not, we keep the default behaviour. This allows us to
ensure both source and binary compatibility.

Regards,

Julien

Stéphane Raimbault

unread,
Oct 28, 2013, 6:18:13 AM10/28/13
to libm...@googlegroups.com

2013/10/25 Julien Blanc <julien...@nmc-company.fr>
With this solution, the current mapping keeps a central position, and a copy to mb_mapping is required but I think it's very simple to write callbacks this way.

It’s simpler to understand how new callbacks are to be defined, but it’s harder to write them, because they have to be more aware of modbus internals, like function codes or how data is packed. Also, without an opaque object from the user, it makes it harder to retain a context without resorting to a global object, which is a Bad Thing™ (in your example, i understand that data refers to the request data, and not to a specific object).


No libmodbus internals are exposed in my solution!
It seems my presentation wasn't crystal clear...

Åke Forslund

unread,
Oct 31, 2013, 3:08:12 AM10/31/13
to libm...@googlegroups.com

Hi Stephane,

the Pull-request has been updated and squashed into some sort of pullable shape maybe you can look through it and comment on what you would like to change? I personally like the interface Julien has implemented as it is already useful to me. If you want to commit it we'll produce some documentation and an exampel (and maybe a template) of how to use the new feature.

Best regards
/Åke
Reply all
Reply to author
Forward
0 new messages