Skip to content

add LifecycleNode::get_transition_graph along with service.#1472

Merged
brawner merged 2 commits intoros2:masterfrom
fujitatomoya:topic-20201122-get_transition_graph
Dec 2, 2020
Merged

add LifecycleNode::get_transition_graph along with service.#1472
brawner merged 2 commits intoros2:masterfrom
fujitatomoya:topic-20201122-get_transition_graph

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya commented Nov 22, 2020

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
Copy Markdown
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.

@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
Copy Markdown
Collaborator Author

fujitatomoya commented Nov 24, 2020

@brawner

requesting review one more time, thanks in advance!

@brawner
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@wjwwood wjwwood Dec 2, 2020

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
Copy Markdown
Contributor

@brawner brawner Dec 2, 2020

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
Copy Markdown
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

@wjwwood wjwwood wjwwood approved these changes

+1 more reviewer

@brawner brawner brawner approved these changes

Reviewers whose approvals may not affect merge requirements

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