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

"Modbus plus" #352

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

"Modbus plus" #352

wants to merge 3 commits into from

Conversation

splatch
Copy link
Contributor

@splatch splatch commented Apr 19, 2022

This pull request solves few errors I spotted while trying to use plc4j modbus stuff.
First of all, it adds support for String outputs from modbus requests, so holding:3000:STRING[30] will bring back up to 15 character text which wasn't covered properly by codegen till now. It works with vstring stuff Ben added a while ago.
Then there is second part which is actual modbus plus stuff. I provided sample request/responses I acquired from inverter, so its possible to see actual packets and payload mapping. For now I did stick with structs as identification answer is nested structure with variable list of elements.

Reorganization of modbus fields, so identification request might be a Struct.
Additionally, fixes for unbalanced stack operations in PlcStruct.

Signed-off-by: Łukasz Dywicki <[email protected]>
@splatch
Copy link
Contributor Author

splatch commented Apr 20, 2022

Seems C templates needs to be updated as well to handle string fields. Sadly I can't handle that part.

@chrisdutz
Copy link
Contributor

chrisdutz commented Apr 20, 2022

Ther test.mspec contains vstring fields ... could you please elaborate on what is actually not working?
In this case the test.mspec should be extended with an example that doesn't work. Then I'll take care of fixing this.

@splatch
Copy link
Contributor Author

splatch commented Apr 20, 2022

Ther test.mspec contains vstring fields ... could you please elaborate on what is actually not working? In this case the test.mspec should be extended with an example that doesn't work. Then I'll take care of fixing this.

I think it might be related to use of vstring inside data-io:

        ['STRING' List
            [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
        ]
        ['WSTRING' List
            [simple vstring '16 * numberOfValues' value encoding='"UTF-16"']
        ]

For Java problem was that case statement was catching this as List, so there is a need to verify plain type read from buffer. If its String then, output is not an array but, just an string.

@chrisdutz
Copy link
Contributor

Your mspec fragment looks a bit fishy in general ... the type you define as "List" and not as "String" and where does the "numberOfValues" come from?

@splatch
Copy link
Contributor Author

splatch commented Apr 20, 2022

Your mspec fragment looks a bit fishy in general ... the type you define as "List" and not as "String" and where does the "numberOfValues" come from?

Its being called when you declare field data type as STRING[10].

@chrisdutz
Copy link
Contributor

"List" would require a field of type array labeled "value" .... So I think this should be "STRING" instead ... but the variable of the length I can't see where it's coming from.

@splatch
Copy link
Contributor Author

splatch commented Apr 20, 2022

I get your point as it is tricky. For me with high level language it makes sense to avoid construction of a String[] {"a", "b", "c", "d" } and return directly a String "abcd" to the caller.

@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
['STRING','1' STRING
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok ... so in this case the numberOfValues is 1 ... and your string consists of only one utf-8 char ... for this case I would use "string 8" instead of this. But I think you need to somehow know how long the string should be ... the numberOfValues shouldn't be the lenght of the string, but the number of strings ... usually I have one internal field that contains the string lenght and then I use that in the vstring.

[simple vstring '8' value encoding='"UTF-8"']
]
['STRING' List
[simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are creating only one single element field of type string that you want to put in a list ... I think you should instead split this up into two cases: numberOfValues = 1, where the type is STRING and one where it's not == 1 where the type is List and the value field then needs to be an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make much sense, cause output should be a String anyway. What's the point of returning a text in chunks to the caller if it can be merged and read at once beforehand?

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 you are mis-understanding the concept here ...
A field of type "string" or "vstring" can contain a string of multiple characters. But with "STRING[10]" we are not reading one String with a length of 10 characters, but 10 strings of individual length (In S7 this would be 255 chars long) ... if you want to explicitly control the string lenght that you want to read use round bracets ... so STRING(10)[20] would read 20 strings of each 10 chars length. You are currently mixing up the array size and the lenght.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it makes more sense then, will check out how rounded brackets go and update/revert template change if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have a look at the S7 protocol ... there we're already using the string-length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at S7 fields I see that its a special case regexp which is not going to fly with modbus unless we modify its definition. Will try cooking something there.

@ottlukas ottlukas added java Pull requests that update Java code Modbus https://plc4x.apache.org/users/protocols/modbus.html driver-modbus labels Apr 28, 2024
@chrisdutz
Copy link
Contributor

@splatch What's the state of this?

@splatch
Copy link
Contributor Author

splatch commented Jul 2, 2024

Hey Chris,
It is stale. Protocol wise, it was rather a minor change. I have no more access to inverter which provided answers, so all I have are samples (device identification request & response) attached to this PR.

@ottlukas ottlukas added the awaiting-feedback This label is applied when an issue has been opened and we need more information from the issuer. label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-feedback This label is applied when an issue has been opened and we need more information from the issuer. java Pull requests that update Java code Modbus https://plc4x.apache.org/users/protocols/modbus.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants