Skip to content

Conversation

@chintal
Copy link
Contributor

@chintal chintal commented Apr 26, 2018

This PR corrects the Modbus Device information object_ids and category boundaries to match with the MODBUS Application Protocol Specification (V1.1b3). This change will cause changes in behavior of existing code, but make it compatible with the specification. A middle ground may be considered which eliminates the intermediate 'reserved' section, making it possible for anyone using the current pymodbus private range of 0x08-0x80 to continue to do so.

In any case, the extant implementation breaks compatibility with fully compliant Modbus clients, including pymodbus itself. These problems revealed themselves during the development of tests for PR #293, and will have to be fixed before #293 will have passing tests.

The following are the major elements of the specs the current code is in violation of :

  • The read device identification function provides a start object_id, which the spec recommends the client start reading at zero - though it need not be zero. In fact, all subsequent calls made necessary by long information strings and limitations of the ADU size will require non-zero values for the start object_id. It should then return all object IDs after it, stopping at the category boundary defined by the read type.
  • The range 0x07 - 0x7F is reserved, not private, and should ideally not be used at all.
  • The range 0x80 - 0xFF is private, not out of bounds, and the content is device / server dependent. In this implementation we do make the additional assumption that any object_id with an empty string is a non-existing / invalid object_id.

@chintal
Copy link
Contributor Author

chintal commented Apr 26, 2018

The proposed implementation still has one break from compliance with the spec.

If the Object Id does not match any known object, the server responds as if object 0 were
pointed out (restart at the beginning).

The changes checked in in the first commit will instead return any objects with id greater than the provided one, instead of starting again from zero. The following commit fixes this problem, but does so in a horribly ugly way. A cleaner solution would probably be preferable, but I can't seem to think of a good way to do it without major restructuring of the DeviceInformationFactory class.

…n an invalid object_id is provided by an external client.
@dhoomakethu
Copy link
Contributor

@chintal I will keep #293 and #295 for up coming release and will be releasing 1.5.0 with out these patches.

@chintal
Copy link
Contributor Author

chintal commented May 1, 2018

@dhoomakethu OK. There's no hurry as such, I currently use pymodbus through an overlay which fixes many of these issues. For the record, though, I see #295 as ready and necessary to merge into pymodbus for compliance with the modbus spec.

@dhoomakethu
Copy link
Contributor

Thanks, will review this PR once I am done going through the specs to better understand these changes.

@dhoomakethu
Copy link
Contributor

@chintal wrt your latest commit #8fbb847 to treat reserved address as private , I am not comfortable with that change and would like the reserved space to be treated as reserved (no writes/no reads) as per the specs. Could you please revert that change ? Also for the cases that requires segmentation (i.e multiple requests to get the complete device information) , I was thinking to have a dedicated request (ReadDeviceInformationRequest_Ext) which would handle sending multiple requests and processing the response.

@chintal
Copy link
Contributor Author

chintal commented May 2, 2018

@dhoomakethu I can revert the change. However, existing users of pymodbus might have utilized addresses as per the pymodbus implementation. This would be contrary to the specification, but it would have more or less worked so far, as long as they have simply been using a ReadExtended based on 0x00 or a ReadSpecific command, which would not have been effected by this bug. Assuming this is acceptable, rolling back the last commit will make it fully spec-compliant, and I will do so.

I was looking at the multiple request problem as well. I don't think a dedicated request is the way to go, given that there is no way for a Modbus command to be differentiated between the standard request and one that results in the need for multiple requests. The reply encoding implementation has instead been restructured in #293 to allow for truncating the response if needed. The reason I did not go all the way with it, however, was that there seemed to be no obvious way for the Response object to get information of the ADU Max Length applicable to the current request / transport. If that information is available in some way from within the response, the remaining portion is fairly trivial to implement.

@dhoomakethu
Copy link
Contributor

I am fairly certain that most of the users implementing slaves using pymodbus server do not use extended device identification (0x80-0xFF) or something in the reserved area of the regular device identification range (0x03-0x7F). Even if it is breaking for few users, they could easily fix just by updating the dict they are passing to create DeviceIdentification object in their respective implementation.
Regarding, mutilple requests, again thats something I haven't encountered so far and I am not aware of any device giving out the information in multiple responses, but I think we can implement something to handle this scenario using More Follows, Next Object Id , Number Of Objects fields. This is again an assumption and I have not spent time to really think of the corner cases yet. But if you think that is not the route to take up , I will be happier to accept this PR and keep that as a problem for the future to deal with.

@chintal chintal force-pushed the fix-mei-deviceinfo-selectors branch from 8fbb847 to 1a1e1da Compare May 2, 2018 08:48
@chintal
Copy link
Contributor Author

chintal commented May 2, 2018

@dhoomakethu I have reverted the commit.

Multiple requests for device information are required when a device / server wants to return more / longer objects than fit within a single ADU, which in the case of RTU is 254 bytes (including header etc.). Supporting it on the server side is required because it is the server which decides how many objects it can fit into a response. I will attempt to fix this problem with #293 where much of the required restructuring is already done. I do, however, require information from within the Response's encode() function about the ADU maximum length, which I'm not sure is available.

Given the relative infrequency of the use of Modbus Device Information, it may not be needed to specially implement handling this problem on the Client side, and leave it to end users to deal with it since they can fairly easily. Once the problem is dealt with in the server, it may be addresses in the client as well. For the moment, I might indicate https://github.com/chintal/ebs-lib-ucdm/blob/c0716b9a1bf0dc074e62ffdfffc920f8b1e8ae58/python/ebs/ucdm/descriptor.py#L74 as a typical solution to the problem.

@dhoomakethu dhoomakethu merged commit d159a44 into pymodbus-dev:dev May 2, 2018
@chintal chintal deleted the fix-mei-deviceinfo-selectors branch May 2, 2018 10:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants