Added list read/write for symbols

[RobertoRoos]

Fixes #266

This adds the option to read multiple symbols at once, or write to multiple at once.

Unfortunately this is not directly compatible with adsSumRead and adsSumWrite (assuming we don't want the redundant info lookup when using only the name).
This is because those functions are based on the SAdsSymbolInfo structs, which contain the integer dataType, instead of a ctypes like plc_type.

I've tried to move some of the logic to separate functions so they can be reused, but the new symbol read/write methods resemble the classic functions a lot.

Example script:

from pyads import Connection
from random import random

plc = Connection('127.0.0.1.1.1', 851)

with plc:

    var_names = [
        "GVL.my_double",
        "GVL.another_double",
        "GVL.my_text",
        # "GVL.my_list"
    ]

    symbols = [plc.get_symbol(var) for var in var_names]

    new_data = [random(), random(), "Hello again"]

    data_with_names = dict(zip(var_names, new_data))

    # response = plc.write_list_by_name(data_with_names)
    # print(response)

    response = plc.write_list_symbols(symbols, new_data)
    print(response)

    # data = plc.read_list_by_name(var_names)
    # print(data)

    result = plc.read_list_symbols(symbols, True)
    print(result)

    for symbol in symbols:
        print(symbol.name + ":", symbol.value)

[RobertoRoos]

@stlehmann I was hoping you might already have some insights here.
Would you like less overlap with the old *_list_by_name functions?
Do the new interfaces seem right?

[chrisbeardy]

This looks like a pretty good additional feature, nice work! The symbol way of using pyads I think is fast becoming a favourite from the looks of things. I can already see a use case to optimise some of my code to use the reading a list of symbols.

I haven't looked through the implementation in detail yet, but some fast thoughts with my opinion:

• My thought is this is going to be a well used feature, as I said as soon as I noticed this PR and your example I got excited
  • given this addition there will sort of be two competing ways to use pyads from an end users perpsective so time should be given to make sure this is straight forward and clear to the end user what all methods do but also maintain familarity between them.
  • Also given my point that I think it will be well used, time should be given to make sure it's straightforward and clear for the maintainer and contributors of pyads to maintain.
• I believe changing the method names to read_list_of_symbols and write_list_of_symbols may read better to the user, gives an accurate description of what the method is doing and isn't that much longer for a name.
• Possibly could think about the parameters for the write_list_symbols; would it make sense to make this a dictionary so the interface is in line with write_list_by_name with the keys being the symbol and the values the value to write to that symbol. There is an argument that could be had to which one is a nicer interface (two lists or a dict) and which one is optimal, but since write_list_by_name and to some extend write_structure_by_name use dictionaries as input I believe consistency across the module would be beneficial.
• Have read_list_symbols always return the dictionary and do away with the optional parameter. Again for consistency with read_list_by_name and also I believe the user will likely be expecting something back when asking to read something from the plc even though in this case it is not actually required. I expect the time overhead of additionally inserting the keys into a dictionary will not be of importance? Alternatively just swap the optional parameter around so the default is to return.
• I am wondering if it is possible to modify adsSumReadBytes to have the same interface as adsSumRead but instead just return bytes instead of doing the processing as you say. I'm not sure it is possible to modify data_symbols: List[Tuple[int, int, int]] into data_names: List[str], data_symbols: Dict[str, SAdsSymbolEntry], structured_data_names: List[str] and then that processing for the conversion can be pushed into the method that calls adsReadSumBytes.
  • The singledispatch library could be considered (or two methods or using more private methods with more descriptive names or another solution) so that am adsSumReadBytes like method could exist for datanames, then we can have adsReadSum just call that. Also down the same lines, just have an adsReadSum which accepts both options.
  • I think my main concern here is that as you say there is an overlap with the existing functions, but I think that is ok and can not be avoided to some extent as both methods (by dataname and by symbol) are both just reading bytes from the PLC and converting them into the desired format at the end of the day. I think it just needs to be thought about and made clear to why there are similar methods that appear to do a similar thing but have different parameters.
  • From an end users perspective this is just the internals of the library but I think for the maintenance of the library going forward it is important.
  • Is there scope to potentially have read_list_of_symbols just wrap around read_list_by_name with the addition of updating the symbol cache. As the symbol contains the dataname. I guess this becomes memory inefficient as you then build up two caches and adds an extra overhead of having to fetch the handle twice.
  • I am sure @stlehmann will have a better understanding of the organisation of the library and be able to give more thoughts
• I am not 100% sure if it is applicable here , but I know beckhoff advise that no more that 500 symbols/requests are read/written in one ADS call. Therefore in the implementation of read/write_list_by_name the list of datanames is chunked up into number of ads_sub_commands blocks. I think this is required here too as there is still a number of ads requests:
  
• Could consider renaming convert_data_to_value method to again be a more specific and descriptive (naming is always hard and contentious so I apologise if this seems petty). possibly something like get_value_from_ctype_data.
• Could also consider refactoring to be more like read_list_by_name where the conversion from bytes to relevant symbol data function is in pyads_ex.py, then can make convert_data_to_value a private method that doesnt span modules, possibly making ads.py easier to read. Again I'm sure @stlehmann will have a better thought on this.
• It could be worth splitting this work into two PRs, purely from an admin perspective, one that refactors pyads_ex.py and adds in the adsReadSumBytes, potentially making it compatible with the existing adsReadSum. Then another PR for actually adding in the read/write_list_symbols feature. This way I think it may make it easier to test and make sure the first PR and refactoring of the pyads_ex.py file does not break anything given the difficulty in thoroughy testing this library. This could be made into a minor release, giving maintainers a chance to easily debug in the future.
• Likely you were planning on it but tests and docs wil need to be added too.

My apolgies for what grew into a longer list than I thought as I spent more time thinking about it but I hope that is food for thought and helps you and @stlehmann guide this feature smootly into the library.

Also, I know that I am not a maintainer of pyads, but I have contributed and I use it a lot so I have an interest in the features that get added so thank you again for your work on the AdsSymbol side of things!

[RobertoRoos]

Whoof, a lot. But thanks @chrisbeardy , that's very useful.

I believe changing the method names to read_list_of_symbols and write_list_of_symbols may read better to the user, gives an accurate description of what the method is doing and isn't that much longer for a name.

Agreed, I'll change that.

Possibly could think about the parameters for the write_list_symbols; would it make sense to make this a dictionary so the interface is in line with write_list_by_name with the keys being the symbol and the values the value to write to that symbol. There is an argument that could be had to which one is a nicer interface (two lists or a dict) and which one is optimal, but since write_list_by_name and to some extend write_structure_by_name use dictionaries as input I believe consistency across the module would be beneficial.

Yep, was thinking about that too. I'll change it.
Though nothing stops us from accepting both? We could detect if the first argument is a list or dict, and take it from there? Though that might make the interface too complicated.

EDIT: And I do want to insist on the option to write values through ._value. I can easily imagine a flow where a user first has a bunch of interaction with symbols directly, and afterwards all values will be send in a single call. Being forced to fiddle with symbols and dicts is then not ideal.

Have read_list_symbols always return the dictionary and do away with the optional parameter. Again for consistency with read_list_by_name and also I believe the user will likely be expecting something back when asking to read something from the plc even though in this case it is not actually required. I expect the time overhead of additionally inserting the keys into a dictionary will not be of importance? Alternatively just swap the optional parameter around so the default is to return.

Also agreed. Will change.

Could consider renaming convert_data_to_value method to again be a more specific and descriptive (naming is always hard and contentious so I apologise if this seems petty). possibly something like get_value_from_ctype_data.

Will change.
No, I don't find it nick-picky! This part of writing good code, and in terms of naming I can still learn some.

Alright, I think that most of the concrete points. I guess now is the part where we debate about structure and details.

About the refactoring stuff, that is a good point. Though I think we need both interfaces (read_list_by_name and read_list_of_symbols) in order to write a good shared low-level base.
I'm personally not too worried about things breaking. The changes I made so far are no big changes. But I understand wanting caution.

[coveralls]

Pull Request Test Coverage Report for Build 1435643505

• 103 of 108 (95.37%) changed or added relevant lines in 2 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.2%) to 94.2%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyads/connection.py	59	61	96.72%
pyads/pyads_ex.py	44	47	93.62%

