Skip to content

Conversation

@pboettch
Copy link
Contributor

@pboettch pboettch commented Aug 7, 2017

Based on @frodete's proposal (#319) a more minimalist implementation of a callback-based reply-function.

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. And my RTU-multi-slave server starts to work.

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. 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.

@stephane stephane self-assigned this Aug 8, 2017
@stephane
Copy link
Owner

stephane commented Aug 8, 2017

It seems very interesting on first quick review 👍
I need a bit more time to check the side effects of the new verify function.

@pboettch pboettch force-pushed the modbus-reply-callback branch from 0d21b1b to bc769b3 Compare August 8, 2017 13:14
application_context *ctx = user_ctx;

/* ... */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want a better example here, ie, what is length meant to be? related, Where is "len" coming from if not the bytes? Your comment above about this being "values" not bytes makes it very hard to do generic code, without having to handle all the modbus function code internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, the doc needs some more work. Have you looked at the implementation I put into modbus.c for the mapping-reply?

Modbus internals yes, but not many. A user needs to know how to get/put bits and registers out of/into the buffer given to the read and write callback. I chose to propose it like this because it avoids allocating memory and additional copies inside libmodbus.

{
application_context *ctx = user_ctx;

/* ... */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewize, I feel some trivial, but real example would be better here than just "/.../"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the works.

@pboettch pboettch force-pushed the modbus-reply-callback branch from bc769b3 to 524fabe Compare August 11, 2017 06:50
@pboettch
Copy link
Contributor Author

Please do not merge the pull-request for the moment, I'm currently working actively on it and it is not yet ready. However, the basic idea probably won't change, feel free to comment.

@pboettch pboettch force-pushed the modbus-reply-callback branch 2 times, most recently from e76dd39 to 7e56e8f Compare August 14, 2017 06:52
@pboettch
Copy link
Contributor Author

pboettch commented Aug 14, 2017

I made more changes to reduce LOC and thus more DRY, maybe the code has become more complex. Basically the verification-part and read/write-part has been separated in two switch-cases and thus exception-replies are done once for all MODBUS_FC_* as well as value-count extraction of buffers.

When reviewing, looking at the diff for modbus.c is maybe not the best idea (anymore).

Doc is still missing.

@pboettch pboettch force-pushed the modbus-reply-callback branch 2 times, most recently from 94cd17f to 8535b71 Compare August 20, 2017 20:49
@pboettch pboettch force-pushed the modbus-reply-callback branch from 8535b71 to 3f8465e Compare October 31, 2017 08:29
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.
@pboettch pboettch force-pushed the modbus-reply-callback branch from 3f8465e to d113379 Compare October 31, 2017 08:30
@pboettch pboettch closed this Oct 31, 2017
@pboettch pboettch reopened this Oct 31, 2017
@pboettch pboettch closed this Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants