Make diff_drive_controller a ChainableControllerInterface

[arthurlovekin]

Using dea3d3c as a reference, I made the diff_drive_controller into a ChainableControllerInterface. I used a RealtimeBuffer to handle the incoming velocity command messages, and I also added lifecycle checks and support for the speed_limiter that were not included in dea3d3c. I also added some tests that run the chainable controller in unchained mode, and I intend to test it in chained mode in one of my current projects (though I couldn't readily figure out how to make this into a test).

[christophfroehlich]

Please always install pre-commit to avoid failing tests ;) otherwise, run it manually with pre-commit run --all

regarding tests: have a look at the mecanum controller, it is chainable anf there are exactly those tests already available

[arthurlovekin]

I've fixed my formatting by running pre-commit run --all-files and added a test for chained mode, using the mecanum controller as a reference.

[codecov]

Codecov Report

Attention: Patch coverage is 88.29787% with 22 lines in your changes missing coverage. Please review.

Project coverage is 84.15%. Comparing base (e0b929b) to head (794b77c).

Files with missing lines	Patch %	Lines
...iff_drive_controller/src/diff_drive_controller.cpp	81.57%	11 Missing and 3 partials ⚠️
...ive_controller/test/test_diff_drive_controller.cpp	92.85%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
+ Coverage   83.86%   84.15%   +0.29%     
==========================================
  Files         122      121       -1     
  Lines       11148    11298     +150     
  Branches      948      955       +7     
==========================================
+ Hits         9349     9508     +159     
+ Misses       1486     1473      -13     
- Partials      313      317       +4     

Flag	Coverage Δ
unittests	84.15% <88.29%> (+0.29%)	⬆️

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

Files with missing lines	Coverage Δ
...ive_controller/test/test_diff_drive_controller.cpp	94.32% <92.85%> (-0.87%)	⬇️
...iff_drive_controller/src/diff_drive_controller.cpp	72.94% <81.57%> (+1.17%)	⬆️

... and 4 files with indirect coverage changes

[christophfroehlich]

Thanks for the follow-up. In principle this looks very good, some comments:

• shouldn't we also check for proper exported reference interfaces like here?
• in principle I'm fine with the change from RealtimeBox to RealtimeBuffer, but this soon might be changed to the lock-free queue. Is there a reason to change this in this PR? If not, I'd ask to remove the change because it is out-of-scope here.

[arthurlovekin]

To be honest I did not know the full implications of switching from a RealtimeBox to RealtimeBuffer, but I chose the change because every other example of chainable interface (eg. mecanum, passthrough, chainable-diff-drive) used it. I figured it probably had a reason for being used, but at the very least using the RealtimeBuffer would provide consistency across the chainable controllers. It seemed like the RealtimeBuffer was designed specifically for use-cases like the chainable interface, where the reference_interfaces could either be safely updated at a realtime rate or more slowly through the publisher, whereas the RealtimeBox would simply make a best-effort attempt at getting and setting the variables (which may in fact be sufficient for this use case) but didn't explicitly account for realtime vs non-realtime. Let me know if you would like me to proceed with changing RealtimeBuffer back to RealtimeBox, and I will do it.

I agree that there should be a check for properly exported reference interfaces. I will imitate the mecanum drive when_controller_configured_expect_properly_exported_interfaces test. I will try to do that later today.

[arthurlovekin]

I just added a test for export_reference_interfaces().

[christophfroehlich]

OK I see your point regarding the RT buffer (I also think that the box was not the right thing to use). Thanks for adding the additional test.

I tested this version with the demos in the non-chained mode, and it still works like expected.

For future PRs: It is not desired to squash the commits (we do that at the time of the merge in the base branch), it just takes more time to review because I can't see now what has changed since the time of my last review.

[arthurlovekin]

