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

[MAPLE-681] Add robot_description_package as parameter to driver launch #531

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

llee-bdai
Copy link
Collaborator

Change Overview

This allows you to change the pack from where to get the robot description. This allows us to have different configurations of the robot (such as custom payloads)
Makes the (hard) assumption that whatever package you point it to has {package}/urdf/spot.urdf.xacro in it.

@tcappellari-bdai @khughes-bdai feel free to agree/disagree with this, but only way I could get extra collision objects in as needed on hardware

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12040145182

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 51.053%

Totals Coverage Status
Change from base Build 12038008121: 0.0%
Covered Lines: 1939
Relevant Lines: 3798

💛 - Coveralls

Copy link
Collaborator

amessing-bdai commented Nov 27, 2024

@llee-bdai I am a urdf/xacro novice, so this is just a question, but is there a way to use xacro variables for this?

Copy link
Collaborator

@amessing-bdai this is the alternative I thought of that uses xacro args in the main spot URDF. it should allow you to not duplicate the rest of the spot urdf internally but I don't know if this is actually a better solution

...
<xacro:arg name="extras_package" default=""/>
...
<load main spot model>

<xacro:if value="$(arg extras_package)">
  <xacro:include filename="$(find extras_package)/urdf/spot_extras.urdf" />

then in the launchfile we could have

robot_description = Command(
        [
            PathJoinSubstitution([FindExecutable(name="xacro")]),
            " ",
            PathJoinSubstitution([pkg_share, "urdf", "spot.urdf.xacro"]),
            " ",
            "arm:=",
            TextSubstitution(text=str(has_arm).lower()),
            " ",
            "tf_prefix:=",
            tf_prefix,
            " ",
            "extras_package:=",
           LaunchConfiguration(robot_description_package)
        ]
    )

there is also this leftover remnant from the ros1 repo https://github.com/bdaiinstitute/spot_ros2/blob/main/spot_description/urdf/empty.urdf but setting an environment variable to add extra things to your URDF seems like not a great way to do things

Copy link
Collaborator

@khughes-bdai khughes-bdai left a comment

Choose a reason for hiding this comment

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

I'm good with this change, by default this doesn't change any behavior. I think this will also be a useful addition for driver integration with the custom gripper Spot. (and with the future push to separate out spot description and consolidate all of our urdfs in RDS, we might come up with a more streamlined way to add to the base spot urdf). but i would also like to hear other opinions on this :)

Copy link
Collaborator

It seems better to have addons vs duplicate urdf files to me, but I wouldn't be a user. @llee-bdai thoughts?

Consolidating in RDS has some hurdles to make happen, so I wouldn't base a decision on it.

Copy link
Collaborator

i think the downside of add-ons is that it's harder to expose custom arguments through the core spot URDF, so it feels less flexible

(with the custom gripper work as an example, I would want to set the xacro args gripperless and custom_gripper_base_link in order to get an add-on to the main spot URDF to work, which is not exposed through the launchfile's xacro command)

Copy link

@scastro-bdai scastro-bdai left a comment

Choose a reason for hiding this comment

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

While I normally would agree that xacro args is better, I'm not entirely sure these "antlers" warrant being an officially supported argument until it becomes more of a standard thing. So I'm onboard with "different URDF" for the short term.

Copy link
Collaborator Author

@amessing-bdai @khughes-bdai after playing around with it a bit today, I was trying to make it so that any additional arguments could be just generic "add ons" to the Spot urdf. Unfortunately, in order to get this working, it would require quite a bit more parameter passing. We can do it, but it gets a little messy for the sake of what I'm trying to do here. I think it makes more sense if we have more add ons in the future, but for just this, the repeat in urdf is less change than all the extra params needed.

I'm inclined to leave it as is if that's cool

Copy link
Collaborator Author

llee-bdai commented Dec 2, 2024

Merge activity

  • Dec 2, 4:14 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 2, 4:14 PM EST: A user merged this pull request with Graphite.

@llee-bdai llee-bdai merged commit fe6729e into main Dec 2, 2024
4 checks passed
@llee-bdai llee-bdai deleted the ll/maple/add_robot_description_package_as_param branch December 2, 2024 21:14
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.

5 participants