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

asynchronous operations #2

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

gabrik
Copy link

@gabrik gabrik commented May 10, 2022

Hi, as mentioned in #1 here is a PR that implements asynchronous operations for interacting using the Dynamixel2 protocol.

Features

  • Added the default sync feature for the current behavior and interface
  • Added async_smol feature for asynchronous operation within async_std
  • Added async_tokio feature for asynchronous operation within tokio
  • Feature gated all the differences between the implementations.

There is still (a lot) a code copy-pasted from sync to async implementation, mostly because of the different signatures and await here and there.

I tested it on my MacBook with a OpenCR board, I'm really open to feedbacks

Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
@de-vri-es
Copy link
Member

de-vri-es commented May 16, 2022

Hey, just wanted to let you know I haven't forgotten about this yet. I'm a bit busy, but I will definitely make time for this.

/edit: And of course, thanks for opening the PR! :)

@de-vri-es de-vri-es self-requested a review May 16, 2022 18:31
@de-vri-es de-vri-es self-assigned this May 16, 2022
@gabrik
Copy link
Author

gabrik commented May 17, 2022

I see that the build fails because of the --all-features flag in the CI configuration, I'll expand the configuration to build with different feature sets

Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
Signed-off-by: gabrik <gabriele.baldoni@gmail.com>
@gabrik
Copy link
Author

gabrik commented May 17, 2022

It should be fine now, tested locally, and on my fork, I had issues with the clippy action, but I guess it is because of the token.

@de-vri-es
Copy link
Member

de-vri-es commented May 18, 2022

I haven't done an in-depth review yet, but it looks like now, enabling a feature changes the existing Bus into an async one, right?

In general, enabling features should not break code that doesn't need/use the feature. This is especially important because two crates might use the same dependency with different sets of features. It's not very likely for this crate, but we should still support it.

So I suggest a slightly different approach: each feature should enable a new submodule that contains the implementation for that back-end. So the tokio feature would enable a crate::tokio module that contains an async Bus. And the smol feature would enable a crate::smol module with another Bus. This way, all features can be used together.

The current implementation could be moved to a sync feature so we can even make the serial2 dependency optional. It would be neat to move that into a sync module too, and re-expose it at the crate root with deprecation warnings that point to the new sync module.

@de-vri-es
Copy link
Member

Another thing that I was considering: it would be cool to see if we can still re-use the decode code. Maybe an internal decode module could help with that. And maybe the same goes for the encoding code.

The different async/sync implementations would then all call the same encode/decode functions, but take care of the reading/writing/buffer-clearing.

@gabrik
Copy link
Author

gabrik commented May 18, 2022

Thanks for the comments, I'll rearrange the code as you suggested

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.

2 participants