-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enable message_filters support of python interfaces and tests #7
Conversation
- use new Subscriber - use ROS2 ROSTIME - enable test_all_funcs for cache interfaces - enable test_headerless to cache headerless message
- enable approximate policy test for messages with timestamp - enable approximate policy test for headerless messages - use ROS2 ROSTIME for time operations
@tfoote @mjcarroll @dirk-thomas maybe you have some extra feedback, please help review and kindly share, thanks ! |
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.
Thanks for the PR @gaoethan I've had a quick look through and generally it looks good. I made a handful of line by line comments on things. My main feedback overall is that there's a bunch of changes that are formatting only and if we could reduce them that would be great for readability of the patch and longer term maintenance.
@tfoote @dirk-thomas Any further feedback ? pls share and I can address it to close the gap ASAP, thanks ! |
@tfoote Can you pleae give this another review. Thanks. |
4998f09
to
80d9b2e
Compare
@tfoote I think that I have already resolved all the reviewing points from you except for the one of using of root logger. whether to use root logger or introduce a node but merely for using BTW, I suggest to merge and close this PR firstly if you guys agree the current changes, but open another new issue/PR to track the point of using root logger separately to avoid this PR pending for a long time. thanks for your great feedback. |
feedback addressed, new review requested
For the short term please at least use the public API rclpy.logging.get_logger with a useful name. https://github.com/ros2/rclpy/blob/master/rclpy/rclpy/logging.py#L40 This will at least make it a little bit more readable. However we've been looking at this elsewhere and for useful logging we will likely need to pass in the logger objects so that they can be remapped and otherwise grouped appropriately. So adding the logger to the init signature wouldn't be unreasonable. We can ticket that as a follow up. |
Now it's already changed to use the named logger with |
Pushed to https://github.com/ros2/message_filters/tree/refactor_py for easier CI running now: |
@tfoote I believe message_filters is not in the repos file so it will not be tested on regular CI. |
Link it to #2 for easy tracking |
@tfoote To run CI. |
This is the relevant failing bit on Windows:
|
The failure looks C++ related and independent of these Python changes. |
@mjcarroll validated image_transport working on Windows so it was known to be working. Unless the screenshots are of the new bionic env that looks just like windows. |
Image transport hasn't worked on CI on Windows, as it requires addition dependencies, but message_filters has worked in the past. @tfoote Do you want to look into this or do you want me to? |
I'm not going to have time to look at this until after ROSCon and vacation the week after. If you have time that would be great. |
We talked about this in the ROS2 meeting and since it's not a regression caused by this we'll merge this and ticket the regressions separately. |
I ported the message_filters ROS2 python support here and it mainly involves the following changes:
Subscriber
Cache
andApproximateTimeSynchronizer
rclpy
root logger for loggingTimeSynchronizer
ament_cmake_pytest