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

Controller Area Network (CAN) Take 4 #314

Merged
merged 10 commits into from
Oct 28, 2021
Merged

Conversation

timokroeger
Copy link
Contributor

Updated to the latest HAL changes:

  • Removed try_ prefix
  • Moved non-blocking implementation to nb module
  • Removed default blocking implementaions

Usage Example

stm32-fwupdate

Implementations

Updated for this PR:

Based on the very similar predecessor traits embedded-can v0.3 (diff v0.3 -> this PR)

Previous Discussion

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

Please see the contribution instructions for more information.

@adamgreig
Copy link
Member

adamgreig commented Sep 28, 2021

This looks good to me! I have a couple of thoughts but apologies if they've already been discussed in the previous threads...

  • Does new_unchecked actually need to be unsafe, i.e. might any memory safety rely on it later?
  • For CAN, might it make more sense to call it transmit and receive in both the blocking and nb traits, rather than read and write in blocking?
  • I saw you mentioned this on the old PR thread but it does seem like it would be nice to add some error types to Add error traits for communication interfaces #296 for things like invalid ID or data-too-long.
  • Could you add a CHANGELOG entry?

src/can/mod.rs Outdated Show resolved Hide resolved
therealprof
therealprof previously approved these changes Oct 19, 2021
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

@timokroeger If you could resolve the merge conflict, this seems to be ready to merge to us.

@eldruin
Copy link
Member

eldruin commented Oct 19, 2021

We agreed this is ready for merge. Could you rebase/resolve the conflict @timokroeger ?

Invalid CAN identifiers are still memory safe.
The blocking traits borrowed the naming convention from socketcan.
Rename the methods to `transmit()` and `receive()` to make them
consistent with the `nb` traits.
@timokroeger
Copy link
Contributor Author

Rebased to resolve the changelog conflict. The PR is ready to be merged.

@eldruin
Copy link
Member

eldruin commented Oct 21, 2021

The MSRV would need to be raised to Rust 1.46.0 in order to keep the StandardId::new as a const function.
Raising the MSRV would be fine with me.

@andresv
Copy link

andresv commented Oct 22, 2021

Just in right time. I would like to use it in https://github.com/esp-rs/esp-idf-hal.

@andresv
Copy link

andresv commented Oct 25, 2021

Is anybody opposing increasing MSRV to 1.46 to get this over finish line?

@eldruin
Copy link
Member

eldruin commented Oct 25, 2021

Opinions @therealprof, @ryankurte ?

src/can/id.rs Outdated

