A method to get node options to setup the controller node #api-breaking

[saikishor]

This PR adds a new API get_node_options method that other controllers will be able to override, this would make it easy for other controllers to be able to setup their own custom node options

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (51ff9ce) 48.02% compared to head (4d63bcd) 48.00%.
Report is 4 commits behind head on master.

❗ Current head 4d63bcd differs from pull request most recent head a0c1341. Consider uploading reports for the commit a0c1341 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
- Coverage   48.02%   48.00%   -0.03%     
==========================================
  Files          41       41              
  Lines        3525     3529       +4     
  Branches     1912     1915       +3     
==========================================
+ Hits         1693     1694       +1     
  Misses        442      442              
- Partials     1390     1393       +3     

Flag	Coverage Δ
unittests	48.00% <40.00%> (-0.03%)	⬇️

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

Files	Coverage Δ
...roller_interface/src/controller_interface_base.cpp	48.14% <100.00%> (ø)
...controller_interface/controller_interface_base.hpp	37.50% <50.00%> (+4.16%)	⬆️
controller_manager/src/controller_manager.cpp	39.12% <0.00%> (-0.07%)	⬇️

[mergify]

This pull request is in conflict. Could you fix it @saikishor?

[saikishor]

@bmagyar @destogl @christophfroehlich As we are breaking the API, do you think this PR make sense to go into rolling?. I'm asking because most of the time, you tend to override the init method to just replace the NodeOptions and with this PR it would be much simpler. I also have a branch with the changes to the ros2_controllers tests : ros-controls/ros2_controllers#840.

What's your opinion on this PR?

[christophfroehlich]

I would never have needed to overwrite NodeOptions (or at least never thought of a solution using this), but the changes look simple and clean for me.
I just ask for some API documentation, which is published here.

[saikishor]

I just ask for some API documentation, which is published here.

@christophfroehlich Thanks for the reminder, I've just added some documentation to the newly added API method

[saikishor]

I would never have needed to overwrite NodeOptions (or at least never thought of a solution using this), but the changes look simple and clean for me.

The motivation behind it is, if someone wants to override the NodeOptions to have allow_undeclared_parameters, right now they have to override this init function to be able to do it. Moreover, the controller manager while initializing the controllers with the load_controller service, doesn't pass this argument at all, so, for me it didn't make sense to be a part of the controller init method

ros2_control/controller_manager/src/controller_manager.cpp

Lines 1298 to 1301 in 2056d6c

	if (
	controller.c->init(
	controller.info.name, robot_description_, get_update_rate(), get_namespace()) ==
	controller_interface::return_type::ERROR)

[mergify]

This pull request is in conflict. Could you fix it @saikishor?

[christophfroehlich]

Should we add any explicit hint that this method can/should be overridden?

[saikishor]

Should we add any explicit hint that this method can/should be overridden?

Sure, why not. I can add it to the documentation of the method.

Thank you

[destogl]

Just one thing to clarify in the test change.

One more thing. What do you think if we named the method define_node_options or define_custom_node_options, rather then get_node_options. The last one sounds more like getter, not sure if name "invites" people to override it when reading it. @christophfroehlich @bmagyar what do you think too?

[destogl]

Shouldn't we still keep this test?

What I understand, you are here not moving the node options only, but changing how init is provoking error in this case. It could be that I am missing a detail or something.

[saikishor]

Ideally, you don't need to have this as the same behavior will be achieved as the main init method in the base class will call the get_node_options method

[saikishor]

@bmagyar @destogl @christophfroehlich
I need some feedback from you guys. Do you think it makes sense to package these arguments into a structure so that next time we like to add or remove entities we won't be breaking any API (Similar to PImpl implementation)?

Moreover, this PR only provides a method for the user to override the node_options of this particular controller. As per the PR: #1293, the CM will need to get these node options from the newly added method and modify them with the arguments of the params-file and then somehow feed it back to the init method to initialize the LifeCycleNode.

• We can still keep this node_options parameter in the init method, but we will have to remove the default argument and also mark it as final so it won't be overridden
• We can package them all together into a single struct (PImpl approach) and then mark this init method as final, then we can do the logic without many issues.

One of the above should be fine. Any more ideas are welcome.

[christophfroehlich]

One more thing. What do you think if we named the method define_node_options or define_custom_node_options, rather then get_node_options. The last one sounds more like getter, not sure if name "invites" people to override it when reading it. @christophfroehlich @bmagyar what do you think too?

I asked already for a more explicit API description that this can be overridden, but renaming is a good idea! I'd go for define_custom_node_options

[saikishor]

@destogl @christophfroehlich @bmagyar I've applied the suggestion proposed by you guys, and then modified a few things to have it ready for the other PR that fixes the parameters: #1293

Let me know your opinion

[christophfroehlich]

LGTM, minor text changes only

[mergify]

This pull request is in conflict. Could you fix it @saikishor?