Skip to content

Conversation

@lightbringor
Copy link
Contributor

Added read and write support for .NET datatype Char (and therefore also Arrays of char) to Types.Class and fixed an issue regarding the start adress of arrays (those are always an even number in S7 structs)

@mycroes
Copy link
Member

mycroes commented Aug 14, 2018

Dear @Buchter,

Sorry for this late response to your contribution. Can you explain what the goal is of this change? The char type in .NET is 16 bits (See char), while your PR only uses 8 bits to encode the value. I guess logically you're using this for strings, in case you assume that every character is in the ASCII set you could just as well use Encoding.ASCII.GetString() / Encoding.ASCII.GetBytes() to convert from/to bytes.

I don't want to merge this change as-is, since it would block proper implementation of UTF-16 strings (which are supported on S7-1500).

Please let me know if you have a different perspective on this. Also, I'd be happy to assist in finding a different solution to solve your problem if you so desire.

@lightbringor
Copy link
Contributor Author

lightbringor commented Aug 15, 2018

Hello @mycroes ,

thanks for your response.
I have to admit that I wasn't aware of the WCHAR Type of the S7-1200 and S7-1500.

The background is, I've written a parser that creates .NET classes from SCL sources of S7 Data Blocks and then use them with the ReadClass and WriteClass methods of S7NetPlus, which I really like. Since the S7-String with its defined and acutal length in the first two bytes is somewhat awkward and incompatible to most other systems, we widley use "Array of Char" in S7 that will be parsed to public char[] Name { get; set; } and initialized with the declarated length in the constructor of the generated class. This will of course also work by using the Encoding class if I parse "Array of Char" to byte[] as you suggest. However, this will make it necessary to have knowledge about the actual type of data, since it will no longer be unambiguous from the declaration.

I totally understand your point and think that in my case the solution could be to pass the information of the actual type as a property comment by the parser.

Nevertheless, there is still the issue that arrays in S7 Data Blocks always start at an even adress what needs to be considered when calculating the class size. I know I shouldn't have included both changes in one commit in the frist place. I'm still new to GitHub, so I need some advice. Shall I create an other commit removing the switch case for char or shall I create a new PR with the current HEAD as base?

@mycroes
Copy link
Member

mycroes commented Aug 15, 2018

Hi @Buchter,

Please do that in a new PR. You can easily do that by resetting to S7NetPlus/develop, then create a new branch from there (fix-struct-field-offset) and then cherry-pick the commit that fixes the offset calculation. I must say though, that the even byte offset doesn't only apply to arrays. However, I'll be happy to merge a change that fixes that part. I don't use this functionality myself (honestly I'm not a real S7NetPlus user at all, I have a competing library that I use), so it's good to get fixes from someone that actually makes use of this functionality.

mycroes added a commit to mycroes/s7netplus that referenced this pull request Aug 13, 2020
@mycroes mycroes closed this in 50f0e62 Aug 13, 2020
@mycroes
Copy link
Member

mycroes commented Aug 13, 2020

Closed because Char support should probably use UTF-16 strings as stated before. Array offsets are fixed in #279.

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.

2 participants