Files with Coverage Reduction	New Missed Lines	%
pyads/connection.py	1	96.42%

Totals
Change from base Build 1358363211:	0.2%
Covered Lines:	1689
Relevant Lines:	1793

---

💛 - Coveralls

[RobertoRoos]

I've started a refactor branch containing the refactors I did in this branch (but without the new interface): https://github.com/RobertoRoos/pyads/tree/feature/refactor

We can create a pull request for that, and retarget this PR into that one to split the diffs.

[chrisbeardy]

I like this as an API for the user.

I agree that it may be the case the user is using symbols and lists and then it would be irritating to force them to convert this to a dictionary just for the input to the write method. My only slight concern is the use of advising the user to write to a private value of the symbol class. I guess the "proper" way do do it would be to to make sure auto_update is off and then write the property value instead but I can understand why that would be awkward and confusing to a user who doesn't care about the implementation. I guess as long as it is properly documented then I see no big harm. I am also happy with the writing of the private variables in the implementation of read/write_list_of_symbols as they are tightly coupled to the symbol class anyway and can be dealt with using unit tests.

The only other change to be considered from a user API perspictive is the ads_sub_commands option which by default should be set to 500.

As you say that I believe that is the concrete / user perspective points. The rest is internal implementation, this can be discussed and I believe @stlehmann is likely best placed to have a view on this when they have time available. Although as it is internal and I agree your changes are not huge, there is always scope to just refactor the internals at a later date if/when required.

