Skip to content
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

Add asyncio support [WIP] #359

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

sveinse
Copy link
Contributor

@sveinse sveinse commented Mar 27, 2023

This PR adds support of asyncio to canopen. The overall goals is to make canopen able to be used in either with asyncio or regular synchronous mode (but not at the same time) from the same code base.

Note that this work is still work in progress. This PR was created to discuss the specific solutions for async and non-async as mentioned in #272. This PR closes #272.

Current status until feature complete:

  • Implement ABlockUploadStream, ABlockDownloadStream and ATextIOWrapper for async in SdoClient. Not needed
  • Implement EcmyConsumer.wait() for async
  • Async implementation of LssMaster
  • Async implementation of BaseNode402
  • Implement async variant of Network.add_node()
  • Update unittests for async
  • Update documentation and examples

@sveinse
Copy link
Contributor Author

sveinse commented Mar 27, 2023

What is the best way to deal with Variable attributes? The feedback from #355 indicates that it is desirable to keep the attr based get/set mechanisms. However, this scheme cannot be used for async as there is no await mechanism built into @data.setter.

    var = param.data   # Non-async use
    var = await param.aget_data()   # Async use

My goal has been to keep the async and non-async uses of canopen as equal as possible, but this is an area where users will see a difference.

@acolomb
Copy link
Collaborator

acolomb commented Apr 25, 2024

There is an implementation for .read() and .write() on the underlying canopen.variable.Variable type. Could that be used in an async wrapper, or something similar added for async?

@sveinse
Copy link
Contributor Author

sveinse commented Apr 25, 2024

@acolomb I'm not precisely sure what you mean, so let me guess: You need separate async methods for read and right and they should only call their respective async setters/getters.

@acolomb
Copy link
Collaborator

acolomb commented Apr 25, 2024

Sorry, I was trying to answer your question in the previous comment about synchronous getters / setters used in the properties. What I meant was that instead we already do have methods to access a variable remotely. Those might be a better fit to mirror to the async world:

value = sdo_var.raw  # This is the usual, terse style recommended in the docs
value = sdo_var.read(fmt='raw')  # This also works already
value = await sdo_var.aread(fmt='raw')  # Feels like a natural enhancement for async

Note that the fmt='raw' argument can of course be left out, it's only needed for phys or desc variants. This would give us an easy way to support async without changing the existing synchronous property-based access.

@sveinse sveinse marked this pull request as draft April 26, 2024 05:31
* Add support for using dot (.) to address sub-index elements in the OD
* Make SdoVariable awaitable for fetching
* Fix SdoClient.aabort() that were missing
* Add __repr__ to the object classes
* Rename PDO Map to PdoMap
* Rename PDO Maps to PdoMaps
* Fix iterator in SdoArray
* Add alen() to SdoArray and SdoRecord
* Add __eq__ to SdoAbordedError
@sveinse
Copy link
Contributor Author

sveinse commented May 15, 2024

Sorry, I was trying to answer your question in the previous comment about synchronous getters / setters used in the properties. What I meant was that instead we already do have methods to access a variable remotely. Those might be a better fit to mirror to the async world:

value = sdo_var.raw  # This is the usual, terse style recommended in the docs
value = sdo_var.read(fmt='raw')  # This also works already
value = await sdo_var.aread(fmt='raw')  # Feels like a natural enhancement for async

Note that the fmt='raw' argument can of course be left out, it's only needed for phys or desc variants. This would give us an easy way to support async without changing the existing synchronous property-based access.

Sorry for late reply here. All of these methods are supported in the async port. In addition to await sdo_var.aget_raw() as mentioned. What if all the getter and setters-like functions and only rely on aread() and awrite() for async? Interesting. I need to contemplate that.

The areas of challenge for async currently are:

  • Variable: .data, .raw, .phys, .desc, .bits
  • BaseNode402: .op_mode, .statusword, .controlword, .state
  • PdoMap.read() and .save() - Because it contains logic among blocking IO. I'm not sure if my dual sync-async solution is elegant or if its smelly.
  • SdoClient - is an ongoing challenge. The *Stream() classes contains considerable logic and relies on RawIOBase mixin, all synchronous code. I'd rather not need to replicate all those methods and functions for async.

* Manual merge in improments from master
* Remove opinionated set_*() and get_*() for non-async calls
* Comment updates
@sveinse sveinse force-pushed the feature-asyncio branch 2 times, most recently from cdd5f6f to 9dd782e Compare May 18, 2024 20:14
sveinse and others added 3 commits May 18, 2024 22:20
…de in async use

* Remove duplicated async code in SdoClient
* Implemented to thread in aupload() and adownload()
* Removed async callbacks
* Temporary fix issue with truncated SDO uploads
* Temporary fix iterator on SdoArray
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add async support to canopen
2 participants