Add modbus_reply_callback() and modbus_set_reply_callback() #408

pboettch · 2017-10-31T10:14:21Z

Based on @frodete's proposal (#319) this implementation adds a a callback-based reply-functionality.

The old mapping-implementation is now based on this version. As a nice side-effect some DRY code optimizations could have been done.

Unit tests are OK (locally, not on travis though). And my RTU-multi-slave server works.

EDIT:
My motivation for needing a callback-based reply is that I need to implement a modbus-tcp-server and a modbus-rtu-slave. Both will handle the data within a cache. The rtu-slave implementation has to be able to handle multiple slaves. On one RS485-port multiple slave-id will be handled.

More details to what I have done: I took the modbus_reply-function and changed its name to modbus_reply_callback. Then, each time where something was checked against the start_bits/start_registers -adresses and tab_*-counts I replaced it with a call to the callback verify. And each time the payload-part of the request or response-buffer was accesses I replaced it with a call to the callback read or write respectively.

Then I implemented the previous modbus_reply-function in a way that it uses the modbus_reply_callback function by providing callbacks for verify, read and write. In these callback-functions I put the code which previously was in charge to fill in the buffer or to verify the boundaries. By doing so, some code-duplicates have been removed, especially accesses to the buffers in the mapping and its verification.

Also adds a MODBUS_SLAVE_ACCEPT_ALL "address", which can be used with modbus_set_slave(). This makes the modbus_receive()-function accept all requests, independently of the slave-address found in the decoded-request. Useful for multi-slave-implementation using libmodbus.

I updated the man-pages and added one for modbus_reply_callback(), please review and comment and merge

pboettch · 2017-11-01T08:16:58Z

Not sure why travis is failing, it works locally.

mhei · 2017-11-02T19:00:40Z

doc/modbus_reply_callback.txt

+RTU-slave. In its implementation the user has to check whether the slave-id, _slave_, which was
+decoded from the request, should be answered to or not and returning TRUE if so, otherwise FALSE.
+Returning FALSE will make *modbus_reply_callback()* exit immedialty and return 0.
+


Why is accept_rtu_slave only required for RTU? The address field is also available in TCP requests. I don't see why this should be handled differently?

But is it used in TCP? To address different devices via one connection? I don't see it being explained in the spec (while flying over it).

If I remember correctly, it is used e.g. in SMA inverters to have different register mappings available. And think about a TCP to RTU modbus proxy where you want to have one address for the gateway itself and the others mapped to the UART...

OK, the slave-field in TCP is useful.

The reason for not needing an accept_rtu_slave in a TCP-connection is, that you probably don't want to ignore a request based on the slave-ID when implementing a TCP-server. A TCP-connection is point-to-point whereas a RTU-bus is broadcast, a slave on a RS482-bus sees all the requests coming by, even the ones it doesn't want to handle, so you need to filter - not answering those you don't care for - someone else will do. In TCP no one else will handle the request.

Is that logic correct?

Absolutely correct. 👍

(but you can still handle it optionally if you want yes? For people making rtu drop in replacements, and that sort of thing...)

mhei · 2017-11-02T19:04:03Z

doc/modbus_reply_callback.txt

+MODBUS_WRITE-function-requests are passed to the write-callback. When encountering the
+MODBUS_FC_WRITE_AND_READ_REGISTERS-function modbus_reply_callback is calling the write and the
+read-callback.
+


I guess a dedicated write_read callback is required here... reason below.

mhei · 2017-11-02T19:05:17Z

doc/modbus_reply_callback.txt

+MODBUS_FC_WRITE_AND_READ_REGISTERS-function modbus_reply_callback is calling the write and the
+read-callback.
+
+These functions are designed for implementing a Modbus TCP server and RTU slaves.


Wording nitpick: better write "...implementing Modbus TCP and RTU slaves." ?

mhei · 2017-11-02T19:07:27Z

doc/modbus_reply_callback.txt

+    buf[1] =  value1       & 0xff;
+    buf[2] = (value2 >> 8) & 0xff;
+    buf[3] =  value2       & 0xff;
+


Can we just use the helper macros here?

If they are part of the API, yes. I personally prefer to tell the dirty truth and leave it to the user.

mhei · 2017-11-02T19:09:26Z

src/modbus.c

+            }
+        }
+
+		// TODO broadcast responses should use the slave-id, probably


Broadcasts simply do not have a response by definition.

Thanks for helping me with this question.

however, it is a quirk that many devices use, and has been porposed as one of the "quirks_modes" that libmodbus should support using.... (sorry for the necro)

mhei · 2017-11-02T19:26:18Z

src/modbus.h