As for naming, I discovered this cheatsheet a little while ago and I thing it sums up and makes some good suggestions.

[chrisbeardy]

Also I forgot to mention, i like the use of

Comparable to :meth:`Connection.read_list_by_name`.
See also :class:`pyads.AdsSymbol`.

in the docstrings.

[stlehmann]

Sorry to keep you guys waiting. I'm a bit busy at work at the moment and hardly finding any sparetime for development. @RobertoRoss I'll try to give it a closer look during the week.

[stlehmann]

@RobertoRoos I have seen through your changes and noticed that there is still some heavy refactoring included here. So I'll address this first before getting to the real stuff.

Arguably we might or might not want to move the helper functions from pyads_ex to another module but I think this PR is not the right place for these changes. Also I don't think I want to have a new module. There is already the ads.py module which contains various helper functions so I think it would be an option to place these functions there. As far as I can see the refactoring is not crucial for the implemented features so I suggest you move these changes away from this PR and place them in a new PR so they can be reviewed and maybe implemented later on.

I think you also might need to rebase the PR as adsSumReadBytes and adsSumWriteBytes are still part of this PR even though they got defined in #269 .

[stlehmann]

given this addition there will sort of be two competing ways to use pyads from an end users perpsective

That is actually a concern I shared as soon as I saw the new methods. As long as it is only two methods that is not so big an issue. But I guess @RobertoRoos will have plenty of ideas concerning symbol handling and I wonder if it would be for the better to put symbol handling in a separate package (e.g. pyads-symbols) that will extend the basic functionality provided by pyads. After all pyads is meant only to be a basic wrapper around the C-DLL provided by Beckhoff without too much bells and whistles. Looking forward to you thoughts on this.

[RobertoRoos]

