Split read write by list into max-ads-sub-comands chunks #200

chrisbeardy · 2021-02-15T13:14:58Z

No description provided.

chrisbeardy · 2021-02-15T13:51:01Z

The tests are failing for the new sub-commands tests, they are failing with a:

pyads.pyads_ex.ADSError: ADSError: parameter size not correct (1797).

I submitted the PR still as I can't work out why the testserver is returning this, so would appreciate some help? I have tested with an actual PLC, and the new code works as expected but just not in the test with the testserver. The spliting of lists function works so what is actually being passed into adsSumRead is what is expected so I can only see that there is an issue with the testserver not responding as expected?

The code I used to test with the PLC is here:

I just set up an array in MAIN:

iTest: ARRAY [0..1400] OF INT;

import pyads
import time

plc = pyads.Connection("169.254.198.195.1.1", 851)

plc.open()


now = time.time()
test_array = {f"MAIN.iTest[{i}]": i for i in range(0, 1401)}
plc.write_list_by_name(test_array)
print(plc.read_list_by_name(test_array))
test_array = {f"MAIN.iTest[{i}]": 0 for i in range(0, 1401)}
plc.write_list_by_name(test_array)
print("First Written: " + str(time.time() - now))
test_array = {f"MAIN.iTest[{i}]": i for i in range(0, 1401)}
plc.write_list_by_name(test_array)
test_array = {f"MAIN.iTest[{i}]": 0 for i in range(0, 1401)}
plc.write_list_by_name(test_array)
print("Second Written: " + str(time.time() - now))
test_array = {f"MAIN.iTest[{i}]": i for i in range(0, 1401)}
plc.write_list_by_name(test_array, ads_sub_commands=1401)
test_array = {f"MAIN.iTest[{i}]": 0 for i in range(0, 1401)}
plc.write_list_by_name(test_array, ads_sub_commands=1401)
print("Third Written: " + str(time.time() - now))
test_array = {f"MAIN.iTest[{i}]": i for i in range(0, 1401)}
plc.write_list_by_name(test_array, ads_sub_commands=2)
test_array = {f"MAIN.iTest[{i}]": 0 for i in range(0, 1401)}
plc.write_list_by_name(test_array, ads_sub_commands=2)
print("Fourth Written: " + str(time.time() - now))


test_array = [f"MAIN.iTest[{i}]" for i in range(0, 1400)]

print(plc.read_list_by_name(test_array, ads_sub_commands=1))
print("Second Read: " + str(time.time() - now))
plc.close()

chrisbeardy · 2021-02-15T13:54:57Z

parameter size not correct suggests that the test server is not correctly storing the size between reads/writes of the test variables?

I don't know if @pylipp or @bdcoyle have any thoughts? I can't remember who added the intial read/write list functionailty?

stlehmann · 2021-02-16T10:56:25Z

That really seems to be a bug or limitation of the testserver. It can only handle 3 or 4 variables on AdsSumRead.

1 variable: Warning: Frame too long: 31>17
2 variables: Frame too long: 31>26
3 variables: OK
4 variables: OK
5 variables: KeyError: 1702101505
6 variables: KeyError: 1702101505

stlehmann · 2021-02-16T13:27:02Z

I actually don't know what's happening here exactly but it seems the SUMUP_READ is limited to return 3 values: 1, 2 and "test". So you might want to adjust your test to read 6 variables and split by 3.

pyads/pyads/testserver.py

Lines 494 to 499 in 0c93dfe

    
           elif index_group == constants.ADSIGRP_SUMUP_READ: 
        
               # Could be improved to handle variable length requests 
        
               response_value = struct.pack( 
        
                   "<IIIIBB4sB", 0, 0, 0, 1, 1, 2, 
        
                   ("test" + "\x00").encode("utf-8"), 0 
        
               )

chrisbeardy · 2021-02-16T13:28:42Z

Ok, I'll give that a go

bdcoyle · 2021-02-16T15:00:56Z

Sorry for the slow response. I remember when writing the tests for the sum read/write I was also getting the 'Warning: Frame Too Long'. It seems to come from AdsLib (AmsConnection.cpp) but I couldn't work out why.

https://github.com/stlehmann/ADS/blob/d0804bc4edc68851c75f982e0aebc7c2386b0a29/AdsLib/AmsConnection.cpp#L244-L248

Since it was working when I tested on an actual PLC, I made the hardcoded response within the limit as mentioned above for the purpose of the testserver

stlehmann · 2021-02-16T16:25:13Z

@chrisbeardy I'll take a deeper dive in the sum read handling on the testserver and will try to make it work with variable length and datatypes. So you might not need to change your tests at all.

chrisbeardy · 2021-02-16T17:42:12Z

@stlehmann that would be great if possible

Testserver: SUMUP_READ and SUMUP_WRITE with variable length

…t_read_write

coveralls · 2021-02-17T13:11:42Z

Pull Request Test Coverage Report for Build 629

23 of 23 (100.0%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.009%) to 94.458%

Files with Coverage Reduction	New Missed Lines	%
pyads/pyads_ex.py	1	90.3%

Totals
Change from base Build 609:	0.009%
Covered Lines:	1125
Relevant Lines:	1191

💛 - Coveralls

coveralls · 2021-02-17T13:11:42Z

Pull Request Test Coverage Report for Build 630

23 of 23 (100.0%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.009%) to 94.458%

Files with Coverage Reduction	New Missed Lines	%
pyads/pyads_ex.py	1	90.3%

Totals
Change from base Build 609:	0.009%
Covered Lines:	1125
Relevant Lines:	1191

💛 - Coveralls

stlehmann · 2021-02-17T13:15:54Z

