You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2022/04/19 22:55:39 UTC
[GitHub] [plc4x] splatch opened a new pull request, #352: "Modbus plus"
splatch opened a new pull request, #352:
URL: https://github.com/apache/plc4x/pull/352
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #352:
URL: https://github.com/apache/plc4x/pull/352#discussion_r853995262
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
Review Comment:
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.
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
+ [simple vstring '8' value encoding='"UTF-8"']
+ ]
+ ['STRING' List
+ [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #352:
URL: https://github.com/apache/plc4x/pull/352#discussion_r854001057
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
+ [simple vstring '8' value encoding='"UTF-8"']
+ ]
+ ['STRING' List
+ [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #352:
URL: https://github.com/apache/plc4x/pull/352#discussion_r854032379
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
+ [simple vstring '8' value encoding='"UTF-8"']
+ ]
+ ['STRING' List
+ [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Review Comment:
Have a look at the S7 protocol ... there we're already using the string-length.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103792196
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a diff in pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on code in PR #352:
URL: https://github.com/apache/plc4x/pull/352#discussion_r857035719
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
+ [simple vstring '8' value encoding='"UTF-8"']
+ ]
+ ['STRING' List
+ [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103774437
> 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 put read from buffer. If its `String`, then output is not an array, but just an string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] "Modbus plus"
Posted by "hutcheb (via GitHub)" <gi...@apache.org>.
hutcheb commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-2560459784
Closing due to inactivity
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103733105
Seems C templates needs to be updated as well to handle string fields. Sadly I can't handle that part.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103789374
> 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]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103789461
"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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103788275
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a diff in pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on code in PR #352:
URL: https://github.com/apache/plc4x/pull/352#discussion_r854031480
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
+ [simple vstring '8' value encoding='"UTF-8"']
+ ]
+ ['STRING' List
+ [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Review Comment:
Ok, it makes more sense then, will check out how rounded brackets go and update/revert template change if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] "Modbus plus"
Posted by "splatch (via GitHub)" <gi...@apache.org>.
splatch commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-2204334781
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] "Modbus plus"
Posted by "hutcheb (via GitHub)" <gi...@apache.org>.
hutcheb closed pull request #352: "Modbus plus"
URL: https://github.com/apache/plc4x/pull/352
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a diff in pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
splatch commented on code in PR #352:
URL: https://github.com/apache/plc4x/pull/352#discussion_r853998595
##########
protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec:
##########
@@ -416,6 +416,15 @@
['WCHAR' List
[array uint 16 value count 'numberOfValues']
]
+ ['STRING','1' STRING
+ [simple vstring '8' value encoding='"UTF-8"']
+ ]
+ ['STRING' List
+ [simple vstring '8 * numberOfValues' value encoding='"UTF-8"']
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #352: "Modbus plus"
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #352:
URL: https://github.com/apache/plc4x/pull/352#issuecomment-1103743231
Ther test.mspec contains vstring fields ... could you please elaborate on what is actually not working?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org