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

add LifecycleNode::get_transition_graph along with service. #1472

Merged
merged 2 commits into from
Dec 2, 2020
Merged

add LifecycleNode::get_transition_graph along with service. #1472

merged 2 commits into from
Dec 2, 2020

Conversation

fujitatomoya
Copy link
Collaborator

address #1460

Signed-off-by: Tomoya.Fujita Tomoya.Fujita@sony.com

@fujitatomoya
add LifecycleNode::get_transition_graph along with service.
d462384 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya mentioned this pull request Nov 22, 2020
Closed
brawner
brawner reviewed Nov 22, 2020
View reviewed changes
Copy link
Contributor

@brawner brawner left a comment

Choose a reason for hiding this comment

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

I think it looks pretty good. Thanks for taking care of this. I just had a few comments.

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/test/benchmark/benchmark_lifecycle_node.cpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/test/test_state_machine_info.cpp Show resolved Hide resolved
@fujitatomoya
add concrete test cases for get_available_transitions with each prima…
a0adec8 
…ry state.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@brawner

requesting review one more time, thanks in advance!

@brawner
Copy link
Contributor

brawner commented Nov 24, 2020

Building with benchmarks on and testing --packages-select rclcpp_lifecycle. With benchmarks on, there should be warnings on aarch64 and windows that osrf_testing_tools_cpp cannot be found.

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

@brawner
Copy link
Contributor

brawner commented Nov 24, 2020

Benchmark results seem reasonable to me:

BenchmarkLifecycleNode/get_available_states                       1915 ns         1915 ns       365023 heap_allocations=1
BenchmarkLifecycleNode/get_available_transitions                  1522 ns         1522 ns       456442 heap_allocations=1
BenchmarkLifecycleNode/get_transition_graph                       2561 ns         2561 ns       273973 heap_allocations=1

@brawner
Copy link
Contributor

brawner commented Dec 2, 2020

The ci jobs all passed here. The warnings on aarch64 are the expected for benchmarks, and the ones on windows have been resolved. @wjwwood did you have any more comments or do you think this is good to merge?

wjwwood
wjwwood reviewed Dec 2, 2020
View reviewed changes
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp
std::vector<Transition> transitions;
transitions.reserve(state_machine_.current_state->valid_transition_size);

for (unsigned int i = 0; i < state_machine_.current_state->valid_transition_size; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this is because the data structure in C uses unsigned int, but really we should be using size_t here and there. Nothing to do for now, just a comment...

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally agreed

wjwwood
wjwwood approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@brawner brawner merged commit 0dc782a into ros2:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brawner brawner brawner approved these changes

@wjwwood wjwwood wjwwood approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@fujitatomoya @brawner @wjwwood