@chrisbeardy thanks for this PR and sorry for the mess with the commits. Actually I wanted to rebase on master but this didn't work out so well.

stlehmann · 2021-02-17T13:18:04Z

tests/test_ads.py

@@ -561,6 +561,61 @@ def test_bytes_from_dict(self):
        with self.assertRaises(KeyError):
            pyads.bytes_from_dict(OrderedDict(), structure_def)

+    def test_dict_slice_generator(self):


Can we remove the test slice generators as they are not used?

do you mean just remove the tests? as the functons are still used in the read/write_list_by name functions in ads.py.

even though the tests for the read/write_list test_read_list_ads_sub_commands technically cover the usage of the _slice_generators, I would argue as they are standalone functions, it is still important to test them with tests spefic to test those standalone functions?

Ah sorry I didn't see that these are tests. I thought it to be a generator for some test data so I looked if they are called anywhere in the code which was not the case. Alright then all is set and I shall merge this PR :)

chrisbeardy · 2021-02-17T13:22:07Z

I always struggle with rebasing a PR in GitHub! Half the time I end up creating a new PR. Thanks for the great work on pyads and others on some of the latest features. It's allowing me to build a really powerful front end for a machine I'm working on now which would have been not as easy otherwise.

stlehmann · 2021-02-17T13:24:35Z

The rebase-button of PyCharm was so tempting 😞

chrisbeardy · 2021-02-17T15:07:38Z

@chrisbeardy thanks for this PR and sorry for the mess with the commits. Actually I wanted to rebase on master but this didn't work out so well.

Actually when you merge a PR in GitHub, do you get the option to rebase it then?

stlehmann · 2021-02-17T15:41:56Z

Might be. I think the problem was that I did a rebase in between.

chris added 2 commits February 15, 2021 09:24

implementation, need tests

4efdc06

added tests for added max-sub-commands func

28204fd

chris and others added 2 commits February 15, 2021 13:59

added new line at end of file

bae35d9

Fix read_state of AdvancedHandler

029b9ff

Count variables

12a3076

stlehmann and others added 14 commits February 17, 2021 09:36

Allow variable length for SUMP_READ

e008fe5

Allow variable length for SUMP_WRITE

40826de

Fix tests

6e66d08

Support string datatype

2eb3f3e

Restore test for SUM_WRITE with str

b281443

Update Changelog

8c71797

Merge pull request stlehmann#202 from stlehmann/fix_testserver

b2dd269

Testserver: SUMUP_READ and SUMUP_WRITE with variable length

implementation, need tests

e0cc056

added tests for added max-sub-commands func

c09309e

added new line at end of file

2c6ec5b

Fix string length and remove print-statement

3eda2c4

Alter tests to fit testserver quirks

bc8347e

Merge remote-tracking branch 'chrisbeardy/split_read_write' into spli…

8adb0f4

…t_read_write

Alter tests again after merge

0b9bda5

Update CHANGELOG.md

ea6c790

stlehmann requested changes Feb 17, 2021

View reviewed changes

stlehmann merged commit b19cbd3 into stlehmann:master Feb 17, 2021

chrisbeardy deleted the split_read_write branch February 17, 2021 15:05

pylipp mentioned this pull request Feb 18, 2021

Support structured variables in read_list_by_name #187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split read write by list into max-ads-sub-comands chunks #200

Split read write by list into max-ads-sub-comands chunks #200

chrisbeardy commented Feb 15, 2021

chrisbeardy commented Feb 15, 2021

chrisbeardy commented Feb 15, 2021

stlehmann commented Feb 16, 2021

stlehmann commented Feb 16, 2021

chrisbeardy commented Feb 16, 2021

bdcoyle commented Feb 16, 2021 •

edited

Loading

stlehmann commented Feb 16, 2021

chrisbeardy commented Feb 16, 2021

coveralls commented Feb 17, 2021

coveralls commented Feb 17, 2021 •

edited

Loading

stlehmann commented Feb 17, 2021

stlehmann Feb 17, 2021

chrisbeardy Feb 17, 2021

chrisbeardy Feb 17, 2021

stlehmann Feb 17, 2021

chrisbeardy commented Feb 17, 2021

stlehmann commented Feb 17, 2021

chrisbeardy commented Feb 17, 2021

stlehmann commented Feb 17, 2021

Split read write by list into max-ads-sub-comands chunks #200

Split read write by list into max-ads-sub-comands chunks #200

Conversation

chrisbeardy commented Feb 15, 2021

chrisbeardy commented Feb 15, 2021

chrisbeardy commented Feb 15, 2021

stlehmann commented Feb 16, 2021

stlehmann commented Feb 16, 2021

chrisbeardy commented Feb 16, 2021

bdcoyle commented Feb 16, 2021 • edited Loading

stlehmann commented Feb 16, 2021

chrisbeardy commented Feb 16, 2021

coveralls commented Feb 17, 2021

Pull Request Test Coverage Report for Build 629

💛 - Coveralls

coveralls commented Feb 17, 2021 • edited Loading

Pull Request Test Coverage Report for Build 630

💛 - Coveralls

stlehmann commented Feb 17, 2021

stlehmann Feb 17, 2021

Choose a reason for hiding this comment

chrisbeardy Feb 17, 2021

Choose a reason for hiding this comment

chrisbeardy Feb 17, 2021

Choose a reason for hiding this comment

stlehmann Feb 17, 2021

Choose a reason for hiding this comment

chrisbeardy commented Feb 17, 2021

stlehmann commented Feb 17, 2021

chrisbeardy commented Feb 17, 2021

stlehmann commented Feb 17, 2021

bdcoyle commented Feb 16, 2021 •

edited

Loading

coveralls commented Feb 17, 2021 •

edited

Loading