Open RobertoRoos opened 2 years ago
@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?
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:
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.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.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.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
.
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. 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.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:
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
.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. 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. 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!
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.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
pyads/connection.py | 59 | 61 | 96.72% | ||
pyads/pyads_ex.py | 44 | 47 | 93.62% | ||
<!-- | Total: | 103 | 108 | 95.37% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
pyads/connection.py | 1 | 96.42% | ||
<!-- | Total: | 1 | --> |
Totals | |
---|---|
Change from base Build 1358363211: | 0.2% |
Covered Lines: | 1689 |
Relevant Lines: | 1793 |
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.
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.
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.
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.
@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 .
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.
@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.
Yes, a rebase would be good. You can rebase on master because the refactoring branch has already been merged.
Ah this looks much better now. Thanks for rebasing. 👍
I'm not quite happy with the code structure and DRYness. I see roughly two options:
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)
)
# 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
AdsSymbol
directly to maintain a clear order of dependency.adsSumRead/Write
code so it works the AdsSymbol
class.
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!
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. :(
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.
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.
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.
@stlehmann I apologize, I wanted to run black
on my code, I didn't look at what it would do to other existing code.
We should look at adding this into the CI pipeline https://pypi.org/project/darker/
To address this issue.
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.
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.
Yes, I think a simple if ... else should do to separate the two cases.
And there it is. I started with updating the docs too.
@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.
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.
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.
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
andadsSumWrite
(assuming we don't want the redundant info lookup when using only the name). This is because those functions are based on theSAdsSymbolInfo
structs, which contain the integerdataType
, instead of a ctypes likeplc_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: