Skip to content

Conversation

@lightbringor
Copy link
Contributor

Fixed size calculation issues for classes when reading and writing arrays, since those always start with an even address in S7 data blocks.

…hose always start with an even address in S7 data blocks.
{
numBytes = Math.Ceiling(numBytes);
if ((numBytes / 2 - Math.Floor(numBytes / 2.0)) > 0)
numBytes++;
Copy link
Member

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++;

Copy link
Contributor Author

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.

Copy link
Member

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.
@mycroes
Copy link
Member

mycroes commented Jul 11, 2019

Can you update the PR to current develop?

@lightbringor
Copy link
Contributor Author

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 numBytes before GetClassSize(...) returns. But when you simply but 2 Bits after each other in the DB and then define the corresponding .NET class like in T31_ReadClassWithNestedClassAfterBit() with a class for the second Bit, my approach fails.

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.

@mycroes
Copy link
Member

mycroes commented Jul 11, 2019

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.

@timverwaal
Copy link
Contributor

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?

@mycroes
Copy link
Member

mycroes commented Aug 13, 2020

@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).

mycroes added a commit to mycroes/s7netplus that referenced this pull request Aug 13, 2020
@mycroes mycroes closed this in #280 Aug 13, 2020
mycroes added a commit that referenced this pull request Aug 13, 2020
Tests/TypeTests: Add ClassTests from #178
@mycroes
Copy link
Member

mycroes commented Aug 13, 2020

@timverwaal I just pushed 0.5.0 to NuGet.org, that could use testing as well (on any PLC).

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.

3 participants