enable message_filters support of python interfaces and tests #7

gaoethan · 2018-08-19T02:06:18Z

I ported the message_filters ROS2 python support here and it mainly involves the following changes:

replace Subsciber with ROS2 Subscriber
use ROS2 ROSTIME to support headerless messages for Cache and ApproximateTimeSynchronizer
use ROS2 rclpy root logger for logging
enable exact timestamp policy for with TimeSynchronizer
enable approximate timestamp policy for messages with header
enable approximate timestamp policy test for headerless messages
enable message cache for messages with header
enable message cache for for headerless messages
fix doc and coding style issue
enable the relevant tests with ament_cmake_pytest

- 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

gaoethan · 2018-08-21T03:48:55Z

@tfoote @mjcarroll @dirk-thomas maybe you have some extra feedback, please help review and kindly share, thanks !

tfoote

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.

CMakeLists.txt

package.xml

src/message_filters/__init__.py

test/test_message_filters_cache.py

gaoethan · 2018-08-31T01:20:23Z

@tfoote @dirk-thomas Any further feedback ? pls share and I can address it to close the gap ASAP, thanks !

dirk-thomas · 2018-09-06T17:20:38Z

@tfoote Can you pleae give this another review. Thanks.

gaoethan · 2018-09-07T14:22:19Z

@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 get_logger() instead of root logger, I personally don't prefer to do like that and it's too heavy, so it needs a final decision.

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

tfoote · 2018-09-11T23:12:35Z

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.

gaoethan · 2018-09-12T06:26:15Z

Now it's already changed to use the named logger with rclpy.logging.get_logger(). additionally, create the issue #8 for the follow-up optimization. thanks !

tfoote · 2018-09-12T22:51:36Z

Pushed to https://github.com/ros2/message_filters/tree/refactor_py for easier CI running now:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2018-09-12T22:57:26Z

@tfoote I believe message_filters is not in the repos file so it will not be tested on regular CI.

gaoethan · 2018-09-17T02:31:58Z

Link it to #2 for easy tracking

dirk-thomas · 2018-09-20T17:16:00Z

@tfoote To run CI.

tfoote · 2018-09-21T02:52:41Z

Linux
Linux-aarch64
macOS
Windows

mjcarroll · 2018-09-21T14:10:07Z

This is the relevant failing bit on Windows:

03:24:38   C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\bin\HostX86\x64\CL.exe /c /IC:\J\workspace\ci_windows\ws\src\ros\angles\include /IC:\J\workspace\ci_windows\ws\install\src\gtest_vendor\include /IC:\J\workspace\ci_windows\ws\install\include /nologo /W3 /WX- /diagnostics:classic /O2 /Ob2 /D WIN32 /D _WINDOWS /D NDEBUG /D "CMAKE_INTDIR=\"Release\"" /D _MBCS /Gm- /EHsc /MD /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /std:c++14 /Fo"message_filters-test_exact_time_policy.dir\Release\\" /Fd"message_filters-test_exact_time_policy.dir\Release\vc141.pdb" /Gd /TP /errorReport:queue C:\J\workspace\ci_windows\ws\src\ros\angles\test\test_exact_time_policy.cpp
03:24:38   test_exact_time_policy.cpp
03:24:38 C:\J\workspace\ci_windows\ws\src\ros\angles\include\message_filters/sync_policies/exact_time.h(119): error C2059: syntax error: 'template' [C:\J\workspace\ci_windows\ws\build\message_filters\message_filters-test_exact_time_policy.vcxproj]
03:24:38   C:\J\workspace\ci_windows\ws\src\ros\angles\test\test_exact_time_policy.cpp(156): note: see reference to function template instantiation 'message_filters::Connection message_filters::sync_policies::ExactTime<Msg,Msg,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType>::registerDropCallback<std::_Binder<std::_Unforced,void (__cdecl Helper::* )(void),Helper *>>(const C &)' being compiled
03:24:38           with
03:24:38           [
03:24:38               C=std::_Binder<std::_Unforced,void (__cdecl Helper::* )(void),Helper *>
03:24:38           ]
03:24:38   C:\J\workspace\ci_windows\ws\src\ros\angles\test\test_exact_time_policy.cpp(156): note: see reference to function template instantiation 'message_filters::Connection message_filters::sync_policies::ExactTime<Msg,Msg,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType,message_filters::NullType>::registerDropCallback<std::_Binder<std::_Unforced,void (__cdecl Helper::* )(void),Helper *>>(const C &)' being compiled
03:24:38           with
03:24:38           [
03:24:38               C=std::_Binder<std::_Unforced,void (__cdecl Helper::* )(void),Helper *>
03:24:38           ]
03:24:38 Done Building Project "C:\J\workspace\ci_windows\ws\build\message_filters\message_filters-test_exact_time_policy.vcxproj" (default targets) -- FAILED.

mikaelarguedas · 2018-09-21T16:17:58Z

The failure looks C++ related and independent of these Python changes.
@tfoote @mjcarroll Was the initial port tested on Windows?
CI with this additional repos file to see if master builds:

tfoote · 2018-09-25T11:04:46Z

@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.

mjcarroll · 2018-09-25T16:20:27Z

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?

tfoote · 2018-09-25T18:21:44Z

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.

tfoote · 2018-09-26T18:19:15Z

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.

gaoethan added 9 commits August 18, 2018 23:30

use ros2 subscriber with ros2 python client library

30dc421

replace ROS ROSTIME python interfaces with ROS2 ROSTIME

7846228

use rclpy logging and log severity level

72bb94c

enable directed.py test

0654f96

enable python test test_message_filters_cache.py

647f2d5

- use new Subscriber - use ROS2 ROSTIME - enable test_all_funcs for cache interfaces - enable test_headerless to cache headerless message

enable message filters python test test_approxsync.py

81a7056

- enable approximate policy test for messages with timestamp - enable approximate policy test for headerless messages - use ROS2 ROSTIME for time operations

enable python interfaces and tests to build and add dependency

b05f0c5

fix the issue that zip object is not subscriptable

efb4e66

fix doc and coding style issues

afceb40

tfoote added the in review Waiting for review (Kanban column) label Aug 19, 2018

gaoethan mentioned this pull request Aug 19, 2018

Integrate changes from intel/ros2_message_filters repository #2

Closed

dirk-thomas requested a review from tfoote August 23, 2018 17:18

tfoote previously requested changes Aug 24, 2018

View reviewed changes

gaoethan added 2 commits August 27, 2018 13:50

remove redundant dependency to std_msgs

b04c7ad

optimize doc and context wrapping

e44387e

dirk-thomas mentioned this pull request Sep 4, 2018

Port image transport ros2/ros2#531

Closed

8 tasks

use semantic datatypes with Time and Duration

80d9b2e

gaoethan force-pushed the refactor_py branch from 4998f09 to 80d9b2e Compare September 7, 2018 14:03

mikaelarguedas requested a review from tfoote September 11, 2018 00:56

gaoethan mentioned this pull request Sep 12, 2018

Optimize the using of logger for python class Cache and ApproximateTimeSynchronizer #8

Open

use named logger instead of root logger

b7826f3

tfoote approved these changes Sep 12, 2018

View reviewed changes

tfoote merged commit 3bfc64a into ros2:master Sep 26, 2018

tfoote removed the in review Waiting for review (Kanban column) label Sep 26, 2018

tfoote mentioned this pull request Sep 26, 2018

message_filters failing CI on Windows #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable message_filters support of python interfaces and tests #7

enable message_filters support of python interfaces and tests #7

gaoethan commented Aug 19, 2018

gaoethan commented Aug 21, 2018

tfoote left a comment

gaoethan commented Aug 31, 2018

dirk-thomas commented Sep 6, 2018

gaoethan commented Sep 7, 2018

tfoote commented Sep 11, 2018

gaoethan commented Sep 12, 2018

tfoote commented Sep 12, 2018

mikaelarguedas commented Sep 12, 2018

gaoethan commented Sep 17, 2018

dirk-thomas commented Sep 20, 2018

tfoote commented Sep 21, 2018

mjcarroll commented Sep 21, 2018

mikaelarguedas commented Sep 21, 2018

tfoote commented Sep 25, 2018 •

edited

Loading

mjcarroll commented Sep 25, 2018

tfoote commented Sep 25, 2018

tfoote commented Sep 26, 2018

enable message_filters support of python interfaces and tests #7

enable message_filters support of python interfaces and tests #7

Conversation

gaoethan commented Aug 19, 2018

gaoethan commented Aug 21, 2018

tfoote left a comment

Choose a reason for hiding this comment

gaoethan commented Aug 31, 2018

dirk-thomas commented Sep 6, 2018

gaoethan commented Sep 7, 2018

tfoote commented Sep 11, 2018

gaoethan commented Sep 12, 2018

tfoote commented Sep 12, 2018

mikaelarguedas commented Sep 12, 2018

gaoethan commented Sep 17, 2018

dirk-thomas commented Sep 20, 2018

tfoote commented Sep 21, 2018

mjcarroll commented Sep 21, 2018

mikaelarguedas commented Sep 21, 2018

tfoote commented Sep 25, 2018 • edited Loading

mjcarroll commented Sep 25, 2018

tfoote commented Sep 25, 2018

tfoote commented Sep 26, 2018

tfoote commented Sep 25, 2018 •

edited

Loading