Repository navigation
Usage of attribute properties obfuscates CAN IO operations #271
Description
Awsome library! It enabled me to talk to some CANopen hardware (CanFestival to be exact) with just a couple of lines straight out of the box! Kudos.
I have a topic that is not strictly an issue, but about methodology. I am currently working on an async port of this library (more to follow later). I have encountered a pattern in canopen where the attribute @property decorators and @x.setter setters is used a lot and that they sometimes ultimately end up doing CAN IO. Especially around class Variable. Examples are v = node.sdo['something'].raw or node.nmt.state = 'OPERATIONAL'. While I understand the intent, neatness and orthogonality of doing so, it kind of caught me by surprise, as I didn't expect attribute operations to have such side effects as to make IO requests.
With all due respect, I definitely think the canopen library should have better separation between which methods and mechanisms that generate CAN IO and which that does not. I would love to see that we rather use explicit names for IO-generating operations. To quote 2nd rule from the Zen of Python (PEP20): "Explicit is better than implicit."
In the work of migrating to async, getters and setters does not interact very well with async operation so it will have to be changed for this usage. It will need a paralell explicit get and set mechanism in order to be able to await the deep inner CAN IO. In that use case the user must know when IO happens because it must be awaited, while immediate functions acting locally does not.
Activity
christiansandberg commentedon Oct 11, 2021
Nice to hear that someone is working on an asynchronous version of this library. I've been thinking about it myself earlier but it felt too difficult making it work for synchronous as well as asynchronous in the same library. But that would be awesome if it could be done!
The attribute access was something that felt logical at the time. You could for example increment a number or do bit wise operations on a bit field using a single statement. Later on I realized it made it difficult to know where blocking operations were actually done so it is also something I regret to some extent.
So I wouldn't be against moving to using methods instead, but since users probably rely on these attributes a lot it would be preferable to keep it for backwards compatibility.
sveinse commentedon May 16, 2024
After a lot of discussion in PR #371 it was concluded that this project will continue with attribute access (using blocking IO) to avoid any code breakage, and that
get_*andset_*methods are not desired. I'm closing this issue.