libmodbus icon indicating copy to clipboard operation
libmodbus copied to clipboard

Add modbus_reply_callback() and modbus_set_reply_callback()

Open pboettch opened this issue 7 years ago • 6 comments

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 avatar Oct 31 '17 10:10 pboettch

Not sure why travis is failing, it works locally.

pboettch avatar Nov 01 '17 08:11 pboettch

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.

martinxyz avatar Mar 27 '19 10:03 martinxyz

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.

pboettch avatar Mar 27 '19 11:03 pboettch

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

ben-edna avatar Mar 27 '19 11:03 ben-edna

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

l29ah avatar Oct 19 '19 11:10 l29ah

Has there been any more progress on this PR?

LaurensEDnA avatar Mar 23 '22 06:03 LaurensEDnA