+                                     int req_length);

 /**
 * UTILS FUNCTIONS


Some general notes:

this PR is very... big 😄 ... I'm not sure that I already understood all parts

When implementing a Modbus server, you usually want to do something with the data, i.e. the Modbus server is only one part of the whole application. The servers which I have written were multi-threaded, so you usually need some locking when accessing shared data. I think this can already prepared by adding lock/unlock callbacks.

I'm wondering whether extending the modbus context makes sense. As already stated in Added messagetype 0x14 and 0x15 for file-access. #407, I could image a second "layer" on top of the existing "ground or base layer" functions which cares about such higher-level functionality.

Yes it's big, I think to make it easier to review I will split out the reply code in a separate file. I think it is not possible to further split the code in several commits. It's complex.

I agree with the fact that you need locking around the data, however I disagree that libmodbus has to take care of this within its API. If you need locking, just lock the modbus_reply_callback()-call in your code and access to the data is atomic.

Is the current reply-based-on-mapping-code used in anything else than examples and tests? When thinking about servers and slaves I was very quickly limited by the mapping-implementation, which made me develop this code. I my eyes, the reply-callback-code fits the library's style, no need to create extra layers.

I'm going to write/import examples in the test-dirs which will show how to write a multi-slave-server, that could help to see how to use it.

Thanks a lot for your review! 👍

Can it be master slave opperate in modbus rtu in PIC18fXXX

If libmodbus compiles and works for this platform, this callback-based server/slave should work as well.

@frodete

Based on @frodete's proposal (stephane#319) this implementation adds a a callback-based reply-functionality. The old mapping-implementation is now based on this version. As a nice side-effect some DRY code optimizations could have been done. Also adds a MODBUS_SLAVE_ACCEPT_ALL "address", which can be used with modbus_set_slave(). This makes the modbus_receive()-function accept all requests, independently of the slave-address found in the decoded-request. Useful for multi-slave-implementation using libmodbus.

martinxyz · 2019-03-27T10:32:52Z

Endorsement: using this PR in production code; I have read some of the code, it looks good to me. Maybe the interface is a bit too low-level, e.g. I don't like that I need to handle both WRITE_SINGLE_REGISTER and WRITE_MULTIPLE_REGISTERS when the callback signature actually covers both cases, but maybe this flexibility can be useful for some cases. I like the fact that it comes with docu, which seems complete and accurate, although it could profit from being translated into "simple English". I have started doing that, and will finish it if there is interest.

pboettch · 2019-03-27T11:09:29Z

Happy to hear that. Same here. Used happily in two different production environments. One of it really uses it all, especially the key-feature: multiple RTU-slaves.

Regarding WRITE_SINGLE_REGISTER and WRITE_MULTIPLE_REGISTERS distinction it is needed. Some slaves/clients might not want to handle the MULTIPLE_REGISTERS in order to be "backward"-compatible for replaced devices (my case).

Thanks for your documentation efforts. Describing all functionalities in a man-pages took me an enormous amount of time if you can simplify it, would be highly appreciated. Happy to pull that in. In my fork at least.

ben-edna · 2019-03-27T11:12:46Z

+1 for this PR as well, would be a good point adding support for custom command codes

l29ah · 2019-10-19T11:22:07Z

Why wasn't the pull request merged? Can i help it somehow?

LaurensEDnA · 2022-03-23T06:45:38Z

Has there been any more progress on this PR?

see stephane#408

mhei reviewed Nov 2, 2017

View reviewed changes

stephane self-assigned this Nov 3, 2017

pboettch force-pushed the modbus-reply-callback branch from d113379 to 029ddca Compare December 11, 2017 14:54

pboettch force-pushed the modbus-reply-callback branch from 029ddca to fdd398e Compare December 11, 2017 16:38

ntfreak added a commit to ntfreak/libmodbus that referenced this pull request Aug 23, 2024

Modbus callback support

67ed49f

see stephane#408

ntfreak added a commit to ntfreak/libmodbus that referenced this pull request Aug 23, 2024

Modbus callback support

322ea2b

see stephane#408

Uh oh!

Add modbus_reply_callback() and modbus_set_reply_callback() #408

Are you sure you want to change the base?

Add modbus_reply_callback() and modbus_set_reply_callback() #408

Uh oh!

Conversation

pboettch commented Oct 31, 2017

Uh oh!

pboettch commented Nov 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pboettch Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinxyz commented Mar 27, 2019

Uh oh!

pboettch commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ben-edna commented Mar 27, 2019

Uh oh!

l29ah commented Oct 19, 2019

Uh oh!

LaurensEDnA commented Mar 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pboettch Nov 3, 2017 •

edited

Loading

pboettch commented Mar 27, 2019 •

edited

Loading