@stlehmann yeah I never rebased this branch onto the other refactoring branch. I'll do that next.

About the double interfaces, I don't think splitting this package would be necessary. I would say all real functionality remains in separate functions, and the Symbol class just references those function.
I find it hard to estimate how confusing the two methods are to new Pyads users. I think it's manageable.

[stlehmann]

Yes, a rebase would be good. You can rebase on master because the refactoring branch has already been merged.

[stlehmann]

Ah this looks much better now. Thanks for rebasing. 👍

[RobertoRoos]

I'm not quite happy with the code structure and DRYness. I see roughly two options:

1. Alter adsSumRead/Write so it accepts both SAdsSymbolEntry type symbol info and another symbol info type (e.g. it could be a tuple like (idx_offset, idx_group, plc_type))
   • This will result if code that's a little ugly or bulky. We will have two procedures in the same function:

# In adsSumRead/Write

# Prepare summed bytes read / write:
if type_is_SAdsSymbolEntry:
    infos = ...
else:
    infos = ...

result = make_request(infos)

for info in infos:
    result_section = result[....]

    if type_is_SAdsSymbolEntry:
        value = ....(result_section)
    else:
        value = ....(result_section)

     # Process values

• And we cannot pass the AdsSymbol directly to maintain a clear order of dependency.

2. Duplicate a lot of the adsSumRead/Write code so it works the AdsSymbol class.
   • This will not by DRY. Many sections of the code will be identical or very similar.

Option 2 is pretty much what I've done so far, but I'm liking it less and less. I now need to add the ads_sub_commands feature but that will again be a copy-paste. EDIT: Bad example, since the request split is done inside Connection, not pyads_ex.

I would appreciate some thoughts. @stlehmann maybe, or @chrisbeardy ? Thanks!

[stlehmann]

At first I dismissed the idea to put Symbol support in read/write_list_by_name functions. But giving it another thought it might be a clean way to avoid repeated code and also provide a seemless integration of Symbols in the existing toolchain. Also I think it won't get too ugly.

So the interface could look like this:

    def read_list_by_name(
            self,
            data_names: List[Union[str, AdsSymbol]],
            cache_symbol_info: bool = True,
            ads_sub_commands: int = MAX_ADS_SUB_COMMANDS,
            structure_defs: Optional[Dict[str, StructureDef]] = None,
    ) -> Dict[str, Any]: 

