Fix TypeError: unhashable type: 'Time' error

jdddog commented 5 years ago

Fixes #34

In the TimeSynchronizer class the following error is thrown because msg.header.stamp (builtin_interfaces/Time) is not hashable (perhaps it was in ROS 1), see below:

  File "/home/jamie/my_ros2_ws/install/message_filters/lib/python3.6/site-packages/message_filters/__init__.py", line 227, in add
    my_queue[msg.header.stamp] = msg
TypeError: unhashable type: 'Time'

This occurs at messagefilters/__init_\.py#L227.

I've just taken the approach used in ApproximateTimeSynchronizer to hash the timestamp, creating a rclpy Time object and using the nanoseconds field as the hash:

stamp = Time.from_msg(msg.header.stamp)
my_queue[stamp.nanoseconds] = msg

There is also a lot of code that was added to ApproximateTimeSynchronizer but not to TimeSynchronizer, perhaps these checks should be in TimeSynchronizer too, in case someone doesn't set a header?

        if not hasattr(msg, 'header') or not hasattr(msg.header, 'stamp'):
            if not self.allow_headerless:
                msg_filters_logger = rclpy.logging.get_logger('message_filters_approx')
                msg_filters_logger.set_level(LoggingSeverity.INFO)
                msg_filters_logger.warn("can not use message filters for "
                              "messages without timestamp infomation when "
                              "'allow_headerless' is disabled. auto assign "
                              "ROSTIME to headerless messages once enabling "
                              "constructor option of 'allow_headerless'.")
                return

            stamp = ROSClock().now()

sloretz commented 5 years ago

CI (testing just message_filters)

Linux
Linux-aarch64
macOS
Windows

sloretz commented 5 years ago

@jdddog It looks like there's a test failure in the time synchronizer test that might be related to this PR. Would you mind fixing it? https://ci.ros2.org/job/ci_linux/8279/testReport/junit/message_filters.test.directed/TestDirected/test_synchronizer/

jdddog commented 5 years ago

@jdddog It looks like there's a test failure in the time synchronizer test that might be related to this PR. Would you mind fixing it? https://ci.ros2.org/job/ci_linux/8279/testReport/junit/message_filters.test.directed/TestDirected/test_synchronizer/

Hey @sloretz, no worries and thanks for the feedback. Yeah sure, I can take a look on the weekend.