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.

Sign up for GitHub

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

Jump to bottom

Add async API for CAN #585

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add async API for CAN #585

wants to merge 5 commits into from

Conversation

liamkinne
Copy link
Contributor

Adds a new CAN trait with both async and fallback non-blocking methods.

Module is named asynch to avoid a collision with the async keyword. Alternatively a separate crate embedded-can-async could be published.

@liamkinne liamkinne requested a review from a team as a code owner April 2, 2024 13:53
burrbull
burrbull reviewed Apr 2, 2024
View reviewed changes
embedded-can/src/asynch.rs Outdated

/// Tries to put a frame in the transmit buffer.
/// If no space is available in the transmit buffer `None` is returned.
fn try_transmit(&mut self, frame: &Self::Frame) -> Option<Result<(), Self::Error>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inspired by a pattern I've seen around like here: https://docs.rs/rtic-sync/latest/rtic_sync/arbiter/struct.Arbiter.html

Can just get rid of it, but I think it's useful.

@adamgreig
Copy link
Member

Thanks for the PR! We had a brief discussion about it in the weekly meeting today. Having the trait in the asynch module is fine. I think the only remaining question is whether to keep the try_x() methods, move to to another trait, or delete them entirely.

It looks like either the crate's MSRV (and related CI tests) will need updating, or the trait needs to be gated, to get CI passing, too.

@liamkinne
Copy link
Contributor Author

@adamgreig thanks for the update.

I'm still strongly for keeping the try methods. The use case is that when libraries (bxcan, fdcan, etc) implement these traits they can only implement one trait at a time so they will have a blocking client and an async client. If you're using the async client but need to do something in a non-async context (think RTIC hardware task) you need something to fallback to that doesn't require .await.

It is very similar to the ideas around nb but more like "hey this method will work first time most of the time, unless your buffer is full which you can chose how to handle".

@reneherrero reneherrero mentioned this pull request Apr 30, 2024
Open
@liamkinne
Copy link
Contributor Author

@adamgreig in the interest of getting this through so it can start being used downstream, I've removed the try methods.

@adamgreig
Copy link
Member

Thanks for the prompt and the update. I'll add this to the agenda for discussion in the weekly meeting this Tues.

Dirbaio
Dirbaio reviewed Jul 2, 2024
View reviewed changes
embedded-can/src/lib.rs Outdated
// We don't immediately remove them to not immediately break older nightlies.
// When all features are stable, we'll remove them.
#![cfg_attr(nightly, allow(stable_features, unknown_lints))]
#![cfg_attr(nightly, feature(async_fn_in_trait, impl_trait_projections))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these cfg_attrs do nothing without this in build.rs.

These were for compat with older nightlies before AFIT was stable. Now that AFIT has been stable for a few rust versions since 1.75, i'd just not put them, and support stable 1.75+ only.

@liamkinne
Revert "fix ci warning"
37f4be6 
This reverts commit 81bc367.
@liamkinne liamkinne force-pushed the feature/embedded-can-async branch from b8f8ec1 to 37f4be6 Compare July 4, 2024 10:54
@elpiel
Copy link

elpiel commented Dec 6, 2024

Are there any blockers for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@burrbull burrbull burrbull left review comments

@Dirbaio Dirbaio Dirbaio left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liamkinne @adamgreig @elpiel @Dirbaio @burrbull