Support structured variables in read_list_by_name

stlehmann / pyads

Python wrapper for TwinCAT ADS

MIT License

255 stars 94 forks source link

Support structured variables in read_list_by_name #187

Closed pylipp closed 3 years ago

pylipp commented 3 years ago

Acc. to discussion in #183.

For support in write_list_by_name I'd open a separate PR.

I could not test it with a PLC yet.

pylipp commented 3 years ago

The unittest failing on Travis locally passes; not sure how to do more inspection here.

chrisbeardy commented 3 years ago

The approach of having to pass the structure definitions in and use it to unpack the bytes is the first approach I pondered about when the question first came up in #183. As has been mentioned I also don't think ADS has native support for structures as the symbol info just tells you it's a structure, (which is how the read_struct_by_name code came about and annoyingly having to know the structure definition up front), so to me this looks like a good "workaround" to allow you to mix the read_list and read_struct fucntionality. I'm quite busy on projects at the minute but I may be able to test at some point.

However if you do have the time, you do not neccesarily need a PLC to test it. If you have a windows laptop or can spin up a Windows virtual machine, you can install TwinCAT 3 on that and test it by pointing the target system to local / or the VM.

stlehmann commented 3 years ago

I don't know why the CR fails, though. It might be some issue related to adslib which is used on Linux only.

pylipp commented 3 years ago

Will update this after the changes of #200 and #202 !

chrisbeardy commented 3 years ago

Will update this after the changes of #200 and #202 !

sorry, I knew that #200 would mean that this would need rewoking. But I thought it to be a sensible addition to add in.

pylipp commented 3 years ago

Will update this after the changes of #200 and #202 !

sorry, I knew that #200 would mean that this would need rewoking. But I thought it to be a sensible addition to add in.

Totally! Was straightforward to update the present branch :)

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 640

10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 94.541%

Totals
Change from base Build 639:	0.04%
Covered Lines:	1143
Relevant Lines:	1209

💛 - Coveralls

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 641

10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 94.541%

Totals
Change from base Build 639:	0.04%
Covered Lines:	1143
Relevant Lines:	1209

💛 - Coveralls

stlehmann commented 3 years ago

Great work @pylipp 👍. I'll merge it right away.