Skip to content
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

Support structured variables in read_list_by_name #187

Conversation

pylipp
Copy link
Contributor

@pylipp pylipp commented Jan 20, 2021

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
Copy link
Contributor Author

pylipp commented Jan 20, 2021

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

@chrisbeardy
Copy link
Collaborator

chrisbeardy commented Jan 26, 2021

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.

Copy link
Owner

@stlehmann stlehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pylipp This PR looks good to me. I also tried a sample on my PC which worked well.

@stlehmann
Copy link
Owner

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

@pylipp
Copy link
Contributor Author

pylipp commented Feb 18, 2021

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

@chrisbeardy
Copy link
Collaborator

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.

- optional argument for passing structure_defs into read_list_by_name
- store bytes returned by SUMUP_READ request
- convert to dict in read_list_by_name
@pylipp pylipp force-pushed the feature/structure-support-for-read-list-by-name branch from b34d631 to adccc9c Compare February 18, 2021 16:41
@pylipp
Copy link
Contributor Author

pylipp commented Feb 18, 2021

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
Copy link

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 Coverage Status
Change from base Build 639: 0.04%
Covered Lines: 1143
Relevant Lines: 1209

💛 - Coveralls

@coveralls
Copy link

coveralls commented Feb 18, 2021

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 Coverage Status
Change from base Build 639: 0.04%
Covered Lines: 1143
Relevant Lines: 1209

💛 - Coveralls

@stlehmann
Copy link
Owner

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

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.

4 participants