Skip to content
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

Use Generic Steering Controller State Message in Steering Controllers Library #826

Closed

Conversation

mbharatheesha
Copy link

I was reading through the code as a part of learning how to write a new steering controller. I understood that a new steering controller inherits from the SteeringControllersLibrary. But I also found the use of Ackermannxxx in the base class code for a status message and the same was being used in other steering controllers such as BicycleSteeringController and TricycleSteeringController.

If I understood the inheritance and the code structure correctly, the SteerinControllersLibrary should be agnostic to the specifics of a steering controller that derives from it. Therefore, this PR introduces the following changes:

  • Remove the use of Ackermannxxx declarations in the SteeringControllersLibrary base class and rename it to SteeringControllerStateMsg
  • Modify the tests and state publishers to use the renamed declaration
  • Remove ackermann_msgs dependency from steering_controllers_library package.

All tests for the modified packages were run successfully (locally).

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, thanks for the cleanup.

According to our guidelines: Please target the PR to the master branch, and we will perform backports to iron+humble.

@bmagyar
Copy link
Member

bmagyar commented Nov 11, 2023

For humble and iron we can only deprecate the use of these messages. Full removal can be done on rolling

@christophfroehlich
Copy link
Contributor

but it doesn't change anything for users, only inherited classes may be broken. we could maybe use the [[deprecated]] c++ qualifier if necessary.

@mbharatheesha
Copy link
Author

Apologies for the late response. Thanks for your remarks. I will submit a new PR targeted at master later today.

@mbharatheesha
Copy link
Author

Closing this in favor of #836.

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