naoki-mizuno / ds4_driver

DualShock 4 driver for both ROS1 and ROS2
http://wiki.ros.org/ds4_driver
85 stars 53 forks source link

[ds4_driver_node.py-1] AttributeError: module 'rclpy' has no attribute 'Timer' #17

Closed geoporus1 closed 3 years ago

geoporus1 commented 3 years ago

Hello!

I am currently working on a project with a bigger robot and I am migrating the code base from ros1 to ros2. In the past I have successfully used your ds4_driver (for which I am eternally thankful :) ) with ros1 melodic without any issues whatsoever. However, now, when I am trying to use the version from the foxy-devel branch, the ds4_driver_node.py fails with the error [ds4_driver_node.py-1] AttributeError: module 'rclpy' has no attribute 'Timer'. This error looks really weird to me as when checking the latest rclpy 0.6.1 documentation it seems like there is no way to instantiate a new timer by just calling rclpy.Timer(). Moreso, when I import rclpy into a simple python terminal and try to do rclpy.Timer() I get the same error.

The weirdness does not stop here. In the ds4_driver_node.py there are 2 places where a Timer is created in this manner and one of them is a "oneshot" timer, which was a really neat feature to have in rospy but during my short development lately with ros2 I have seen that rclpy doesn't even support oneshot timers anymore.

I have forked this repo and tried to convert the timers to the format documented in the official rospy 0.6.1 documentation but just ran into more weird errors. The thing is that when I am running the driver from inside the docker container built with the provided dockerfile, the error does not appear when doing ros2 run ds4_driver ds4_driver_node.py but if i again just open a simple python terminal inside the container and try to import rclpy.Timer (as done in the script), I get the same error [ds4_driver_node.py-1] AttributeError: module 'rclpy' has no attribute 'Timer'.

All things found and said above lead me to believe that somehow the foxy version of the ds4_driver has been developed with an older version of rclpy which was still in early development. I would like to modify it to work with rclpy 0.6.1 (if this is the actual issue) but I have no clue what older version was used for developing the current version in the foxy-devel branch. Do you have any idea what version was used or why do I keep bumping into that error if my assumptions above are not correct? Any help will be appreciated and if needed I can help with bringing the foxy version up-to date with a pull-request, once I figure out why all these things are happening!

Cheers!

naoki-mizuno commented 3 years ago

Glad this package helped, and thanks for reporting! As for ROS2 support, to be honest I haven't had the time to test on my actual machine (at work we're still using melodic on Ubuntu 18.04 so... :( ).

I did confirm that if I run from inside the container (with the latest ros:foxy image) it runs OK. The version of rclpy is 1.0.5-1focal.20210415.032658:

$ root@897db662aac9:/opt/underlay_ws# dpkg -l | grep rclpy
ii  ros-foxy-rclpy                                  1.0.5-1focal.20210415.032658         amd64        Package containing the Python client.

It does look like 0.6.1 is a relatively old version from their release page. What version do you have on your machine?

geoporus1 commented 3 years ago

Hello @naoki-mizuno and thank you for the quick reply! Sorry for getting back to you this late about this, just today I have had the time to get back to it and work more at it.

I can confirm that the version of rclpy we are using in our Docker container is also 1.0.5-1, specifically it is this one: 1.0.5-1focal.20210423.012257.

After digging a bit further on this issue I want to say that actually even in the Docker container you provide for it, the issue I have found is there. You probably tested it by just running the container and then running the ds4_driver, but actually the issue appears when the use_standard_msgs parameter is set to True and the autorepeat_rate is set to something else than 0, I have used 0.1 for testing. When these 2 parameters are set like this, then this

# Use ROS-standard messages (like sensor_msgs/Joy)
        if self.use_standard_msgs:
            self.pub_report = self.node.create_publisher(Report, 'raw_report', 0)
            self.pub_battery = self.node.create_publisher(BatteryState, 'battery', 0)
            self.pub_joy = self.node.create_publisher(Joy, 'joy', 0)
            self.pub_imu = self.node.create_publisher(Imu, 'imu', 0)
            self.sub_feedback = self.node.create_subscription(JoyFeedbackArray, 'set_feedback', self.cb_joy_feedback, 0)

            if self._autorepeat_rate != 0:
                period = 1.0 / self._autorepeat_rate
                rclpy.Timer(rclpy.Duration.from_sec(period), self.cb_joy_pub_timer)
                self.node.create_timer(timer_period_sec=period, callback=self.cb_joy_pub_timer)

sequence from the ControllerRos class gets executed and it actually gets to the part where the timer gets created in a wrong way. Setting those parameters like that and then running the node gives the same error as I have encountered above: [ds4_driver_node.py-1] AttributeError: module 'rclpy' has no attribute 'Timer'.

After looking more into this it seems like there is no actual way to create a timer like that in rclpy, like it is done in that part of the code. There are 2 places in the controller_ros.py file where a timer is created in this manner and I believe this is an implementation issue. I am trying to replace them with the correct way of creating a timer shown in the latest rclpy documentation but some help on this would be appreciated. I am not sure I have the time now to actually do a fix for this so maybe you can also do it? Please let me know soon what do you think about these findings.

geoporus1 commented 3 years ago

Coming back with an update on this. I have managed to modify all the wrong usages of rclpy.Timer() in the package and tested them and they work correctly. I will submit now a pull request for the foxy-devel branch with my changes and if you see them fit and actually needed you can merge them to the main repo. 😃

naoki-mizuno commented 3 years ago

Since #18 is merged I'll close this issue. Thanks for your contribution!