Skip to content

added a fixed control period to loop#647

Merged
bmagyar merged 5 commits intoros-controls:masterfrom
jackcenter:jack/fixed_control_period
Feb 16, 2022
Merged

added a fixed control period to loop#647
bmagyar merged 5 commits intoros-controls:masterfrom
jackcenter:jack/fixed_control_period

Conversation

@jackcenter
Copy link
Copy Markdown
Contributor

@jackcenter jackcenter commented Feb 16, 2022 •
edited
Loading

This uses a fixed control period to remove the drift issue brought up in Issue #644. It also has the time adjustment from PR #646. I ran some simple tests based on the attachment replacing controller_manager/ros2_control_node.cpp (similar to what was done in #644) while running the rrbot (ros2 launch ros2_control_demo_bringup rrbot.launch.py) using 10 and 1,000 Hz control loops for 10 seconds.

At 1000Hz the current implementation and PR #646 drift almost a full second over the 10-second experiment. This proposed implementation doesn't show any signs of drift over the same test. The data from my quick experiment is here.

I wasn't sure if the period time would need to be updated in the loop or not (I didn't think so), but please let me know if you think it should be moved.

(edit: added test file attachment)
ros2_control_node.cpp.txt

@jackcenter jackcenter mentioned this pull request Feb 16, 2022
Closed
@bmagyar bmagyar requested a review from destogl February 16, 2022 08:27
bmagyar
bmagyar approved these changes Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thanks guys for opening #644 and the data in this PR.

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Feb 16, 2022

And it seems this should be backported to Foxy and Galactic too

@gavanderhoorn

This comment was marked as off-topic.

Sign in to view
@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Feb 16, 2022 •
edited
Loading

We haven't discussed this option yet, no that I'm aware of at least.
However the secondary point with this node was to give a good example for people in case they need to implement their own loop for a reason. I think we could have a second node (we can debate which should be the default one ;) ) that uses timers.

So I definitely welcome the timer suggestion but I think that should be additional to this PR, not replacing.

@gavanderhoorn

This comment was marked as off-topic.

Sign in to view
@gavanderhoorn gavanderhoorn mentioned this pull request Feb 16, 2022
Closed
This was linked to issues Feb 16, 2022
Main control loop spin rate diverges with increasing CM update_rate #644
Closed
[ros2_control_node] Let's check if we are measuring time correctly #645
Closed
destogl
destogl requested changes Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
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.

In light of clearer naming of variable as proposed in #645 and trying to make the code cleaner by reducing some lines, I propose the following updates.

@jackcenter can you test this one more time? My proposal should not change the behavior in any way. Sorry for additional effort, but I hope to solve this once for all :)

@bmagyar you will have to review this again.

Jack and others added 3 commits February 16, 2022 08:33
@destogl
Improvements to naming convention and readability
a2c6da4 
Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
@destogl
Removed unnecessary code missed in last commit
c137b57 
Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
updated variable begin to current_time
0ea706f
@jackcenter
Copy link
Copy Markdown
Contributor Author

jackcenter commented Feb 16, 2022

@destogl I made the requested changes and it is much easier to read now, thanks! I ran the tests again and there was no loss in performance or apparent drift.

destogl
destogl approved these changes Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for the great contribution and detailed analysis!! 🚀

@destogl destogl requested a review from bmagyar February 16, 2022 21:26
@bmagyar bmagyar merged commit 4d2bce8 into ros-controls:master Feb 16, 2022
mergify bot pushed a commit that referenced this pull request Feb 16, 2022
@destogl
added a fixed control period to loop (#647)
fd386c2 
* added a fixed control period to loop

* fixe formatting

* Improvements to naming convention and readability

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

* Removed unnecessary code missed in last commit

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

* updated variable begin to current_time

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
(cherry picked from commit 4d2bce8)
@mergify mergify bot mentioned this pull request Feb 16, 2022
Merged
mergify bot pushed a commit that referenced this pull request Feb 16, 2022
added a fixed control period to loop (#647)
65f032c 
* added a fixed control period to loop

* fixe formatting

* Improvements to naming convention and readability

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

* Removed unnecessary code missed in last commit

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>

* updated variable begin to current_time

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
(cherry picked from commit 4d2bce8)

# Conflicts:
#	controller_manager/src/ros2_control_node.cpp
@mergify mergify bot mentioned this pull request Feb 16, 2022
Merged
destogl added a commit that referenced this pull request Feb 17, 2022
@mergify @destogl
added a fixed control period to loop (#647) (#650)
1379722 
* added a fixed control period to loop
* Improvements to naming convention and readability

Co-authored-by: Denis Štogl <denis@stogl.de>
Co-authored-by: Jack <jack.center@picknik.ai>
bmagyar pushed a commit that referenced this pull request Feb 23, 2022
@mergify @destogl
added a fixed control period to loop (backport #647) (#651)
b7e29fc 
* added a fixed control period to loop (#647)
Co-authored-by: Jack <jack.center@picknik.ai>
Co-authored-by: Denis Štogl <denis@stogl.de>
pac48 pushed a commit to pac48/ros2_control that referenced this pull request Jan 26, 2024
@Nils-ChristianIseke
Updated hyperlink of pluginlib documentation (ros-controls#647)
d62d7fa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@destogl destogl destogl approved these changes

@bmagyar bmagyar bmagyar approved these changes

@livanov93 livanov93 Awaiting requested review from livanov93

@TomoyaFujita2016 TomoyaFujita2016 Awaiting requested review from TomoyaFujita2016

+2 more reviewers

@AndyZe AndyZe AndyZe approved these changes

@aprotyas aprotyas aprotyas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ros2_control_node] Let's check if we are measuring time correctly Main control loop spin rate diverges with increasing CM update_rate

6 participants

@jackcenter @bmagyar @gavanderhoorn @destogl @AndyZe @aprotyas