-
Notifications
You must be signed in to change notification settings - Fork 666
socketcan: support use of SO_TIMESTAMPING for hardware timestamps#1882
socketcan: support use of SO_TIMESTAMPING for hardware timestamps #1882pevsonic wants to merge 1 commit intohardbyte:main from
Conversation
0ad787e to
deb6c79
Compare
e1a7050 to
84e002c
Compare
can/logger.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, interface specific parameters can be passed like --can-hardware-timestamps=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective, enabling hardware timestamping is a core feature I use and I imagine others might see it that way too. As an argument, it's availability is advertised by --help. Sending as you suggest would (I don't believe?) have any way for the user to see the args availability without understanding the mechanism and digging into the code to find the arg name. Additionally theres no validation on them so if I used --can-hard-we-ar-timestamps=True there'd be no detection of this, failure or feedback to the user.
Ive not got a big issue with this if you'd like me to drop the argument, but I think there's a solid case for leaving as an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only one interface supports this argument, i'd prefer to remove it. Otherwise we could add 100 more interface specific "important" arguments.
84e002c to
865866d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can/logger.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only one interface supports this argument, i'd prefer to remove it. Otherwise we could add 100 more interface specific "important" arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jayceslesar
commented
Apr 7, 2025
Any update on this?
pevsonic
commented
Apr 7, 2025
Any update on this?
I’d love to get it in as I’m using it in derived code we have here and I’m sure it’s helpful, but I’m not sure what changes I need to make to get accepted?
jayceslesar
commented
Apr 7, 2025
I’d love to get it in as I’m using it in derived code we have here and I’m sure it’s helpful, but I’m not sure what changes I need to make to get accepted?
I think you can just remove the argparse code you added and things should just work as the library knows to just pass in kwargs to the bustype by using the following:
parser.add_argument( "extra_args", nargs=argparse.REMAINDER, help="The remaining arguments will be used for the interface and " "logger/player initialisation. " "For example, `-i vector -c 1 --app-name=MyCanApp` is the equivalent " "to opening the bus with `Bus('vector', channel=1, app_name='MyCanApp')", )
So you would just pass
--can-hardware-timestamps=True
like @zariiii9003 commented in the initial review.
This is beneficial because this library acts as an abstraction over many bustypes abstracted over and made to look like CAN buses even though they are not and allows users to pass in arbitrary "kwargs" to the underlying interface that actually gets initialized without having to worry about what arguments actually live on the argument parser object
The rest just look like minor docs changes
The current implemenation of socketcan utilises SO_TIMESTAMPNS which only offers system timestamps. I've looked at how can-utils candump.c configures hardware timestamping and implemented this in socketcan as a new option 'can_hardware_timestamps' which is disabled by default to avoid any potential adverse impact on existing usage. Additionally modify logger.py to provide an additional '-H' flag in the same way that candump does in order to use this functionality.
865866d to
3f148c2
Compare
pevsonic
commented
Apr 8, 2025
I’d love to get it in as I’m using it in derived code we have here and I’m sure it’s helpful, but I’m not sure what changes I need to make to get accepted?
I think you can just remove the argparse code you added and things should just work as the library knows to just pass in kwargs to the bustype by using the following:
parser.add_argument( "extra_args", nargs=argparse.REMAINDER, help="The remaining arguments will be used for the interface and " "logger/player initialisation. " "For example, `-i vector -c 1 --app-name=MyCanApp` is the equivalent " "to opening the bus with `Bus('vector', channel=1, app_name='MyCanApp')", )So you would just pass
--can-hardware-timestamps=Truelike @zariiii9003 commented in the initial review.This is beneficial because this library acts as an abstraction over many bustypes abstracted over and made to look like CAN buses even though they are not and allows users to pass in arbitrary "kwargs" to the underlying interface that actually gets initialized without having to worry about what arguments actually live on the argument parser object
The rest just look like minor docs changes
That's really helpful thanks! I didn't know that's how kwargs works - while I can get by with Python, I'm really a 'C' guy..! Hopefully that should be enough for a merge now...
The current implemenation of socketcan utilises SO_TIMESTAMPNS which only offers system timestamps.
I've looked at how can-utils candump.c configures hardware timestamping and implemented this in socketcan as an option which is disabled by default to avoid any potential adverse impact on existing usage. This is using the same param 'use_system_timestamp' as established by neovi_bus.py.
I've also modified logger.py to provide an additional '-H' flag in the same way that candump does in order to use this functionality.