search

S7NetPlus / s7netplus

S7.NET+ -- A .NET library to connect to Siemens Step7 devices
MIT License
1.33k stars 588 forks source link

Jjelks - Resolves issues from #414 (added 64-bit value support) #416

Closed Jason-Jelks closed 1 year ago

Jason-Jelks commented 3 years ago

Added support for LWord, LInt and ULInt PLC types. Changed most byte[] passed parameters to Span, except in specific cases where performance and/or complexity outweighed benefits. Class now supports all types except SInt and USInt. The SInt family is a single byte but gets allocated 2 bytes which conflicts with Int16 conversion process. Added UnitTest for Class for 64-bit.

mycroes commented 3 years ago

Hi Jason,

Thanks for your contribution. Contribution might actually be a huge understatement, because with a grand total of 70 files changed I'm guessing there's a lot more than just one contribution. Unfortunately that's also the problem with this PR.

As with commits I'd like to keep PR's scope limited to a single feature, but this PR contains so many changes I'm not even sure where to start reviewing. I took a peek through all the changed files and noticed there are quite some changes that are probably the result of some merges. I'm not sure if there are valid changes in there, but certainly there are some changes that shouldn't be merged (like a bunch of unimplemented unit tests).

My suggestion would be to create a new branch from the latest commit in this PR, do a mixed reset to S7NetPlus develop and stage and commit relevant changes only. You could repeat the procedure if there are other changes you want to contribute, like perhaps some of the changes to the unit tests. Another option would be that I do this myself, picking only the changes related to 64 bit sized data. I'd still be reviewing that code in that case and might change it as I see fit at that point, while I can also imagine you might actually want to be the one contributing these changes.

Let me know what would work best for you!

Regards, Michael

Jason-Jelks commented 3 years ago

Yes I have been working on many of these for some time. Everything descends from 0.13 release and heavily tested on S7-1200 & S7-1500, but I will get those modifications parsed out (likely this weekend); as I would certainly like to be a contributor.

Not sure if you would be interested in another section I am working on (but that is heavily dependent on the 'Class.cs' modifications we have done). I have just finished our preliminary gRPC interface using current s7netplus (obviously from my branch), and am looking to add a section for limited Protobuf parsing from both Unary calls and Streams. I will say... it is extremely fast, particularly when streaming things like real-time position from axis controller. (Probably few months out on something to release here as we have some other priorities, but if you think it is something others would like I will keep it in mind when I am adding it).