autowarefoundation / ros2_socketcan

A ROS2 wrapper around Linux SocketCAN
Apache License 2.0
119 stars 57 forks source link

Fix unit conversion bug in to_timeval() #24

Closed LukaJuricic closed 2 years ago

LukaJuricic commented 2 years ago

Small fix for a bug which caused incorrect waiting intervals when accessing the CAN socket.

kenji-miyake commented 2 years ago

@ljuricic @MarcelDudek Hello, thank you for fixing it. And we're sorry for the late reply. Would it be possible for you to show me how to reproduce the bug? For example, like this issue https://github.com/autowarefoundation/ros2_socketcan/issues/17.

@JWhitleyWork cc @mitsudome-r @xmfcx Would you review this PR since you are the original author?

LukaJuricic commented 2 years ago

Hello @kenji-miyake, here is how you can easily show the issue functionally.

  1. In terminal 1: start the CAN module, setup vcan0 interface and generate there random messages every 50ms
    sudo apt install can-utils
    sudo modprobe vcan
    sudo ip link add dev vcan0 type vcan
    sudo ip link set up vcan0
    cangen vcan0 -g 50
  2. In terminal 2: start top to check the CPU load
  3. In terminal 3: start a _socket_canreceiver node on vcan0 interface and theoretical 100ms polling period
    ros2 launch ros2_socketcan socket_can_receiver.launch.py interface:=vcan0 interval_sec:=1e-1

    The socket_can_receiver thread loads the CPU to 100% and receives a warning with 1Hz (due to the warning throttling)

    [socket_can_receiver_node_exe-1] [WARN] [1666111964.538596370] [socket_can_receiver]: Error receiving CAN message: vcan0 - Resource temporarily unavailable
  4. In terminal 3: stop the current process with Ctrl+C and restart the _socket_canreceiver by compensating the factor 1e6 bug
    ros2 launch ros2_socketcan socket_can_receiver.launch.py interface:=vcan0 interval_sec:=1e-7

    Now the socket_can_receiver does not load the CPU anymore by freewheeling and the warning is gone

kenji-miyake commented 2 years ago

I'm sorry to be late, and thank you for your information!

In terminal 3: start a socket_can_receiver node on vcan0 interface and theoretical 100ms polling period ros2 launch ros2_socketcan socket_can_receiver.launch.py interface:=vcan0 interval_sec:=1e-1 The socket_can_receiver thread loads the CPU to 100% and receives a warning with 1Hz (due to the warning throttling)

Unfortunately, I couldn't reproduce it. There was no warning.

ros2 launch ros2_socketcan socket_can_receiver.launch.py interface:=vcan0 interval_sec:=1e-7

Also, nor by this command. However, considering the bug that the timeout becomes too big, it would be the correct behavior.

And after applying your fix, there are warnings shown, but considering the interval, it seems to be the correct behavior.

$ ros2 launch ros2_socketcan socket_can_receiver.launch.py interface:=vcan0 interval_sec:=1e-7
[INFO] [launch]: All log files can be found below /home/kenji/.ros/log/2022-10-20-20-54-22-601501-DPC2108002-52466
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [socket_can_receiver_node_exe-1]: process started with pid [52480]
[socket_can_receiver_node_exe-1] [INFO 1666266862.707317456] [socket_can_receiver]: interface: vcan0
[socket_can_receiver_node_exe-1] [INFO 1666266862.707357515] [socket_can_receiver]: interval(s): 0.000000
[socket_can_receiver_node_exe-1] [INFO 1666266862.707367662] [socket_can_receiver]: use bus time: 0
[socket_can_receiver_node_exe-1] [WARN 1666266863.052225989] [socket_can_receiver]: Error receiving CAN message: vcan0 - CAN Receive Timeout
[socket_can_receiver_node_exe-1] [WARN 1666266864.052222490] [socket_can_receiver]: Error receiving CAN message: vcan0 - CAN Receive Timeout

But anyway, since the change looks reasonable, I'll approve it. And @xmfcx will review this after he comes back from ROSCon.

xmfcx commented 2 years ago

@ljuricic can you rebase this onto main? I don't seem to have the rights to do it myself on your fork.

xmfcx commented 2 years ago

@ljuricic or if you want, I can reopen another PR with a branch I rebased your commit.

LukaJuricic commented 2 years ago

@xmfcx sorry for the delay, it should be fixed now, let me know if there is anything else