socketcan: support use of SO_TIMESTAMPING for hardware timestamps #1882
Conversation
pevsonic
force-pushed
the
pevsonic/socketcan_enable_hw_timestamps
branch
2 times, most recently
from
0ad787e to
deb6c79
Compare
pevsonic
force-pushed
the
pevsonic/socketcan_enable_hw_timestamps
branch
3 times, most recently
from
e1a7050 to
84e002c
Compare
can/logger.py
Outdated
| @@ -46,6 +46,13 @@ def _create_base_argument_parser(parser: argparse.ArgumentParser) -> None: | |||
| choices=sorted(can.VALID_INTERFACES), | |||
| ) | |||
|
|
|||
| parser.add_argument( | |||
There was a problem hiding this comment.
This is not necessary, interface specific parameters can be passed like --can-hardware-timestamps=True
There was a problem hiding this comment.
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.
If only one interface supports this argument, i'd prefer to remove it. Otherwise we could add 100 more interface specific "important" arguments.
pevsonic
force-pushed
the
pevsonic/socketcan_enable_hw_timestamps
branch
from
84e002c to
865866d
Compare
| Use raw hardware timestamp for can messages if available instead | ||
| of the system timestamp. By default we use the SO_TIMESTAMPNS | ||
| interface which provides ns resolution but low accuracy. If your | ||
| can hardware supports it you can use this parameter to |
There was a problem hiding this comment.
| can hardware supports it you can use this parameter to | |
| CAN hardware supports it you can use this parameter to |
can/logger.py
Outdated
| @@ -46,6 +46,13 @@ def _create_base_argument_parser(parser: argparse.ArgumentParser) -> None: | |||
| choices=sorted(can.VALID_INTERFACES), | |||
| ) | |||
|
|
|||
| parser.add_argument( | |||
There was a problem hiding this comment.
If only one interface supports this argument, i'd prefer to remove it. Otherwise we could add 100 more interface specific "important" arguments.
| alternatively use the SO_TIMESTAMPING interface and request raw | ||
| hardware timestamps. These are much higher precision but will | ||
| almost certainly not be referenced to the time of day. There | ||
| may be other pitfalls to such as loopback packets reporting with |
There was a problem hiding this comment.
| may be other pitfalls to such as loopback packets reporting with | |
| may be other pitfalls too such as loopback packets reporting with |
| @@ -642,6 +656,17 @@ def __init__( | |||
| channel using :attr:`can.Message.channel`. | |||
| :param receive_own_messages: | |||
| If transmitted messages should also be received by this bus. | |||
| :param bool can_hardware_timestamps: | |||
| Use raw hardware timestamp for can messages if available instead | |||
There was a problem hiding this comment.
| Use raw hardware timestamp for can messages if available instead | |
| Use raw hardware timestamp for CAN messages if available instead |
|
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? |
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 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.
pevsonic
force-pushed
the
pevsonic/socketcan_enable_hw_timestamps
branch
from
865866d to
3f148c2
Compare
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.