-
Notifications
You must be signed in to change notification settings - Fork 55
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
Reorg namespaces #37
Reorg namespaces #37
Conversation
@flynneva Good call. Removed |
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.
looks great, i like the changes - just a few questions to follow up with
udp_driver/CMakeLists.txt
Outdated
@@ -42,11 +50,16 @@ include_directories(include) | |||
ament_auto_add_library(${PROJECT_NAME} SHARED | |||
src/udp_socket.cpp | |||
src/udp_driver.cpp | |||
examples/udp_driver_node.cpp) | |||
src/udp_driver_node.cpp) | |||
ament_target_dependencies(${PROJECT_NAME} "Boost") | |||
|
|||
target_compile_options(${PROJECT_NAME} PUBLIC "-O0" PRIVATE -Wno-sign-conversion -Wno-conversion) |
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.
for consistency with io_context
, should this be changed to
target_compile_options(${PROJECT_NAME} PUBLIC "-O0" PRIVATE -Wno-sign-conversion -Wno-conversion) | |
target_compile_options(${PROJECT_NAME} PUBLIC "-O0" PRIVATE -Wall |
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.
This line shouldn't be required at all because CMAKE_CXX_FLAGS
should cover that above. Removed.
ament_target_dependencies(${PROJECT_NAME} "Boost") | ||
|
||
target_compile_options(${PROJECT_NAME} PUBLIC "-O0" PRIVATE -Wno-sign-conversion -Wno-conversion) | ||
|
||
#rclcpp_components_register_node(udp_driver_node |
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.
assuming you are planning on "compontent-izing" this later?
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.
Yes. Sorry this got mixed in. Planning on creating two nodes: sender and receiver. They will both be components and managed lifecycle nodes.
udp_driver/CMakeLists.txt
Outdated
#PLUGIN "drivers::udp_driver::UdpDriverNode" | ||
#EXECUTABLE ${PROJECT_NAME}_node_exe | ||
#) | ||
|
||
if(BUILD_TESTING) | ||
find_package(ament_lint_auto REQUIRED) | ||
ament_lint_auto_find_test_dependencies() |
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.
same thing here, do we need the test/gtest_main.cpp
similar to what we did within io_context
?
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.
We don't. Removed here and removed the actual file.
Using just the namespace
drivers
in theudp_driver
package was too vague. This addsdrivers::common
for theio_context
package anddrivers::udp_driver
for theudp_driver
package.Also removes some unnecessary destructors.
@flynneva Would you be available to review this?