- Notifications
You must be signed in to change notification settings - Fork81
RFC: Add support for timestamps#77
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b890db0
toc5e31bb
Comparejr1221 commentedOct 10, 2024
If you need someone to test this PR for stabilty or performance please let me know. |
dbadfc4
toc10b400
Comparepoeschel commentedOct 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thank you for this offer. I am quite certain, that this works and it will be as performant as everything else in socketcan-rs. You're welcome to test it and give feedback
|
tuna-f1sh commentedFeb 17, 2025
@fpagliughi this seems like a good stab at timestamp support and closing#22 after 3 years! If this was rebased to 3.5 release, what do you think? Or do you have other ideas/WIP? |
This adds the possibility to retrieve a timestamp from the kernel whenreceiving can frames.The way this works is you have to open a special Socket, calledCanSocketTimestamp that yields a SystemTime along with the CanFrameinstead of only the CanFrame. Have a look at the examples.
Co-authored-by: Sven Bachmann <dev@mcbachmann.de>
Co-authored-by: Sven Bachmann <dev@mcbachmann.de>
358257f
to982d4f0
Comparefrom_ref is used in the timestamping code and supported since Rust1.76.0
Uh oh!
There was an error while loading.Please reload this page.
This pull-request adds support for receiving can frames with timestamps from the kernel.
It tries to solve issue#22. It shows, that the concept works. See the examples.
I chose to provide new socket types, which provide a tuple as a return type, when
read_frame
'ing from the socket. This tuple contains aCanFrame
and aOption<SystemTime>
. While providing these Sockets requires a lot of boilerplate code for implementing the Traits, I think this way feels very natural from a socketcan-rs user perspective.There are however some points I would like to discuss:
(CanFrame, Option<SystemTime>)
I got this from the TODO inside of src/socket.rs. Maybe it is better to provide a named struct here? Something likeCanFrameTimestamp
that then contains theCanFrame
and theSystemTime
.struct timespec
on the C side. The nix crate hasTimeSpec
for this, but I think returning a nix crate type here is maybe not the right thing? What do we have in std? Well, there isDuration
,Instant
andSystemTime
. From these three,SystemTime
felt the best, but I am not sure.SystemTime
in anOption
, because, well is there something that can go wrong while obtaining the timestamp? Maybe when we do not have acmsg
? Can this fail? What to do, when this fails? I don't know. For now, this is an Option. Is this a good idea?IoSliceMut
, that I get fromrecvmsg
into alibc::canframe
without copying bytes around. Copying of the bytes does happen when constructing aCanFrame::from(libc_f)
a little later. Is this a feasible way here? Can anybody come up with something better? Maybe something more idiomatic rust?ptr::from_ref
which is not accepted by the current CI. I know this. This can also be done by another pointer-casting-dereference-dance, which looks very ugly. Maybe when anybody comes up with a better solution for 5., this can be eliminated along the way? If not, I would opt to raiserust-version
and CI to 1.76 which bringsstd::ptr::from_ref
out of nightly. I added a patch raising the rust-version as test. This can be removed if there are any objections.CanSocketTimestamp
andCanFdSocketTimestamp
. Does this sound right?open_with_timestamp
with an enumTimestampingMode
, to choose between hardware and software timestamping. Any comments on this?open_addr
is doingTimestampingMode::Software
as a default. Is this sane?I could only test software timestamps so far. I try to set up something to test hardware timestamps as well.