Skip to content

Remove members to pid state message #178

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
merged 1 commit into from
Apr 24, 2025
Merged

Conversation

ViktorCVS
Copy link
Contributor

@ViktorCVS ViktorCVS commented Mar 11, 2025

Remove members from the pid state message to make it cleaner, without affecting users.

These changes support modifications made to the PID controller in the control_toolbox package. For more details, see ros-controls/control_toolbox#298.

If this PR is approved, it should be merged together with the main PR in the control_toolbox package referenced above.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

I also question the original design: Do we really need to add these variables to the message? For what purpose? I see that it might be beneficial to add the gains for the case when any auto-gain-scheduling is used. But the clamp values and anti-windup methods won't change for a controller, right?

@ViktorCVS
Copy link
Contributor Author

I also question the original design: Do we really need to add these variables to the message? For what purpose? I see that it might be beneficial to add the gains for the case when any auto-gain-scheduling is used. But the clamp values and anti-windup methods won't change for a controller, right?

Well, I agree with you. Changing those variables at runtime might be appropriate in some situations, but I think a simpler message is more reasonable. I don’t see this as a common strategy, so it likely won’t affect most users. I’ll make this change and update the message access in the control_toolbox PR.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

@bmagyar bmagyar merged commit 68a437f into ros-controls:master Apr 24, 2025
4 of 5 checks passed
@christophfroehlich
Copy link
Contributor

@bmagyar this will now break the semi-binary builds, until we can merge ros-controls/control_toolbox#298 (I hadn't had time for a review yet) 🙈

@ViktorCVS ViktorCVS changed the title Adds members to pid state message Remove members to pid state message Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants