Skip to content
This repository has been archived by the owner on Jun 10, 2021. It is now read-only.

Refactor PeriodicMeasurementNode to inherit from LifecycleNode #72

Merged
merged 11 commits into from Jan 17, 2020
Merged

Refactor PeriodicMeasurementNode to inherit from LifecycleNode #72

merged 11 commits into from Jan 17, 2020

Conversation

dabonnie
Copy link
Contributor

Inherit from LifecycleNode in order to use the existing state machine for a user to start and stop collection. This PR does the following:

  • refactor PeriodicMeasurementNode to inherit from LifecycleNode
  • refactor unit tests to manually call lifecycle transitions
  • refactor launch entry points to automatically activate and shutdown

@dabonnie dabonnie self-assigned this Jan 15, 2020
@dabonnie
Copy link
Contributor Author

Will remove from draft after adding support for the lifecycle message types (instead of comparing expected lifecycle states to integer values, compare to the actual name).

@dabonnie
Copy link
Contributor Author

dabonnie commented Jan 15, 2020 •
edited

Manually tested the launch script using ros2 topic echo /system_metrics


measurement_source_name: linuxMemoryCollector
metrics_source: system_memory_percent_used
window_start: 
  sec: 1579124931         
  nanosec: 84125089
window_stop:
  sec: 1579124991
  nanosec: 83888960
statistics:
- data_type: 1
  data: 43.977439880371094
- data_type: 3
  data: 44.28981018066406
- data_type: 2
  data: 43.68082046508789
- data_type: 5
  data: 59.0
- data_type: 4
  data: 0.10391871631145477
---
measurement_source_name: linuxCpuCollector
metrics_source: system_cpu_percent_used
window_start:
  sec: 1579124931
  nanosec: 84741476
window_stop:
  sec: 1579124991
  nanosec: 84595369
statistics:
- data_type: 1
  data: 6.064512729644775
- data_type: 3
  data: 13.94169807434082
- data_type: 2
  data: 3.7809648513793945
- data_type: 5
  data: 58.0
- data_type: 4
  data: 1.8589410781860352
---

@mm318
Copy link
Member

mm318 commented Jan 15, 2020

Are LifecycleNodes composeable? https://answers.ros.org/question/333498/composable-lifecycle-node/

@dabonnie
Copy link
Contributor Author

Are LifecycleNodes composeable? https://answers.ros.org/question/333498/composable-lifecycle-node/

Reading the last part of the post, I think we are OK given that this PR manually invokes the state transitions. We could do the same for the composable process nodes in their constructor (autostart flag?).

@dabonnie dabonnie marked this pull request as ready for review January 16, 2020 00:52
piraka9011
piraka9011 approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

After hallway discussion, LGTM 👍

@dabonnie
Refactor PeriodicMeasurementNode to inherit from LifecycleNode
d468e35 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Refactor launch entry points to automatically activate and shutdown
c5a64e3 
Refactor unit tests to use lifecycle methods

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Address review comments
f8740b7 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Improve readability of lifecycle unit tests
3c8e360 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Add missing documentation
d734c7c 
Revert unrelated changes
Manually invoke lifecycle transition in main.cpp

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Remove comments
be1e753 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Add tests for lifecycle publisher state
d0c69da 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Fix rebase issues
2b6956f 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Address review comments
b1a024b 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie
Add error handling
58b2291 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
thomas-moulard
thomas-moulard approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

LGTM. A few nits. You should also probably update the README to explain why this is a managed node? And maybe explain what is the value/how to do things? (e.g. I want to pause data collection, how I do this with the ROS2 cli?)

system_metrics_collector/CMakeLists.txt Outdated Show resolved Hide resolved
system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Outdated Show resolved Hide resolved
system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Outdated Show resolved Hide resolved
system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Outdated Show resolved Hide resolved
system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.hpp Outdated Show resolved Hide resolved
zmichaels11
zmichaels11 approved these changes Jan 17, 2020
View reviewed changes
Copy link

@zmichaels11 zmichaels11 left a comment

Choose a reason for hiding this comment

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

A few comment suggestions, otherwise LGTM

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Outdated Show resolved Hide resolved
system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp Show resolved Hide resolved
system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp Show resolved Hide resolved
system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp Show resolved Hide resolved
@dabonnie
Address review comments
5d4ca51 
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
@dabonnie dabonnie merged commit dc1dfc4 into master Jan 17, 2020
@dabonnie dabonnie deleted the lifecycle branch January 17, 2020 21:30
@dabonnie
Copy link
Contributor Author

Build and tests pass, but codecov is still broken.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mm318 mm318 mm318 left review comments

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@piraka9011 piraka9011 Awaiting requested review from piraka9011

Assignees

@dabonnie dabonnie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dabonnie @mm318 @thomas-moulard @zmichaels11 @piraka9011