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

Demos using composition #375

Merged
merged 15 commits into from
Jul 29, 2019
Merged

Demos using composition #375

merged 15 commits into from
Jul 29, 2019

Conversation

skucheria
Copy link
Member

@skucheria skucheria commented Jul 15, 2019 •
edited
Loading

Addressing #325

@skucheria
demo_node_cpp_native uses composition
9cf59c6 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria skucheria changed the title demo_node_cpp_native uses composition demo_node_cpp_native using composition Jul 15, 2019
@skucheria
converting demo_nodes_cpp to composition
4d1d93f 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
added windows symbol export cmake statement
ddcb5a7 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
added windows symbol cmake command
2966783 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria skucheria changed the title demo_node_cpp_native using composition demo_node_cpp_native and demo_nodes_cpp using composition Jul 15, 2019
@skucheria
refactor for cmake functions
938c5be 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
cmake changes
8384239 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
more changes
9c5d4ce 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
more demos converted
2afc9b7 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
more demos to composition
09d26f7 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
pendulum demos
050647a 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
fixes
32eb315 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria
fix for pendulum
d90a6be 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria skucheria force-pushed the kucheria/compostion_demos branch 2 times, most recently from 5890e65 to 08d15ba Compare July 22, 2019 23:46
@skucheria
added windows check
c5150d5 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria skucheria force-pushed the kucheria/compostion_demos branch 2 times, most recently from d8f49fb to 15fb12d Compare July 23, 2019 16:57
@skucheria
removed composition changes to lifecycle
5600fe1 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
@skucheria skucheria force-pushed the kucheria/compostion_demos branch 3 times, most recently from 901bb7c to bfba9a4 Compare July 24, 2019 18:21
@skucheria skucheria changed the title demo_node_cpp_native and demo_nodes_cpp using composition Demos using composition Jul 24, 2019
@skucheria skucheria marked this pull request as ready for review July 24, 2019 18:23
@skucheria
Copy link
Member Author

skucheria commented Jul 24, 2019 •
edited
Loading

Image_tools CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@skucheria
Copy link
Member Author

Demo_nodes_cpp_native CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

jacobperron
jacobperron reviewed Jul 25, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

This is a partial review, just looking at demo_nodes_cpp.

demo_nodes_cpp/CMakeLists.txt Outdated Show resolved Hide resolved
demo_nodes_cpp/CMakeLists.txt Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/parameter_blackboard.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/services/add_two_ints_client_async.cpp
print_usage();
rclcpp::shutdown();
} else {
std::string tmptopic = get_command_option(args, "-s");
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the CLI args are only used to set the service name, which can be accomplished with a remap instead. Therefore, I think we can remove all the logic related to parsing CLI args.
But, if we still plan to backport these changes then I would leave it as-is and remove it in a follow-up.

demo_nodes_cpp/src/services/add_two_ints_client_async.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/services/add_two_ints_server.cpp
print_usage();
rclcpp::shutdown();
} else {
std::string tmptopic = get_command_option(args, "-s");
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above. All of the CLI argument parsing logic can be removed now or in a follow-up if we want to backport this PR.

demo_nodes_cpp/src/topics/listener.cpp
print_usage();
rclcpp::shutdown();
} else {
std::string tmptopic = get_command_option(args, "-t");
Copy link
Member

Choose a reason for hiding this comment

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

Same comment, we should prefer using CLI remaps than options.

@skucheria
revisions
f4b8a7d 
Signed-off-by: Siddharth Kucheria <kucheria@usc.edu>
jacobperron
jacobperron approved these changes Jul 29, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@skucheria
Copy link
Member Author

demo_nodes_cpp:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@skucheria
Copy link
Member Author

image_tools"

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@skucheria
Copy link
Member Author

demo_nodes_cpp_native:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@skucheria skucheria merged commit 86f16de into master Jul 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the kucheria/compostion_demos branch July 29, 2019 23:06
dirk-thomas
dirk-thomas reviewed Jul 29, 2019
View reviewed changes
demo_nodes_cpp/CMakeLists.txt Show resolved Hide resolved
demo_nodes_cpp/src/parameters/even_parameters_node.cpp
{
// Force flush of the stdout buffer.
setvbuf(stdout, NULL, _IONBF, BUFSIZ);
Copy link
Member

@dirk-thomas dirk-thomas Jul 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't recommend this within a node. Simply because what other nodes specify might collide. This should happen on the container level instead.

demo_nodes_cpp/src/parameters/set_and_get_parameters.cpp Show resolved Hide resolved
@skucheria skucheria mentioned this pull request Jul 30, 2019
Merged
@skucheria skucheria mentioned this pull request Jul 31, 2019
Merged
@nuclearsandwich nuclearsandwich mentioned this pull request Aug 13, 2019
Closed
@ivanpauno ivanpauno mentioned this pull request Dec 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

@dirk-thomas dirk-thomas dirk-thomas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@skucheria @jacobperron @dirk-thomas