-
Notifications
You must be signed in to change notification settings - Fork 646
Fixed size calculation issues when reading and writing arrays #178
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
Fixed size calculation issues when reading and writing arrays #178
Conversation
…hose always start with an even address in S7 data blocks.
S7.Net/Types/Class.cs
Outdated
| { | ||
| numBytes = Math.Ceiling(numBytes); | ||
| if ((numBytes / 2 - Math.Floor(numBytes / 2.0)) > 0) | ||
| numBytes++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say if (numBytes & 1 == 1) numBytes++;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this looks a little bit more clean, though it misses braces around the & operation. But you still would need to do the Math.Ceiling(numBytes) and cast the result to int to apply the logic operator. Thereafter it is cast back to double for the calling environment. I made a quick test with 10E7 runs and a ranoum value, both approaches needed 3.5 to 4 seconds on my machine. So from the performance side, there is not really a difference.
But I'm happy to change this if you finally merge it into master, because we are forced to use our own fork for almost a year now and cannot participate in new features without merging constantly from master for ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of readability I'd like a change, doesn't have to be my suggested change, but the current line isn't very expressive with regards to an odd/even check.
About the release part, I'm really behind on that, but my intent is to release soon, would be good to have this included.
…UnitTest for size calculation of objects with uneven sizes.
|
Can you update the PR to current develop? |
|
Ok, some tests are failing now. The problem is, I have assumed, that if one puts a class into another class, this structure is also reflected in the S7 DataBlock by using a struct. As structs always start at an even byte adress I've increased the So in the end, one cannot really tell how large a DB actually is only by analyzing the class structure. Two Siemens DBs can vary in their size and offset adresses even with the exact same content. I still woud prefer my approach, since using a struct in the DB where you define a sub class in the .NET world seems reasonable to me while its not that good of a style to use seperate classes in .NET when stroing everything as basic types in the PLC. But this is only an opinion. However, the approach as it is won't work as soon as someone uses a struct in a DB where the preceding variable is at DBX2.3 for example. In this case the struct would start at DBX4.0 but S7NetPlus would assume it is DBX3.0. This happens alsways if one wants to use multiple UDTs in one DB which is an often seen scenario from my expirience. Unfortunately this is my last day before 3 weeks of summer vacation, you've had a bad timing when bringing this up again :-) I won't have access to a Windows machine until week 32. But I'll look into this further after my return. |
|
I think I agree on your view with regard to the addressing. If I understand you correctly (and from my experience) fields, structs and arrays always start at even offsets, so if you mimic the data types used in the PLC in C# then the next class should also start at an even offset. Personally I don't like the class reading code at all. I use some mapping code that provides similar functionality, but translates to a multiple read call with all variables provided separately. I'll see when you're done with the changes. I also intend to do releases more often. I've tried to get people to test some features a couple of times, but unfortunately there's hardly any feedback to be found. |
What kind of features need to be tested? |
|
@timverwaal I'm currently unsure if all supposedly supported PLC types are actually still working, because I basically only hear from people when it doesn't work. I also added string support, but I'm unsure if it works at all (I can't remember if I ever tested it, at all) and honestly I'd have to hook up a PLC and install TIA Portal before I can actually test anything at all (bar testing against snap7, which isn't perfect either). |
Tests/TypeTests: Add ClassTests from #178
|
@timverwaal I just pushed 0.5.0 to NuGet.org, that could use testing as well (on any PLC). |
Fixed size calculation issues for classes when reading and writing arrays, since those always start with an even address in S7 data blocks.