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.

Sign up for GitHub

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

Jump to bottom

feat: xacro compilation #361

Merged
merged 21 commits into from
Dec 27, 2024
Merged

feat: xacro compilation #361

merged 21 commits into from
Dec 27, 2024

Conversation

Owen-Liuyuxuan
Copy link
Contributor

@Owen-Liuyuxuan Owen-Liuyuxuan commented Dec 20, 2024 •
edited
Loading

Updates (12/20)

Internal Document

Feature 1: Independent aip_urdf_compiler

This package contains the scripts, templates, tests, and cmake macro. Other packages only need to add

find_package(aip_urdf_compiler REQUIRED)
aip_cmake_urdf_compile()

in CMakeLists.txt to get the compilation done.

Feature 2: Optional frame ID annotation in config

  velodyne_right_base_link:
    x: 0.0
    y: -0.56362
    z: -0.30555
    roll: -0.01
    pitch: 0.71
    yaw: -1.580
    type: VLP-16.urdf
  gnss_link:
    x: -0.1
    y: 0.0
    z: -0.2
    roll: 0.0
    pitch: 0.0
    yaw: 0.0
    type: imu
    frame_id: gnss  # optional frame_id, which will be the child frame name in urdf links that determine frame_id
  tamagawa/imu_link:
    x: 0.0
    y: 0.0
    z: 0.0
    roll: 3.14159265359
    pitch: 0.0
    yaw: 3.14159265359
    type: imu
    frame_id: tamagawa/imu # optional frame_id, which will be the child frame name in urdf links that determine frame_id

Original PR:
#274

Updates (12/13)

After accumulating experience in integration. Here is an updated/robust version that I would like to push forward.

Feature 1: Type Annotation in config

In the Xacro Compilation script, we defined URDF Links into the following types.

# Links mainly available from commen_sensor_description
class LinkType(enum.Enum):
    """Enum class for the type of the link."""

    CAMERA = "monocular_camera"
    IMU = "imu"
    LIVOX = "livox_horizon"
    PANDAR_40P = "pandar_40p"
    PANDAR_OT128 = "pandar_ot128"
    PANDAR_XT32 = "pandar_xt32"
    PANDAR_QT = "pandar_qt"
    PANDAR_QT128 = "pandar_qt128"
    VELODYNE16 = "velodyne_16"
    VLS128 = "velodyne_128"
    RADAR = "radar"
    GNSS = "gnss"
    JOINT_UNITS = "units"

Then when sensor configurations are defined as

ars408_front_center:
    x: 3.8
    y: 0.0
    z: 0.5
    roll: 0.0
    pitch: 0.0
    yaw: 0.0
    type: radar  # This needs to align with the string in LinkType
  sensor_kit_base_link:
    x: 0.9
    y: 0.0
    z: 2.0
    roll: -0.001
    pitch: 0.015
    yaw: -0.0364
    type: units # This needs to align with the string in LinkType
  livox_front_right_base_link:
    x: 3.290
    y: -0.65485
    z: 0.3216
    roll: 0.0
    pitch: 0.0
    yaw: -0.872664444
    type: livox_horizon # This needs to align with the string in LinkType

The compiler will reliably determine what kind of link each transform is.

If the type is not annotated, the compiler will produce a warning and still try to infer the LinkType from the name of the link.

image

Feature 2: Force Recompiling on Each Colcon Build

The former PR has the problem of needing to modify the CMakeLists.txt to trigger re-compilation. Now we improve the CMakeLists.txt so that we will create a brand-new urdf every time we rebuild, so that we can make tuning on the fly easily.

Description

Proposal of dynamically building xacro files during CMake from sensor calibration configs instead of hard-coding them in the repo.

#275

Methods

Using the jinja2 template mechanism, we can dynamically create as many joints as defined in the calibration Yaml file.

Tricky Problem

To determine the link types, we use the names of the links to infer the types, trying our best to be consistent with the common inputs and also readable. (determine_link_type)

Test

I have tried successfully launching PSim with the created xacro.

Owen-Liuyuxuan and others added 12 commits December 19, 2024 15:48
@Owen-Liuyuxuan
aip launch fix for XX1
2723baa 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
aip_xx1_gen2 example
17eb547 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
clean up
0b2d64f 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
feat(aip_urdf_compiler): significantly improve documentation; add add…
5a9e75a 
…itional script for comparing different xacro files for testing; add optional keywords for frame_id

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fix(xx1_des): add frame_id for certain sensors to restore the origina…
596d5b0 
…l look

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fix(xx1_gen2_des): add frame_id for certain sensors to restore the or…
c6095b6 
…iginal look

Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
create a cmake macro to simplify adaptation of other packages
bc97d66 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
xx1_des utilize the new macro
c2fd487 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
xx1_gen2 utilize the new macro
8634e74 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fixbug
3f54a6f 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fix debug
b54e73b 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@pre-commit-ci
ci(pre-commit): autofix
51ad4bd
@Owen-Liuyuxuan
Copy link
Contributor Author

raw outputs from the xml_diff test script:

Analyse files for XX1 sensors

Key Differences

1. Include Files

Changes:

Added:

  • $(find radar_description)/urdf/radar.xacro
    Removed:
  • $(find aip_xx1_description)/urdf/radar.xacro

2. Sensor Configuration Changes

Lidars

Parameter Changes:

Modified parameters:

  • gpu: "false" → "$(arg gpu)"
  • topic: "velodyne_rear/velodyne_points" → "/points_raw"

Analyse files for XX1 sensor_kit

Key Differences

1. Include Files

Changes:

Added:

  • $(find livox_description)/urdf/livox_horizon.xacro
  • $(find radar_description)/urdf/radar.xacro
    Removed:
  • $(find velodyne_description)/urdf/HDL-32E.urdf.xacro

2. Sensor Configuration Changes

Cameras

Parameter Changes:

Lidars

Parameter Changes:

Imus

Parameter Changes:

Analyse files for XX1 Gen2 sensors

Key Differences

1. Include Files

2. Sensor Configuration Changes

Lidars

Parameter Changes:

Radars

Parameter Changes:

Analyse files for XX1 Gen2 sensor_kit

Key Differences

1. Include Files

Changes:

Added:

  • $(find radar_description)/urdf/radar.xacro
    Removed:
  • $(find velodyne_description)/urdf/VLP-16.urdf.xacro

2. Sensor Configuration Changes

Cameras

Parameter Changes:

Lidars

Parameter Changes:

Imus

Parameter Changes:

@Owen-Liuyuxuan Owen-Liuyuxuan marked this pull request as ready for review December 20, 2024 04:21
This was referenced Dec 20, 2024
Open
Open
@shmpwk
Copy link
Contributor

shmpwk commented Dec 23, 2024 •
edited
Loading

Amazing! This PR can handle various type of sensors and reduce the double mentainance of the sensor configuration. Also, it does not change the link name itself and guarantees rosbag data compatibility.
Type annotation and frame_id specification help us with it, but on the other hand, it increases complexity.

I once wondered if there was any possibility of not designating them, but in vain as follows:

  • If we don't introduce type annotation, we need to change the root xacro file name. There is a non-zero chance that it could be renamed, but the thing is there are no substitute name for it. Then I confirm we definitely need type annotation.
  • If we don't introduce frame_id, we will use sensor link name. But it will change the current tf tree, which will destroy the rosbag compatibility. Then I confirm we definitely need optional frame_id overwrite.

@shmpwk
Copy link
Contributor

shmpwk commented Dec 23, 2024

@Owen-Liuyuxuan
"type" infers xacro name right? I feel like we can directly say "xacro_name" because the name "type" is a little bit ambiguous and it says "imu" for gnss type which was confusing me.

@Owen-Liuyuxuan
Copy link
Contributor Author

@shmpwk
I was trying to indicate the "type of sensors" when I designed that name, which is more straight forward for deployers. I think it is easier than I also add to the "GNSS" type but render it into an "imu" xacro.

@KYabuuchi
Copy link
Contributor

Thank you for creating this efficient feature. 👏
Initially I was concerned about availability to other vehicle configurations, but I realized we can easily address this by adding functionality to compile_xacro.py. (e.g. new sensors support). 👍 As such, I don't see any critical issues with adding this feature.

Below are some points I noticed in the code:

  1. Please create a README.md for aip_urdf_compiler
    I think copying the content from this elegant Confluence page would be sufficient for the README.

  2. Why is only velodyne's type format different? I don't think this name is intuitive.

  • monocular_camera
  • panda_xt32
  • pandar_ot128
  • imu
  • VLS-128.urdf 🤔
  • VLP-12.urdf 🤔
  1. Should xml_diff.py be included in this PR?
    I think attaching it to the PR description might be sufficient. Becasuse usually there is no opportunity to compare old and new xacro files. Also I think the name xacro_diff.py would be more intuitive.

@shmpwk
Copy link
Contributor

shmpwk commented Dec 23, 2024

Aside from this PR, do we also need to change https://github.com/tier4/autoware_individual_params.jpntaxi ?

@Owen-Liuyuxuan
Copy link
Contributor Author

@shmpwk
Important note also to other users: we do not need meta values in individual_params. We only need meta values in aip_launcher

@Owen-Liuyuxuan
Copy link
Contributor Author

  • monocular_camera
  • panda_xt32
  • pandar_ot128
  • imu
  • VLS-128.urdf 🤔
  • VLP-12.urdf 🤔

Actually, these are the name of xacro files, when I created them at the very first place, I was just following the xacro file names.

But for now I believe it is OK to let us use more natural names and hide this abstraction.
image

@Owen-Liuyuxuan
Copy link
Contributor Author

Update: improve warning mechanism when type is not annotated.
image

shmpwk
shmpwk reviewed Dec 23, 2024
View reviewed changes
aip_urdf_compiler/readme.md Outdated Show resolved Hide resolved
@Owen-Liuyuxuan
improve documentation
ef0bff6 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fix grammar
415326c 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fix cspell
1c21d10 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@shmpwk
Copy link
Contributor

shmpwk commented Dec 24, 2024

@Owen-Liuyuxuan
BTW, this feature is for xacro compilation. How about changing the name aip_urdf_compiler to aip_xacro_compiler and change the naming of the other urdf as well?

@Owen-Liuyuxuan
Copy link
Contributor Author

@shmpwk
Sorry for the late reply. Xacro is a special XML format, while URDF means robot description files using XML format.
I think I should update the name to compile_urdf because many of the functions are only for robot descriptions (Autoware/aip description)

@Owen-Liuyuxuan
unify the name in aip_urdf_compiler to be URDF compile
91ff326 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@Owen-Liuyuxuan
fix bug
13bb21f 
Signed-off-by: YuxuanLiuTier4Desktop <[email protected]>
@shmpwk
Copy link
Contributor

shmpwk commented Dec 27, 2024

@Owen-Liuyuxuan Thank you, make sense!

shmpwk
shmpwk approved these changes Dec 27, 2024
View reviewed changes
Copy link
Contributor

@shmpwk shmpwk left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@Owen-Liuyuxuan Owen-Liuyuxuan merged commit ef46885 into tier4/universe Dec 27, 2024
8 of 10 checks passed
@Owen-Liuyuxuan Owen-Liuyuxuan deleted the feat/xacro_compilation branch December 27, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shmpwk shmpwk shmpwk approved these changes

@yukkysaito yukkysaito Awaiting requested review from yukkysaito

@KYabuuchi KYabuuchi Awaiting requested review from KYabuuchi

@Ericpotato Ericpotato Awaiting requested review from Ericpotato

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Owen-Liuyuxuan @shmpwk @KYabuuchi