Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Generate node interfaces' getters and traits. #1069

Merged
merged 8 commits into from
Apr 22, 2020
Merged

Generate node interfaces' getters and traits. #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
092b269
first try of node like something
Karsten1987 May 9, 2019
20bbf8e
Do not hardcode node interfaces for generation.
hidmic Apr 16, 2020
235f8c1
Generate node interfaces getters.
hidmic Apr 16, 2020
8d21dce
touchups
Karsten1987 Apr 17, 2020
1b28df4
use rcpputils::remove_pointer
Karsten1987 Apr 17, 2020
b7441e3
create_timer takes shared pointers
Karsten1987 Apr 17, 2020
ad10603
Add nullptr checks in node interfaces' getters.
hidmic Apr 17, 2020
20795de
Fix node interface traits' tests.
hidmic Apr 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 57 additions & 3 deletions rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,19 +97,65 @@ configure_file(
COPYONLY
)
# generate header with logging macros
set(python_code
set(python_code_logging
"import em"
"em.invoke(['-o', 'include/rclcpp/logging.hpp', '${CMAKE_CURRENT_SOURCE_DIR}/resource/logging.hpp.em'])")
string(REPLACE ";" "$<SEMICOLON>" python_code "${python_code}")
string(REPLACE ";" "$<SEMICOLON>" python_code_logging "${python_code_logging}")
add_custom_command(OUTPUT include/rclcpp/logging.hpp
COMMAND ${CMAKE_COMMAND} -E make_directory "include/rclcpp"
COMMAND ${PYTHON_EXECUTABLE} ARGS -c "${python_code}"
COMMAND ${PYTHON_EXECUTABLE} ARGS -c "${python_code_logging}"
DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/logging.hpp.em.watch"
COMMENT "Expanding logging.hpp.em"
VERBATIM
)
list(APPEND ${PROJECT_NAME}_SRCS
include/rclcpp/logging.hpp)

file(GLOB interface_files "include/rclcpp/node_interfaces/node_*_interface.hpp")
foreach(interface_file ${interface_files})
get_filename_component(interface_name ${interface_file} NAME_WE)

# "watch" template for changes
configure_file(
"resource/interface_traits.hpp.em"
"${CMAKE_CURRENT_BINARY_DIR}/${interface_name}_traits.hpp.em.watch"
COPYONLY
)
set(python_${interface_name}_traits
"import em"
"em.invoke(['-D', 'interface_name = \\'${interface_name}\\'', '-o', 'include/rclcpp/node_interfaces/${interface_name}_traits.hpp', '${CMAKE_CURRENT_SOURCE_DIR}/resource/interface_traits.hpp.em'])")
string(REPLACE ";" "$<SEMICOLON>" python_${interface_name}_traits "${python_${interface_name}_traits}")
add_custom_command(OUTPUT include/rclcpp/node_interfaces/${interface_name}_traits.hpp
COMMAND ${CMAKE_COMMAND} -E make_directory "include/rclcpp/node_interfaces"
COMMAND ${PYTHON_EXECUTABLE} ARGS -c "${python_${interface_name}_traits}"
DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/${interface_name}_traits.hpp.em.watch"
COMMENT "Expanding interface_traits.hpp.em into ${interface_name}_traits.hpp"
VERBATIM
)
list(APPEND ${PROJECT_NAME}_SRCS
include/rclcpp/node_interfaces/${interface_name}_traits.hpp)

# "watch" template for changes
configure_file(
"resource/get_interface.hpp.em"
"get_${interface_name}.hpp.em.watch"
COPYONLY
)
set(python_get_${interface_name}
"import em"
"em.invoke(['-D', 'interface_name = \\'${interface_name}\\'', '-o', 'include/rclcpp/node_interfaces/get_${interface_name}.hpp', '${CMAKE_CURRENT_SOURCE_DIR}/resource/get_interface.hpp.em'])")
string(REPLACE ";" "$<SEMICOLON>" python_get_${interface_name} "${python_get_${interface_name}}")
add_custom_command(OUTPUT include/rclcpp/node_interfaces/get_${interface_name}.hpp
COMMAND ${CMAKE_COMMAND} -E make_directory "include/rclcpp/node_interfaces"
COMMAND ${PYTHON_EXECUTABLE} ARGS -c "${python_get_${interface_name}}"
DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/get_${interface_name}.hpp.em.watch"
COMMENT "Expanding get_interface.hpp.em into get_${interface_file}.hpp"
VERBATIM
)
list(APPEND ${PROJECT_NAME}_SRCS
include/rclcpp/node_interfaces/get_${interface_name}.hpp)
endforeach()

include_directories("${CMAKE_CURRENT_BINARY_DIR}/include")

add_library(${PROJECT_NAME}
Expand Down Expand Up @@ -514,6 +560,14 @@ if(BUILD_TESTING)
target_link_libraries(test_init ${PROJECT_NAME})
endif()

ament_add_gtest(test_interface_traits test/test_interface_traits.cpp
APPEND_LIBRARY_DIRS "${append_library_dirs}")
if(TARGET test_interface_traits)
ament_target_dependencies(test_interface_traits
"rcl")
target_link_libraries(test_interface_traits ${PROJECT_NAME})
endif()

ament_add_gtest(test_multi_threaded_executor test/executors/test_multi_threaded_executor.cpp
APPEND_LIBRARY_DIRS "${append_library_dirs}")
if(TARGET test_multi_threaded_executor)
Expand Down
4 changes: 2 additions & 2 deletions rclcpp/include/rclcpp/create_timer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ namespace rclcpp
template<typename CallbackT>
typename rclcpp::TimerBase::SharedPtr
create_timer(
node_interfaces::NodeBaseInterface * node_base,
node_interfaces::NodeTimersInterface * node_timers,
std::shared_ptr<node_interfaces::NodeBaseInterface> node_base,
std::shared_ptr<node_interfaces::NodeTimersInterface> node_timers,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is reasonable, but we will also break a lot of downstream packages if the old function returning a raw pointer is deleted.
IMO, we should deprecate instead of delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and we could so for rclcpp::create_timer, but not for the generated API (at least not seamlessly, template argument deduction won't be carried out for return types). #1035, on the other hand, doesn't drop API. Ultimately, there's a discussion about ownership that has to settled on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately, there's a discussion about ownership that has to settled on.

I think that it's up to the class requiring the interfaces to decide if they need shared ownership or just a non-owning pointer.

IMO, we should only provide getters returning shared pointers, and deprecate the old ones returning raw pointers (which can be replaced with get_shared_*_interface().get()).
But deleting that signature now will require a lot of changes in downstream packages.

I agree, and we could so for rclcpp::create_timer, but not for the generated API (at least not seamlessly, template argument deduction won't be carried out for return types)

I don't understand why it can be done in the generated API, I will check again tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not look pretty, but we should be able to write some empy conditions to check if we're generating getters for one of the previously existing interfaces and insert the deprecated API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and we could so for rclcpp::create_timer, but not for the generated API (at least not seamlessly, template argument deduction won't be carried out for return types).

Yes, template argument deduction won't work, but the same applies for the PR creating the files manually.
Both functions need to have a different name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precisely, that's what #1035 does. We can make this PR do the same, though it'll take two deprecation cycles for the std::shared_ptr flavor API to get the name we intended it to have here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this PR do the same, though it'll take two deprecation cycles for the std::shared_ptr flavor API to get the name we intended it to have here.

That's a great point.
If you want to continue with the API breaking version, I can help with reviewing the PRs needed in the downstream packages.
It would also be good to add a comment in the release notes.

If you want to continue with two functions with different names, and optionally deprecate the ones returning a raw pointer, I also approve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobperron we need an executive decision here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'd rather not go through two deprecation cycles to get the API we want. I think it will be okay to have this API break and document it in the release notes.

rclcpp::Clock::SharedPtr clock,
rclcpp::Duration period,
CallbackT && callback,
Expand Down
149 changes: 0 additions & 149 deletions rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp
View file
Open in desktop

This file was deleted.

149 changes: 0 additions & 149 deletions rclcpp/include/rclcpp/node_interfaces/get_node_timers_interface.hpp
View file
Open in desktop

This file was deleted.

Loading