/// Creates a new `StandardId` without checking if it is inside the valid range.
#[inline]
pub const fn new_unchecked(raw: u16) -> Self {
Copy link

Choose a reason for hiding this comment

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

Does new_unchecked() actually need to be unsafe, i.e. might any memory safety rely on it later?

I'd actually argue that yes, it must be an unsafe function. Implementations and users of the traits should be able to rely on StandardId/ExtendedId always containing a valid ID. In my opinion, that's the entire point of using such wrapper types in the first place (ref "Parse, don't validate"). "Relying on" should then mean that even writing unsafe code based on this assumption should be possible - without the need for additional validation of the passed IDs.

Copy link

Choose a reason for hiding this comment

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

Hmm, for StandardId it is not marked as unsafe but for ExtendedId it is:

/// Creates a new `ExtendedId` without checking if it is inside the valid range.
#[inline]
pub const unsafe fn new_unchecked(raw: u32) -> Self {
Self(raw)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unsafe on ExtendedId was an oversight, pushed a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO both perspectives on the unsafe are appropriate here. Are there any "unsafe API guidelines" which could help us find a decision?

Copy link

Choose a reason for hiding this comment

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

My reasoning for functions needing to be unsafe is as follows: As soon as there exists a way to create an instance of StandardId containing an invalid ID from safe Rust, any possible downstream code which wants to rely on it containing a valid ID must add its own validation checks to ensure that it does. The is necessary for logic purposes and UB-purposes alike.

If, however, the API "contract" of the StandardId type is to only ever contain a valid ID and from safe Rust only abiding instances can be created, then downstream code can rely on this guarantee. Again, this is useful for logic reasons and UB avoidance alike.

As an example from the Rust stdlib, from_utf8_unchecked() is also unsafe. The reason is that some code handling strs downstream might want to optimize on the guarantee that str always contains valid UTF-8 data. Analogous, a CAN peripheral driver might want to optimize on the assumption that the u16 representing an ID will only ever have some of its lower 13 bits set. By keeping these functions safe, we're disallowing any such downstream code to be sound without additional code for validation.


Asking the other way around: What is gained from making these functions safe in the first place? For statically initializing a variable of type StandardId, it is irrelevant: The check in ::new() will be constant-folded away in probably all conceivable cases. The only place where ::new_unchecked() is really needed is when an ID is dynamically retrieved from elsewhere and you can guarantee that it has already been validated before. And even then, it is only relevant for edge-cases where the last bytes of memory and cycles of CPU time need to be optimized.

Copy link

Choose a reason for hiding this comment

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

I agree with @Rahix. It seems *_unchecked() APIs are typically marked as unsafe in rust std lib and also for example in heapless.

Are those CAN traits going to be in v1.0?

Copy link
Member

Choose a reason for hiding this comment

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

I am generally against marking functions as unsafe unless they actually lead to UB, and especially against doing it just because they seem like "scary" functions or some extra care (unrelated to UB) is required. In this case though I think @Rahix is right: the list of Rust UB explicitly includes "Producing an invalid value... The following values are invalid: Invalid values for a type with a custom definition of invalid values".

So, if we do reasonably define this type as only containing valid IDs, then it's right to say producing an invalid valid could cause UB, if some other code later depended on this property in order to be sound. I can't think of any such situations but I guess they could exist.

Copy link

@andresv andresv Oct 26, 2021

Choose a reason for hiding this comment

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

What if we just remove new_unchecked() for now?

One downside is that using unwrap and expect introduces format bloat and makes binaries bigger and in this case there is also a little performance hit by always checking those values.

let canid = read_can();
let id = ExtendedId::new(canid).expect("always valid");
vs
let id = unsafe { ExtendedId::new_unchecked(canid) };

Invalid CAN identifiers are still memory safe.
ryankurte
ryankurte previously approved these changes Oct 25, 2021
Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

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

lgtm, MSRV bump seems fine to me, and real excited to try this...

thanks everyone for the huuge effort in prior PRs and to get this over the line!

This reverts commit b2d00d1 and
31eabd5.

Mark the constructor as unsafe again to prevent UB in safe code that assumes the CAN IDs to contain valid data.
This is in line with functions like `from_utf8_unchecked()` from the std library.
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

@timokroeger Could you also do the following?

  • Add a couple of unit tests for the functions that have an implementation.
  • Raise the MSRV to 1.46.0

@therealprof
Copy link
Contributor

Looks good to me as well. 👍🏻

@chemicstry
Copy link

A bit late, but what about the timestamp API, which was proposed in earlier PR? I would like to use this for UAVCAN, which needs timestamp support.

I guess it can also be added later as a non breaking change to not block this merge with discussions on wether to use embedded-time.

@andresv
Copy link

andresv commented Oct 27, 2021

rtic folks found that embedded-time moves too slowly is too complicated and tries to do too much, therefore korken89 created https://github.com/korken89/const-embedded-time which is not yet released.

It would be better if timestamp API is added later. It might take a lot of time to figure out what is the best way for that.

@ryankurte
Copy link
Contributor

ryankurte commented Oct 27, 2021

I guess it can also be added later as a non breaking change to not block this merge with discussions on wether to use embedded-time.

It would be better if timestamp API is added later. It might take a lot of time to figure out what is the best way for that.

yep, definitely a few other dependencies so we won't include this here. feel free to open a new issue / PR to discuss this ^_^

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Alright, let's get this merged. I will add the couple of unit tests in a separate PR.
Thank you @timokroeger for your work!
Thank you as well to everybody else involved in all the previous PRs and discussions!

bors merge

@bors bors bot merged commit 45d3170 into rust-embedded:master Oct 28, 2021
@burrbull
Copy link
Member

Should we backport this to 0.2?

@eldruin
Copy link
Member

eldruin commented Oct 28, 2021

This could be backported if there is interest. We would need to remove the const in the const fns and see if there are other things that do not build with Rust 1.31.0.

@andresv
Copy link

andresv commented Oct 28, 2021

I guess it also depends on when 1.0 is going to be released. If this happens rather quickly then I think it is not needed to backport it. We actually want that 1.0 is adopted and all the stuff is updated from 0.2 to 1.0.
Otherwise we have python 2 vs 3 situation.

@burrbull
Copy link
Member

I guess it also depends on when 1.0 is going to be released. If this happens rather quickly then I think it is not needed to backport it. We actually want that 1.0 is adopted and all the stuff is updated from 0.2 to 1.0. Otherwise we have python 2 vs 3 situation.

Sure. But we need several implementations to be sure all work as expected before stabilize in 1.0. I see 0.2 as test playground, not alternative implementaion. Nothing similar to python2/3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants