Skip to content

Use RealtimeThreadSafeBox for PID class #387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

christophfroehlich
Copy link
Contributor

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.38%. Comparing base (910093d) to head (c2bfbd2).

Files with missing lines Patch % Lines
control_toolbox/src/pid.cpp 86.66% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           ros2-master     #387      +/-   ##
===============================================
- Coverage        78.41%   78.38%   -0.04%     
===============================================
  Files               30       30              
  Lines             1886     1888       +2     
  Branches           113      114       +1     
===============================================
+ Hits              1479     1480       +1     
  Misses             338      338              
- Partials            69       70       +1     
Flag Coverage Δ
unittests 78.38% <87.87%> (-0.04%) ⬇️

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

Files with missing lines Coverage Δ
control_toolbox/include/control_toolbox/pid.hpp 66.98% <100.00%> (-0.31%) ⬇️
...ontrol_toolbox/include/control_toolbox/pid_ros.hpp 100.00% <ø> (ø)
control_toolbox/src/pid_ros.cpp 70.84% <100.00%> (ø)
control_toolbox/src/pid.cpp 74.84% <86.66%> (-0.16%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

mergify bot commented Jun 10, 2025

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

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

The changes are fine but I've some questions. I've mentioned those comments.

BTW why not use the RTThreadSafeBox Directly?. As we are using the realtime mutexes, it is safe. This way we can avoid another variable completely

@christophfroehlich
Copy link
Contributor Author

BTW why not use the RTThreadSafeBox Directly?. As we are using the realtime mutexes, it is safe. This way we can avoid another variable completely

what do you mean? instead of the gain_ copy? but what happens if try_get in the compute_command fails, then we can't proceed?

saikishor
saikishor previously approved these changes Jun 17, 2025
Copy link

mergify bot commented Jun 17, 2025

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

@christophfroehlich christophfroehlich added backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 jazzy. labels Jun 17, 2025
saikishor
saikishor previously approved these changes Jun 17, 2025
@christophfroehlich christophfroehlich merged commit 4a64d8f into ros-controls:ros2-master Jun 17, 2025
17 of 22 checks passed
@christophfroehlich christophfroehlich deleted the rt_box branch June 17, 2025 21:16
mergify bot pushed a commit that referenced this pull request Jun 17, 2025
Co-authored-by: Sai Kishor Kothakota <[email protected]>
(cherry picked from commit 4a64d8f)

# Conflicts:
#	control_toolbox/include/control_toolbox/pid.hpp
christophfroehlich added a commit that referenced this pull request Jun 18, 2025
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-jazzy This label should be used by maintainers only! Label triggers PR backport to ROS 2 jazzy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants