Demo nodes cpp native #164
Conversation
demo_nodes_cpp_native/CMakeLists.txt
Outdated
| string(REPLACE ";" "_" exe_list_underscore "${tutorial_executables}") | ||
| configure_file( | ||
| test/test_executables_tutorial.py.in | ||
| test_${exe_list_underscore}${target_suffix}.py.configured |
There was a problem hiding this comment.
Where does target_suffix come from?
demo_nodes_cpp_native/package.xml
Outdated
| <buildtool_depend>ament_cmake</buildtool_depend> | ||
|
|
||
| <build_depend>rclcpp</build_depend> | ||
| <build_depend>rmw_implementation_cmake</build_depend> |
There was a problem hiding this comment.
What is this dependency used for (I didn't see it being used in the cmake code)?
demo_nodes_cpp_native/package.xml
Outdated
| <build_depend>std_msgs</build_depend> | ||
|
|
||
| <exec_depend>rclcpp</exec_depend> | ||
| <exec_depend>rmw_implementation</exec_depend> |
There was a problem hiding this comment.
Is this needed too? I see it in the other demos, but shouldn't this be provided transitively by rclcpp (I don't see this package uses it directly)?
demo_nodes_cpp_native/package.xml
Outdated
|
|
||
| <exec_depend>rclcpp</exec_depend> | ||
| <exec_depend>rmw_implementation</exec_depend> | ||
| <exec_depend>rosidl_default_runtime</exec_depend> |
There was a problem hiding this comment.
Again, shouldn't this be brought in implicitly by the message packages this package depends on (like std_msgs in this case)? Put another way, if this package did not use messages would you depend on this package?
|
|
||
|
|
||
| def setup(): | ||
| os.environ['OSPL_VERBOSITY'] = '8' # 8 = OS_NONE |
| exit_handler = ignore_exit_handler | ||
| exit_on_match = False | ||
|
|
||
| filtered = get_default_filtered_prefixes() + [b'service not available, waiting again...'] |
There was a problem hiding this comment.
I don't think this package uses services?
| rcl_node_t * rcl_node = node->get_node_base_interface()->get_rcl_node_handle(); | ||
| rmw_node_t * rmw_node = rcl_node_get_rmw_handle(rcl_node); | ||
| eprosima::fastrtps::Participant * p = rmw_fastrtps_cpp::get_participant(rmw_node); | ||
| printf("eprosima::fastrtps::Participant * %zu\n", reinterpret_cast<size_t>(p)); |
There was a problem hiding this comment.
Does this actually prove anything? Maybe it would be better to call a function on the participant, something like getting the name out of it or the entity id? Then at least you know the pointer is valid.
There was a problem hiding this comment.
I think it is sufficient to check that we get a valid pointer to a FastRTPS specific type.
I updated the regex to ensure that the received pointer is not 0 - otherwise the test would also pass for that case.
dirk-thomas
force-pushed
the
demo_nodes_cpp_native
branch
from
9a19750
to
4d85c33
Compare
|
I addresses all of the other comments by basically removing the pointed out part since it was copy-n-pasted from another demo package. Sorry for the noise and not cleaning it up earlier. |
dirk-thomas
force-pushed
the
demo_nodes_cpp_native
branch
from
4d85c33
to
5398122
Compare
|
Together with the workaround from ros2/rmw_fastrtps#148 this demo finally works on Windows: CI builds with FastRTPS only:
CI builds with both FastRTPS and Connext
CI builds with only Connext
A bit extensive on the builds but better be safe than sorry 😉 |
|
Is the goal to have this demo only for Fast-RTPS or also have a Connext version of it ? |
|
I don't plan to extend this demo to include a Connext example. But it could certainly be added if anyone would like to contribute it. Therefore I wouldn't change the package name either. |
dirk-thomas
force-pushed
the
demo_nodes_cpp_native
branch
from
a5fc0bb
to
8be3203
Compare
Maybe a readme could be useful in that case ? explaining what the package does and the fact the it works only for fast-RTPS at the moment |
While more information about a package is always helpful most of our other demo packages don't have a README either. The description in the manifest of each package tries to cover the scope / goal / purpose of a package ( ). Please feel free to create a PR if you think more information is necessary in this case. |
Demonstrate how to access the RMW specific native handles exposed in ros2/rmw_fastrtps#145 and ros2/rmw_fastrtps#146.
libfastrtps-1.5.lib. According to the linkedrmw_fastrtps_cpp_LIBRARIESit is supposed to link againstC:/J/workspace/ci_windows/ws/install/fastrtps/lib/fastrtps-1.5.lib. I don't see where the additionallibprefix is coming from (the file itself exists but the linker probably doesn't know where to find it). Any ideas what is going wrong here would be appreciated.