-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement function (0x2B / 0x0E) Read Device Identification. #649
base: master
Are you sure you want to change the base?
Conversation
The function is defined in section 6.21 of the Modbus Application Protocol v1.1b. There are many ways to read the objects. The one used here is code 01, which seems to be the most widely supported one. It might be necessary to make many requests in order to receive all the data. This is all handled internally.
We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient... |
Shall I fill in the CLA before this gets reviewed? |
IMO this is too narrow support, you've hardcoded to function 1 because you believe it's more widely used. I'd like to see better underlaying support for the primitives, so that they can be composed by users, rather than just a single "does everything, but only via this single style of operation" function. Do you have hardware that implements stream access to basic, but not specific object access? I'm also not entirely sure that the nicest API is to continue re-reading as long as the server keeps indicating that it has more data, you're can block ~indefinitely now, if the server just keeps spewing data. And yes, you should absolutely fill in CLA's before anyone with any authority looks at it. Why should anyone spend time reviewing code if someone then refuses to sign the CLA? |
@karlp First, WRT to the CLA, it is not problem to sign it, I just did not want to bother my boss before I knew there was interest in merging that. I will get you the CLA ASAP. I have a Siemens PAC2200 electric meter which only supports mode 01, here is the response:
The relevant part is <2B><0E><01><01> The modbus specification seems to indicate that there is a sort of "hierarchy" of modes:
In any case, the structure of the response is the same for all mode, so it would be quite easy to add a parameter to the function to select the mode. You are right that a buggy device could cause an infinite loop and that is bad. It could be solved by exiting the loop if zero objects are returned, but I think it is better to just remove the loop. What do you think about the following prototype? int modbus_read_device_id(modbus_t *ctx, int access_type, int object_id, int max_objects,
uint8_t *obj_values[], int obj_lengths[], int *more_follows); The internal loop would be eliminated, and if there is more data, the "more follows" value would be returned. Should I add an additional output parameter to retrieve the "conformity level" (i.e. which read modes are supported)? Another option would be to have two functions, one for stream access, and another one for individual access. The "more_follows" parameter is not required for the latter and the obj_values and obj_lengths don't have to be arrays there. |
Also, retrieve the internal parameters "more follows", "next object" and the conformity level. The value of the next object is clipped, otherwise, the users would have to be very careful when writing loops since a buggy device could send them into an infinite loop.
We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient... |
I signed the CLA and also made changes to the code:
I'm still not sure whether the "parallel arrays" interface is a good thing or wether the function should take in an array of structures of the form: struct object {
int id;
int obj_size;
int buffer_size;
uint8_t *buffer;
}; or maybe instead of |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
Hi, any update on this? Is there anything that needs to be changed? AFAIU there is no way to implement this using |
Hello, has this been tested on a RTU device? |
@DaAwesomeP yes, it is tested with RTU and TCP, but with "basic" support level, though that should not be an issue because the response format is the same in all cases. #261 is about the server side support.
so I changed the implementation to not handle "more-follows", "next-id" automatically. This resulted in an interface that requires the user too loop to get all results. I'd like to make a few changes:
|
@DaAwesomeP , @karlp I'd very much like to have this feature merged in the official repo, instead of keeping it as a private patch. I'm 100% willing to make any enhancements or modifications that the maintainers request. What is blocking this feature? Is it lack of interest, or lack of hardware to test? |
I'm working on a project where this would be an extremely useful addition. When is this going to get merged? |
@stephane what are we missing here? |
@stephane Indeed I'd like to see this happen too! Can I help test anything? |
Description
This implements the function (0x2B / 0x0E) as defined in section 6.21 of the Modbus Application Protocol v1.1b.
I'm aware of PR #443 , but that one uses the method "04"
which will be less supported than "01" which is used here- this one supports all four methods.Testing
I have tested this on a Modbus/TCP device. I will test on a RTU device on the coming days. I'm not sure how the testing works in general for this library.