Skip to content
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

Feature/extended register read #174

Merged
merged 5 commits into from
Aug 9, 2020

Conversation

hutcheb
Copy link
Contributor

@hutcheb hutcheb commented Jul 27, 2020

Add extended register/file record read only support, function code 20.

This allows you to query data within the 600000 memory range.

The 600000 memory range starts at file 1 address 0 (600000). Each file has a length of 10000 so 610000 would be the first address in file 2.

Copy link
Contributor

@chrisdutz chrisdutz left a comment

Choose a reason for hiding this comment

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

Could you please elaborate what these "extended-registers" are? I implemented the driver according to the Modbus spec and that didn't contain this form of address. But in general I'm very happy with your's PR as it showed that you nicely understood all the stuff I built, but didn't have the time do document :-)

@@ -247,9 +247,9 @@
]

[type 'ModbusPDUReadFileRecordResponseItem'
[implicit uint 8 'dataLength' '(COUNT(data) * 2) + 1']
[simple uint 8 'dataLength']
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 in this case the field could remain an implicit ... just change the syntax to "(COUNT(data) + 1)" cause this way you don't have to manually manage it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Chris,

For this response you can have multiple sub responses/response items within the one response. In the overall PDU/response header there is only a total length and function code, there isn't any details about the length of each sub response or the number of sub responses.
I don't see a way of determining the number of sub responses without using the length from each sub response.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if there is no 100% link between the dataLength and the length of the data ... we will be having trouble during the parsing of the messages.

Do I understand this correctly, that if in the "data" part there are 3 short items then you want to set this to 3 instead of 6? If the problem is that you need to know the request in order to decode the response, you can treat the response as a raw byte-array payload and split things up in the driver. For S7 I don't know how to decode the response unless I know the request ... same with the KNX protocol, where you even need to pull in some external data structures in order to know how to decode the payload ... It's not that uncommon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Chris,

The response does include the length of the sub-response within each sub-response, but this length can't be implied without using the other information such as from the request. Changing it to simple uses the length returned within the response.

The length included in the sub-response is in bytes (not including the datalength byte), so if three short items are being returned the length would be 6 + 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Woudln't this exactly match my proposed: "(COUNT(data) + 1)"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

sorry for keeping on asking ... but I still don't seem to grasp the problem ... so translating this to objects from the mspec. In your case you would have:

0x14 -> error = false & function = 0x14

  • ModbusPDUReadFileRecordResponse
    0x08 -> byteCount (Used to know how many ModbusPDUReadFileRecordResponseItems have to be parsed)
    • ModbusPDUReadFileRecordResponseItem[0]
      0x03 -> dataLength (used to know the size of the byte array, which is 3 - 1 = 2)
      0x06 -> referenceTypte = 6
      0x00 -> recordData[0]
      0x01 -> recordData[1]
    • ModbusPDUReadFileRecordResponseItem[1]
      0x03 -> dataLength (used to know the size of the byte array, which is 3 - 1 = 2)
      0x06 -> referenceTypte = 6
      0x00 -> recordData[0]
      0x01 -> recordData[1]

I can't see the problem here ... the byteCount of ModbusPDUReadFileRecordResponse matches the number of bytes consumed by all ModbusPDUReadFileRecordResponseItems together and the dataLength in each ModbusPDUReadFileRecordResponseItem matches the length of the bytes in the data array + the one byte for the "referenceType".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Chris,

Maybe I'm confused by what implicit and simple mean. I take them to mean:-
Simple - Use the value that is included in the response.
Implicit - Attempt to calculated the value based on the other fields in the response.

As the length of the ModbusPDUReadFileRecordResponseItems is included in the response I changed it to simple instead of trying to calculate it from the data field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also as the data field has a variable length if you try to infer the length of it, it wouldn't know the length of the data field in the first ModbusPDUReadFileRecordResponseItem.

So when it starts parsing the first ModbusPDUReadFileRecordResponseItem it would receive:-
0x03 - It would then try to infer this field by using the rest of the message. It would end up with 7 instead of 3.
0x06
0x00
0x01
0x03
0x06
0x00
0x01

Copy link
Contributor

Choose a reason for hiding this comment

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

No ... it would read the 0x03 as length (When parsing implicits are parsed just like simple fields, however they are not stored in a property as this information can be calculated by the given expression when serializing) ... so when parsing the 0x03 would be parsed and would be available to other parsing expressions as dataLength (which would be used to know how many elements of "data" have to be read). When serializing instead of reading a property of the object, it would check how many elements the "data" array has and add 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops sorry about that, I shouldn't assume things.

Your correction works.

@hutcheb
Copy link
Contributor Author

hutcheb commented Jul 29, 2020

Hi Chris,

The extended registers/memory is another area of memory that can be referenced using the File Record Read function code 20. This memory area is split into files 0x1 thru 0xFFFF, with each file containing 10000 registers 0 thru 9999.
(In the modbus spec it mentions that while you can go above 10dec files interoperability with older equipment could be compromised, assuming they are talking about the Modicon Modbus protocol in which 984 controllers had file record up to 10.)

For a client I see no reason to implement up to 0xFFFF file records as the server should respond with an error code if we go out of bounds. (I can fix this, currently we can only query up to 99999 registers.)

For each request you can query multiple non-contiguous areas using sub requests, as long as the total length of the response is < 253 bytes. For our implementation I have only used a second sub request if the user queries registers that span a file record e.g. 609998[10]. This keeps it simpler and in line with the format used by the other register areas.

The Modicon Modbus protocol document introduces the terms extended registers and the addressing format using 6x notation. It also references allows the user to reference the data area as contiguous memory 600000 thru 698303.

When I started I thought these were used more than they seem to be. Some old 984 controllers use them but it seems they have been removed from newer Schneider controllers. I've found other references to equipment that use these.

I'll add some documentation to the user Modbus page.

@chrisdutz
Copy link
Contributor

Thanks for the feedback ... and absolutely possible I didn't entirely read an implement all of the parts of the spec ... going through so many specs it's difficult to read and process all of them. Therefore I am extremely happy you took on adding stuff I missed ... it's really highly appreciated. As soon as we figure out that the mspec change doesn't have a negative effect, I'm happy to merge this.

@hutcheb
Copy link
Contributor Author

hutcheb commented Jul 29, 2020

Let me add some documentation.

On a side note within the ModbusTestSuite, in the response cases in the data area. What is the encoding used for that? I can't work it out.

@chrisdutz
Copy link
Contributor

The encoding Jackson seems to use here is Base64 ... please feel free to encode/decode this using something like this: https://www.base64decode.org/

Add additional documentation in the user/modbus protocol to include a 
short description about the memory area.
Changed the type of field for the datalength field in 
ModbusPDUReadFileRecordResponseItem back to implicit.
@@ -249,7 +249,7 @@
[type 'ModbusPDUReadFileRecordResponseItem'
[implicit uint 8 'dataLength' '(COUNT(data) * 2) + 1']
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 this needs to be adjusted to "COUNT(data) + 1" as the datatype changed from short to byte

Fixed datalength calc as the data array type was changed to int8 instead 
of int16

Remove the dataLength property from ModbusPDUReadFileRecordResponseItem 
in the test case.
@hutcheb
Copy link
Contributor Author

hutcheb commented Aug 3, 2020

Yes you are right, I have changed it.

I know I didn't run the test cases before pushing the commit the other day, I did run them afterwards but must of run them on the develop branch instead.

With the change from simple to implicit does this remove the ability to include that property in the test case? I had to remove the dataLength property in the test case for the test to pass.

@chrisdutz
Copy link
Contributor

Yeah ... now things look great ... thanks :-)

@chrisdutz chrisdutz merged commit 02db626 into apache:develop Aug 9, 2020
@hutcheb hutcheb deleted the feature/extended_register_read branch November 12, 2020 11:01
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.

None yet

2 participants