[GitHub] [plc4x] chrisdutz commented on a diff in pull request #352: "Modbus plus"

[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