-
Notifications
You must be signed in to change notification settings - Fork 100
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
Added list read/write for symbols #268
base: master
Are you sure you want to change the base?
Conversation
@stlehmann I was hoping you might already have some insights here. |
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 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.
Agreed, I'll change that.
Yep, was thinking about that too. I'll change it. EDIT: And I do want to insist on the option to write values through
Also agreed. Will change.
Will change. 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 ( |
Pull Request Test Coverage Report for Build 1435643505
💛 - Coveralls |
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 The only other change to be considered from a user API perspictive is the 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
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 . |
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. |
Yes, a rebase would be good. You can rebase on master because the refactoring branch has already been merged. |
… on top of the refactor)
b73f92d
to
ec4e338
Compare
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:
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 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 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 |
…unctions compatible with tuples
Okay, I've created the above. I think this could work, I like the unit-purposes:
@stlehmann , thoughts? I'll add more docs and tests once we like the overall structure. |
@@ -571,36 +610,95 @@ def read_list_by_name( | |||
structure_defs = {} | |||
|
|||
if cache_symbol_info: | |||
new_items = [i for i in data_names if i not in self._symbol_info_cache] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i
always makes me think of a numeric key, so I wanted to replace with something more verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@stlehmann I apologize, I wanted to run |
We should look at adding this into the CI pipeline https://pypi.org/project/darker/ To address this issue. |
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
structure_defs: Optional[Dict[str, StructureDef]] = None, | ||
def read_list( | ||
self, | ||
symbols: Union[List[str], List[AdsSymbol]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this can be symbols and names we might just call them items
symbols: Union[List[str], List[AdsSymbol]], | |
items: Union[List[str], List[AdsSymbol]], |
return return_data | ||
|
||
def write_list( | ||
self, | ||
data_names_and_values: Dict[Union[str, AdsSymbol], Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
data_names_and_values: Dict[Union[str, AdsSymbol], Any], | |
items: Union[Dict[str, AdsSymbol], List[AdsSymbol]] |
|
||
def _write_list_of_symbols( | ||
self, | ||
symbols_and_values: Dict[AdsSymbol, Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be just a list of symbols
symbols_and_values: Dict[AdsSymbol, Any], | |
symbols: List[AdsSymbol], |
Either specify a dict of symbols, or first set the `_value` property of | ||
each symbol and then pass them as a list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of symbols
@stlehmann , alright, that makes sense, no worries. |
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. |
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 the
SAdsSymbolInfo
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: