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

Integrate the frame_prefix argument and make robot_name optional (related to https://github.com/bdaiinstitute/spot_ros2/pull/506). #157

Conversation

Imaniac230
Copy link
Contributor

This PR is related to the changes proposed in bdaiinstitute/spot_ros2#506.

  • The new explicit frame_prefix parameter is exposed to the wrapper.
  • The robot_name parameter is now actually optional.

@coveralls
Copy link

coveralls commented Feb 21, 2025

Pull Request Test Coverage Report for Build 13461765204

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 40.506%

Totals Coverage Status
Change from base Build 13334890157: -0.03%
Covered Lines: 1873
Relevant Lines: 4624

💛 - Coveralls

…e as optional to actually respect the specified usage.

Signed-off-by: Imaniac230 <[email protected]>
@Imaniac230 Imaniac230 force-pushed the proposal-customizable-frameprefix-param branch from de81e6e to 15fd7bd Compare February 21, 2025 11:43
Signed-off-by: Imaniac230 <[email protected]>
logger: logging.Logger,
robot_name: typing.Optional[str] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm good with adding the frame prefix topic, but I'm worried about making the robot name optional as it changes the order of the arguments, and there's some code we have internally that would break with this. How necessary is this for the spot_ros2 PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't really be necessary at all. It was more of a unification thing, since I saw that it was already being handled and passed in as an optional:

self._frame_prefix = ""
if robot_name is not None:
self._frame_prefix = robot_name + "/"

https://github.com/bdaiinstitute/spot_ros2/blob/702c2fa967735f5cde9d7b16937aea0f408ecbbc/spot_driver/spot_driver/spot_ros2.py#L305-L307

https://github.com/bdaiinstitute/spot_ros2/blob/702c2fa967735f5cde9d7b16937aea0f408ecbbc/spot_driver/spot_driver/spot_ros2.py#L367

Only the prefix should be an actual optional, because an empty string prefix is still considered a valid option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the changes made to robot name in 64b9497.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yeah sorry I mispoke, the default value was the thing that would mess up the order of the arguments, not the fact that it was optional. it probably should be changed to an optional without a default value but it's not necessary to fix in this pr :)

@khughes-bdai khughes-bdai merged commit ac2fa01 into bdaiinstitute:main Feb 21, 2025
4 checks passed
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