jcarrano commented 1 year ago

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.

cla-bot[bot] commented 1 year ago

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

jcarrano commented 1 year ago

Shall I fill in the CLA before this gets reviewed?

karlp commented 1 year ago

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?

jcarrano commented 1 year ago

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

cla-bot[bot] commented 1 year ago

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

jcarrano commented 1 year ago

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

jcarrano commented 1 year ago

@cla-bot check

cla-bot[bot] commented 1 year ago

The cla-bot has been summoned, and re-checked this pull request!

jcarrano commented 1 year ago

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.

DaAwesomeP commented 1 year ago

Hello, has this been tested on a RTU device?

DaAwesomeP commented 1 year ago

Similar to #474, #261. Seems like another feature with lots of open pulls but no merges.

jcarrano commented 1 year ago

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

jcarrano commented 1 year ago

@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?

jovere commented 1 year ago

I'm working on a project where this would be an extremely useful addition. When is this going to get merged?

DaAwesomeP commented 1 year ago

@stephane what are we missing here?

kb3ien commented 12 months ago

@stephane Indeed I'd like to see this happen too! Can I help test anything?

stephane / libmodbus

Implement function (0x2B / 0x0E) Read Device Identification. #649

Description

Testing

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: