fuse -> ROS 2 fuse_variables: Port fuse_variables #288
Conversation
80808f3 to
7ebabbf
Compare
fuse_variables/CMakeLists.txt
Outdated
| ) | ||
|
|
||
| install( | ||
| FILES fuse_plugins.xml | ||
| DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION} | ||
| DESTINATION share/${PROJECT_NAME} |
There was a problem hiding this comment.
Same comment #287 (comment) about using pluginlib_export_plugin_description_file()
fuse_variables/CMakeLists.txt
Outdated
|
|
||
| ## fuse_variables library | ||
| add_library(${PROJECT_NAME} | ||
| add_library(${PROJECT_NAME} SHARED |
There was a problem hiding this comment.
same comment about leaving off SHARED and using ament_cmake_ros instead of ament_cmake #287 (comment)
fuse_variables/test/CMakeLists.txt
Outdated
| target_link_libraries(test_load_device_id ${PROJECT_NAME}) | ||
|
|
||
| ament_add_pytest_test(test_load_device_id_py "launch_tests/test_load_device_id.py" | ||
| PYTHON_EXECUTABLE "${_PYTHON_EXECUTABLE}" |
There was a problem hiding this comment.
Where is _PYTHON_EXECUTABLE coming from?
fuse_variables/test/CMakeLists.txt
Outdated
|
|
||
| install( | ||
| DIRECTORY launch_tests | ||
| DESTINATION share/${PROJECT_NAME}/test |
There was a problem hiding this comment.
I don't think tests should be installed
| { | ||
| exec_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>(); | ||
|
|
||
| spinning_ = true; |
There was a problem hiding this comment.
I don't think spinning is needed here, just exec_->cancel() to stop the thread.
| spinning_ = true; | ||
| spinner_ = std::thread([&](){ | ||
| auto context = rclcpp::contexts::get_global_default_context(); | ||
| while(context->is_valid() && spinning_) { |
There was a problem hiding this comment.
This loop and context getting can be removed and replaced with exec_->spin().
|
|
||
| @pytest.mark.launch_test | ||
| @launch_testing.markers.keep_alive | ||
| def generate_test_description(): |
There was a problem hiding this comment.
Use launch_pytest instead of launch_testing
beb5af7 to
57301bb
Compare
1696f10 to
6b3f4d2
Compare
| <build_depend>libceres-dev</build_depend> | ||
| <build_depend>fuse_core</build_depend> | ||
| <build_depend>pluginlib</build_depend> | ||
| <build_depend>rclcpp</build_depend> |
There was a problem hiding this comment.
is rclcpp required? I don't see it find_package()'d in the main CMakeLists.txt - it looks like it's just a <test_depend>.
There was a problem hiding this comment.
I made it a test_depend. Although, rclcpp gets included via fuse_core; if that's the case should I remove it entirely?
Likewise, outside of tests, rclcpp::Time gets included and used via #include <fuse_core/time.hpp>
There was a problem hiding this comment.
Ahh I only looked at the headers. I didn't check for types. Having a <build_depend> or <depend> on rclcpp seems fine to me then!
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
6b3f4d2 to
7a247f8
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
7a247f8 to
73ebc53
Compare
See: #276
Description
This PR ports the entirety of fuse_variables, including:
Other stuff like docs are in other PRs
Notes
Pinging @svwilliams for visibility.