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 .
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
-----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,
-----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,
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
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 .
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
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
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.
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
-----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 :
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).