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

Copter: default topic names #27

Merged
merged 1 commit into from
Sep 2, 2023

Conversation

pedro-fuoco
Copy link
Contributor

@pedro-fuoco pedro-fuoco commented Aug 30, 2023

The point of this PR is to eliminate some of the remappings necessary to work with this package, as discussed with @Ryanf55.

/tf and /tf_static should receive a ros rep 105 compliant tf tree, in this case, the one coming from robot_state_publisher.
Gazebo's tf tree can be renamed through the ros_gz yaml files instead of using another remap.
/laser/scan was renamed to /scan as this is used by both cartographer and nav2

ArduPilot/ardupilot_ros#14
ArduPilot/ardupilot#24819

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Aug 30, 2023

@srmainwaring The principle of this makes sense to me, especially robot_state_publisher now being the official source of tf and tf_static. Do you have any thoughts?

@srmainwaring
Copy link
Collaborator

Using robot_state_publisher as the system of record for tf makes sense. Using the sdformat_urdf ensures that this is consistent with the model in Gazebo and is not exposed to Gazebo specific rearrangement of the model's links.

Perhaps we could use the prefix gz for Gazebo generated topics rather than gazebo. The former is standard for the new generation Gazebo releases, whereas gazebo refers to the classic version (Gazebo 11 and earlier).

IIRC we introduced the ap prefix to avoid conflicts with other sources of TF:

  • In AP_DDS: use /tf instead of /ap/tf ardupilot#24819 we are changing /ap/tf_static to /tf_static, which is published by DDS.
  • If robot_state_publisher is intended as the primary source of \tf_static how will that be differentiated from that published by DDS?
  • \tf is currently subscribed to by DDS, so no conflict there.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Aug 30, 2023

Using robot_state_publisher as the system of record for tf makes sense. Using the sdformat_urdf ensures that this is consistent with the model in Gazebo and is not exposed to Gazebo specific rearrangement of the model's links.

Perhaps we could use the prefix gz for Gazebo generated topics rather than gazebo. The former is standard for the new generation Gazebo releases, whereas gazebo refers to the classic version (Gazebo 11 and earlier).

IIRC we introduced the ap prefix to avoid conflicts with other sources of TF:

Yea, seems like we should keep the prefix ap for stuff coming from ArduPilot.

Copy link
Collaborator

@srmainwaring srmainwaring left a comment

Choose a reason for hiding this comment

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

LGTM

@Ryanf55 Ryanf55 merged commit aae4d6c into ArduPilot:main Sep 2, 2023
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