Added parameters to handle the overruns behaviour and prints

[saikishor]

Provides a way to fix: #2529. It also provides parameters to toggle the behaviours

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.46%. Comparing base (1506412) to head (7363f61).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2546      +/-   ##
==========================================
+ Coverage   89.43%   89.46%   +0.02%     
==========================================
  Files         148      148              
  Lines       16723    16728       +5     
  Branches     1406     1406              
==========================================
+ Hits        14957    14965       +8     
+ Misses       1222     1220       -2     
+ Partials      544      543       -1     

Flag	Coverage Δ
unittests	89.46% <88.88%> (+0.02%)	⬆️

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

Files with missing lines	Coverage Δ
controller_manager/src/ros2_control_node.cpp	57.57% <100.00%> (+1.51%)	⬆️
controller_manager/src/controller_manager.cpp	76.37% <85.71%> (+0.16%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AndreasKuhner]

Hi @saikishor ,
thanks for the PR. Will test it tomorrow on the robot.

Maybe one more request/suggestion: Due to your new logging of names/cycle times and periodicity, we found out that our Master Controlller of the robot runs not perfectly a 1khz loop relativ to my notebook. Meaning, if the cycle time of ros2_control is tight to 1khz, the actual closed-loop-frequency should be/would be 1003 Hz (e.g. from yesterday). If there is a sleep, we can't run with optimal performance.
Hence, the request would be if we can toggle the sleep altogether. Meaning, that you can de-activate the sleep completely because the components within make sure that the correct frequency is executed (like @BarisYazici did in #2540)

Cheers,
Andreas

[saikishor]

@AndreasKuhner Thank you!

Due to your new logging of names/cycle times and periodicity, we found out that our Master Controlller of the robot runs not perfectly a 1khz loop relativ to my notebook.

That's weird and I'm not sure because it usually gets published in a nonRT thread and I'm successfully using that on a system with 2 kHz. Try compiling in release mode and test it again

Hence, the request would be if we can toggle the sleep altogether. Meaning, that you can de-activate the sleep completely because the components within make sure that the correct frequency is executed (like @BarisYazici did in #2540)

I will discuss with others and let you know, but I don't think that is going to be done in this PR, and if agreed, it should be a separate parameter on its own. Additionally, it is not related to the overrun at all, and that's exactly how it is in Humble currently. With that setup, if someone disables the overruns management, then there is no one ensuring that the calls are happening at a certain frequency, instead, it will max out the CPUs. For that reason, I'm not comfortable doing it.

[AndreasKuhner]

That's weird and I'm not sure because it usually gets published in a nonRT thread and I'm successfully using that on a system with 2 kHz. Try compiling in release mode and test it again

All is tested in release mode :) I trust the periodicity of what you guys compute. If I didn't express myself correctly: I see the 1003 Hz if I disable the sleep. Aka it blocks within our read -> Then it can have 'any' frequency, just controlled by our blocking read and the frequency of the master controller (aka pc) of the robot.

For that reason, I'm not comfortable doing it

I understand 🤔 Yet, the user would still set a target frequency. Hence, if you see that the value deviates too much from what you see during execution, you could also throw warnings..?
Having the user as part of the closed loop control is for sure an expert level setting - but it also means that the user needs more 'direct' control, probably bypassing some comforting features like a controlled cycle time. With great power comes great responsibility 😁

[saikishor]

@AndreasKuhner Sure, I can discuss and get back to you on adding a new parameter to disable the sleep. However, it won't be with the same parameter of the overruns.

I get your explanation. Do you have the same issues with the sleep in Humble too?

[AndreasKuhner]

@AndreasKuhner Sure, I can discuss and get back to you on adding a new parameter to disable the sleep. However, it won't be with the same parameter of the overruns.

That makes totally sense.

I get your explanation. Do you have the same issues with the sleep in Humble too?

We always noticed that we couldn't get the same stability with humble compared to our pure libfranka library and that some of the messages don't arrive/are too late/... Yet, it was stable enough to work on other priorities...
Now with the recent investigations, we also know why. And if we are now on that matter... then we can also try to get the 'best' solution for us :)

[AndreasKuhner]

The test looks good:

What is missing, is the mute for the print overruns. At the moment, I get constantly
[ros2_control_node-2] [WARN] [1758203074.938879830] [controller_manager]: Overrun might occur, Total time : 1072.919 us (Expected < 1000.000 us) --> Read time : 961.822 us, Update time : 77.316 us, Write time : 33.781 us
I see that you introduced a 'mute' with overruns.print_warning but you can't set that yet.

Cheers,
Andreas

[saikishor]

Hello @AndreasKuhner!

Thanks for testing the fix.

I see that you introduced a 'mute' with overruns.print_warning but you can't set that yet.

What do you mean?. Like it is not allowing you to declare it?

[AndreasKuhner]

I just sent to comment and 1 minute later, I got the infamous communication_constraint_violation 😢 I guess to resolve our issues completely, we would need to have this toggleable sleep...

What do you mean?. Like it is not allowing you to declare it?

Yes, I wanted to put overruns.print_warnings: false in the controllers.yaml but it complained with Exception thrown while initializing controller manager parameters: parameter 'overruns.print_warnings' has already been declared.

[saikishor]

Thank you. I'll check it :)

[saikishor]

@AndreasKuhner the issue is fixed now :)

[AndreasKuhner]

Nice, thanks :)

Unfortunately, I am now in vacation for 3 weeks (:partying_face: ) but I guess this PR would be good to go from my side.

Another step then would be about the toggle for the sleep... which would help us as well :)

Cheers,
Andreas

[saikishor]

@AndreasKuhner sure :) I'll keep you posted

[destogl]

Why don't you use generate parameters library for setting up the parameter?

[saikishor]

Because, I can't toggle the default value in the GPL. I want to enable it by default for general applications and at the same time disable it by default for simulations.

[destogl]

You do, but why not print only is managed here?

[saikishor]

Because managed is part of the ros2_control_node and not exactly the controller manager parameter. So, ideally, if we integrate controller_manager in a different way like in a simulation or a custom node, setting this parameter does nothing. So, I separated them

[bmagyar]

@AndreasKuhner whenever you are back or if someone else is happy to chime in from Franka, you guys could put up a PR with that toggle feature and let us know how measuring against that worked on your end 👍

[christophfroehlich]

LGTM