Apologies, in the future I will not squash commits while the PR is in progress. The changes I made were to add the test TEST_F(TestDiffDriveController, reference_interfaces_are_properly_exported), and I renamed TEST_F(TestDiffDriveController, configure_succeeds_when_wheels_are_specified) to TEST_F( TestDiffDriveController, command_and_state_interface_configuration_succeeds_when_wheels_are_specified) just to make it more clear that that test evaluates the state_interface_configuration() and command_interface_configuration() functions. The "Compare" button next to my force-push above shows these changes alone.

[christophfroehlich]

The "Compare" button next to my force-push above shows these changes alone.

you are right, I haven't realized that button before. You never stop learning ;)

[mergify]

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

[arthurlovekin]

Is there a way that I can fix the Rolling ABI compatibility check? It seems to be broken because of the change from ControllerInterface to ChainableControllerInterface and I see no way around that.

Side question: I see in the documentation that there is a Working Group meeting every second Wednesday, but I cannot find the link to join. Would it be possible for me to attend?

[olivier-stasse]

Side question: I see in the documentation that there is a Working Group meeting every second Wednesday, but I cannot find the link to join. Would it be possible for me to attend?

Dear @arthurlovekin the announcement, in ROS discourse, for tomorrow's meeting group is here: https://discourse.ros.org/t/ros2-control-working-group-meeting-15th-january-2025/41528

To join I copied paste the invitation in the announcement:
"You can join the meeting through google groups or consult the calendar for the Google Meet link (note that the calendar may use a different time zone than yours)."

[saikishor]

Thanks for the nice contribution. I've some questions around the code that I've reviewed. Thank you

[christophfroehlich]

Is there a way that I can fix the Rolling ABI compatibility check? It seems to be broken because of the change from ControllerInterface to ChainableControllerInterface and I see no way around that.

This change truly is ABI breaking, but this is no show-stopper for this PR. Furthermore, for controllers this is not really an issue because we load them as plugins dynamically.

[christophfroehlich]

Can you please fix the merge conflict after #1478?

[arthurlovekin]

@christophfroehlich the merge conflict should be resolved

[arthurlovekin]

One thing that occurred to me as I was writing the reset_buffer() code: If you want to switch from one controller to another, is it desirable for this controller to halt() on_deactivate()? Perhaps I misunderstand the controller_manager because it seems like you would want to be able to switch without stopping the motors. If you switch into the diff_drive_controller from another one it also seems like there could be issues with the acceleration limiter, since from the diff_drive_controller's perspective its previous_two_commands have been set to zero, but you may be switching at a time when the other controller is commanding a high value. (but if you don't set the previous_two_commands to zero then a single starting command at the beginning will be able to bypass the acceleration limits, which can only be applied if multiple commands have been sent).

[christophfroehlich]

Please fix the pre-commit (cpplint) error. (consider installing pre-commit to avoid this the next time)

[arthurlovekin]

@saikishor can you let me know your thoughts on my responses to your comments?

[saikishor]

It looks pretty good to me now.

I've one question. I see that you are resetting the buffer in the on_configure, and inside we are setting a message with NaN's, so does that mean upon activation until the cmd_timeout_ and if no message is received within this timeout, it will keep printing Command message contains NaNs. Not updating reference interfaces.?

[arthurlovekin]

I've one question. I see that you are resetting the buffer in the on_configure, and inside we are setting a message with NaN's, so does that mean upon activation until the cmd_timeout_ and if no message is received within this timeout, it will keep printing Command message contains NaNs. Not updating reference interfaces.?

You are correct. For me, it was giving a warning about four times on startup. This is kind of annoying so I switched it to RCLCPP_WARN_SKIPFIRST_THROTTLE so that it will skip the first warning, then wait the cmd_vel_timeout_ until logging again (by which time it won't be hitting this warning). However, if you send it a NaN later, then it should give you a warning.

[saikishor]

LGTM. Thanks for all the followups.

[christophfroehlich]

LGTM!