Of course the function name would not fit well anymore. :(

[stlehmann]

A way I can think of: Create two new functions read_list and write_list which support both types (symbols and names). The old functions read/write_list_by_name will be marked as deprecated and eventually be removed from the API.

This way we avoid to have two functions that do pretty much the same and we also get rid of repeating code eventually when the old functions are removed.

[RobertoRoos]

I would take a different approach. Keep read_list_by_name and read_list_of_symbols but let both access some common function. That way the API remains compatible.

[RobertoRoos]

Okay, I've created the above. I think this could work, I like the unit-purposes:

• read_list_by_name handes the name-info retrieval while read_list_of_symbols only processes the symbol objects
• _read_list queries the pyads_ex back end to get Python values out of symbol infos (also handles ads query limit)
• adsSumRead gets binary data from symbol info and turns that into Python values (regardless of the type of info storage)

@stlehmann , thoughts?

I'll add more docs and tests once we like the overall structure.

[RobertoRoos]

i always makes me think of a numeric key, so I wanted to replace with something more verbose.

[stlehmann]

@RobertoRoos please remove the reformatting from the PR. We had this a few times now and I won't accept any PRs refactoring or reformatting other parts of the code anymore. If you see something that needs reformatting please raise an issue or make a separate PR for this.

[RobertoRoos]

@stlehmann I apologize, I wanted to run black on my code, I didn't look at what it would do to other existing code.

[chrisbeardy]

We should look at adding this into the CI pipeline https://pypi.org/project/darker/

To address this issue.

[stlehmann]

Okay, I've created the above. I think this could work, I like the unit-purposes:

* `read_list_by_name` handes the name-info retrieval while `read_list_of_symbols` only processes the symbol objects

* `_read_list` queries the pyads_ex back end to get Python values out of symbol infos (also handles ads query limit)

* `adsSumRead` gets binary data from symbol info and turns that into Python values (regardless of the type of info storage)

@stlehmann , thoughts?

I gave this some thoughts and I come to dislike the idea of different functions for symbols and names more and more. The Symbols approach should integrate smoothly in the current API and shouldn't feel so much like a complete different way of doing things with pyads. So my preferred way is to with Connection.read_list and Connection.write_list processing both strings and symbols and given some time deprecate the read_list_by_name function.

[RobertoRoos]

Alright, that's understandable. I'll modify the code (but I think I'll archive the current version on my fork just in case).

I intend to approach it with an if ... else ... in read_list(), calling one of two private methods to do the real read.

[stlehmann]

Yes, I think a simple if ... else should do to separate the two cases.

[RobertoRoos]

And there it is. I started with updating the docs too.

[stlehmann]

@RobertoRoos Thank you very much for your many efforts on this issue.

Sadly I must tell you that I'm more than a bit reluctant to apply your changes because they add so much complexity and are hard to overview and will be even harder to maintain. You must understand that pyads is just a small side-project of mine and I have limited time for it. So my priority is to keep the package up-to-date and maintain the code-base. Additional features are appreciated but should be implemented in a straight-forward way with minimal changes to the existing codebase. This PR applies many fundamental changes to the existing codebase. Plainly speaking it is just too big for me to overview. And even though I'd like to have this feature implemented I think it is not worth all the refactoring, at least at the moment. I'm more than willing to keep this PR open and check if we can implement it at some later point in time. Also you are more than welcome to work on the codebase in small steps to make the necessary changes to apply this feature smoothly in the future.

Kind Regards
Stefan

[stlehmann]

As this can be symbols and names we might just call them items

Suggested change

	symbols: Union[List[str], List[AdsSymbol]],
	items: Union[List[str], List[AdsSymbol]],

[stlehmann]

Just a thought: In my opinion the point of using symbols is that we can cache the values in dedicated objects. The normal use-case for me is to write all the values to the symbol objects and then write the list with one command:

symbol1.value = 10
symbol2.value = 11

plc.write_list([symbol1, symbol2])

I don't see the point of passing a dictionary with symbols and values. Actually this would be redundant. My suggestion (let's use items again):

Suggested change

	data_names_and_values: Dict[Union[str, AdsSymbol], Any],
	items: Union[Dict[str, AdsSymbol], List[AdsSymbol]]

[stlehmann]

This might also be just a list of symbols

Suggested change

	symbols_and_values: Dict[AdsSymbol, Any],
	symbols: List[AdsSymbol],

[stlehmann]

List of symbols

[RobertoRoos]

@stlehmann , alright, that makes sense, no worries.
I think I'll just merge this into my own project, so it's the same to me in the end.

[chrisbeardy]

In the meantime @RobertoRoos you could look at making a separate pypi package as an extension of pyads, called pyads-symbols as Stefan suggested. This would allow you to work downstream of pyads and add lots of symbol features. This could make pyads easier to maintain in the future and give the symbols some more freedom. It can then always be merged back in later if deemed wise at the time.

[stlehmann]

In the meantime @RobertoRoos you could look at making a separate pypi package as an extension of pyads, called pyads-symbols

I very much support this idea. It has many benefits in terms of maintainability and it makes it easier to implement new features on top of pyads without bloating the pyads package too much. A first step could be to move the symbols module there and then go on and add new features successively.

Also it would solve the dilemma of two concurrent approaches in pyads by just adding the symbols approach as a convenient Add-On for anyone intereseted in a more object-oriented approach.