Skip to content

Conversation

v-lopez
Copy link
Contributor

@v-lopez v-lopez commented Dec 9, 2020

Summary of changes:

  • Replace LifecyclePublishers with regular Publishers, unfortunately now controllers could publish while deactivated.
  • Refactor the calls to get_lifecycle_node

The LifecyclePublisher above made me notice that there are no LifecycleSubscribers or LifecycleServices. Is it because they have yet to be implemented, or because a LifecycleNode stops serving callbacks if it's not active? I could not find mention to this.

@bmagyar bmagyar self-requested a review December 9, 2020 12:53
@codecov-io
Copy link

codecov-io commented Dec 12, 2020

Codecov Report

Merging #124 (d7c87cb) into master (38f793d) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
- Coverage   37.63%   37.63%   -0.01%     
==========================================
  Files          30       15      -15     
  Lines        3762     1868    -1894     
  Branches     2354     1171    -1183     
==========================================
- Hits         1416      703     -713     
+ Misses        246      119     -127     
+ Partials     2100     1046    -1054     
Flag Coverage Δ
unittests 37.63% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ller/test/test_load_forward_command_controller.cpp
...mand_controller/src/forward_command_controller.cpp
...ler/test/test_load_joint_trajectory_controller.cpp
...te_controller/test/test_joint_state_controller.cpp
...ectory_controller/test/test_trajectory_actions.cpp
...ory_controller/test/test_trajectory_controller.cpp
...ntroller/test/test_load_joint_state_controller.cpp
...include/joint_trajectory_controller/trajectory.hpp
...include/joint_trajectory_controller/trajectory.hpp
...nt_state_controller/src/joint_state_controller.cpp
... and 35 more

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

All good!


#include "hardware_interface/types/hardware_interface_return_values.hpp"
#include "hardware_interface/types/hardware_interface_type_values.hpp"
#include "lifecycle_msgs/msg/state.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Also remove this line please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@v-lopez v-lopez changed the title WIP: Remove lifecycle node controllers Remove lifecycle node controllers Dec 13, 2020
@bmagyar
Copy link
Member

bmagyar commented Dec 14, 2020

Ok I've touched things up a bit, added back the node nullptr check to facilitate one of the existing tests and modified another test to not try the ill-posed update() call. I think we should discuss what exactly the role of each state is, where we want to allow memory allocation and where is shouldn't be. When we mixed lifecycle nodes and the back-and-forth mechanism of "let me declare what I want then you assign" with controllers and CM made all of that a little fuzzy, the tests will probably need to be revised on a couple of places to reflect this properly. Not a blocker at the moment though.

@bmagyar bmagyar merged commit 6a4c7a0 into ros-controls:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants