libmodbus
libmodbus copied to clipboard
stephane
Implement function (0x2B / 0x0E) Read Device Identification.
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.
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...
![cla-bot[bot] avatar](https://avatars.githubusercontent.com/in/2404?v=4 "Published by a pub.dev verified publisher"){kind=small}
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:
./tests/.libs/dev-id-test-client 0 50 50 50
Connecting to 127.0.0.1:1502
[00][00][00][00][00][05][FF][2B][0E][01][00]
Waiting for a confirmation...
<00><00><00><00><00><49><FF><2B><0E><01><01><00><00><03><00><0A><53><69><65><6D><65><6E><73><20><41><47><01><07><50><41><43><32><32><30><30><02><2A><46><57><20><56><33><2E><30><2E><31><31><2E><30><5F><31><2E><31><2E><30><2E><31><20><2F><20><42><4C><20><56><33><2E><30><2E><30><2E><30><5F><31><2E><31><2E><30><2E><31>
mf: 0, oid: 0, to_add: 3
Requested: 3, Read: 3
Obj ID Length Trunc? Value
00 10 Siemens AG [53][69][65][6D][65][6E][73][20][41][47]
01 7 PAC2200 [50][41][43][32][32][30][30]
02 42 FW V3.0.11.0_1.1.0.1 / BL V3.0.0.0_1.1.0.1 [46][57][20][56][33][2E][30][2E][31][31][2E][30][5F][31][2E][31][2E][30][2E][31][20][2F][20][42][4C][20][56][33][2E][30][2E][30][2E][30][5F][31][2E][31][2E][30][2E][31]
The relevant part is <2B><0E><01><01>
The modbus specification seems to indicate that there is a sort of "hierarchy" of modes:
If the server device is asked for a description level ( readDevice Code )higher that its conformity level , It must respond in accordance with its actual conformity level.
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.
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:
- All read methods are supported
- The user is not responsible for looping in order to retrieve all the values
- The conformity level is reported back to the user
- A few cleanups.
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 buffer_size call it actual_size and have the library place the minimum there (i.e. to spare the user from having to compute that again).
Hi, any update on this? Is there anything that needs to be changed?
AFAIU there is no way to implement this using send_raw_request and receive_confirmation because of the way that the packet length is computed. Also, I have devices that only expose some data through the Object address space so that means that the data is not accessible using "vanilla" libmodbus.
Similar to #474, #261. Seems like another feature with lots of open pulls but no merges.
@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. #474 is somehow similar. Interestingly, one of the criticisms was that the client had to handle "more-follows", "next-id" etc, while in this PR, the opposite issue was raised:
I'd like to see better underlaying support for the primitives, so that they can be composed by users
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:
- Change the interface to use an array of structs instead of parralel arrays for ids, object contents and lenghts
- Do some of the min() computations inside the function so that the user's code does not have to repeat itself over and over again.
- Have stronger guarantees on the returned values so that the check at https://github.com/stephane/libmodbus/pull/649/files#diff-2b85fedeaca59ceede0671e3bc1793b10bf292d65bc387b1df41461f2ea302dfR41 can be simplified.
@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?
