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

Move drivers into separate .repos file #3366

Open
3 tasks done
xmfcx opened this issue Mar 24, 2023 · 18 comments
Open
3 tasks done

Move drivers into separate .repos file #3366

xmfcx opened this issue Mar 24, 2023 · 18 comments
Assignees
Labels
type:ci Continuous Integration (CI) processes and testing. type:installation Issues or improvements related to the installation process of the software. type:new-feature New functionalities or additions, feature requests.

Comments

@xmfcx
Copy link
Contributor

xmfcx commented Mar 24, 2023

Checklist

  • I've read the contribution guidelines.
  • I've searched other issues and no duplicate issues were found.
  • I've agreed with the maintainers that I can plan this task.

Description

This issue is about moving the driver packages to a separate extra-packages.repos file from autoware.repos file.

Purpose

Since not every vehicle will use all the drivers, or some other packages in the future, it's better for them to have a separate extra-packages.repos file.

This way when people fork autoware, it's easier for them to disable these repositories.

Possible approaches

Current repos that can be moved:

  sensor_component/external/tamagawa_imu_driver:
    type: git
    url: https://github.com/tier4/tamagawa_imu_driver.git
    version: ros2
  vehicle/external/pacmod_interface:
    type: git
    url: https://github.com/tier4/pacmod_interface.git
    version: main

Definition of done

  1. Move the pacmod_interface into extra-packages.repos
    • The Autoware doesn't have any reference to this package, easy to just move.
  2. Update the following to also vcs import src < extra-packages.repos
  3. Remove references of tamagawa_imu_driver from:
  4. Move the tamagawa_imu_driver into extra-packages.repos
  5. Update the installation documentation to include information about extra-packages.repos

Additional points:

  • For now, extra-packages.repos doesn't need to have a -nightly version.
@xmfcx
Copy link
Contributor Author

xmfcx commented Mar 24, 2023

@kenji-miyake @mitsudome-r @esteve maybe we can also discuss about whether we should build these packages as part of the CI or not.

Or discuss whether we should do the proposal on this issue at all.

From these 3, I think velodyne is the more essential one but I'm not sure about what to do with the others. Any thought is appreciated.

@esteve
Copy link
Contributor

esteve commented Mar 24, 2023

@xmfcx I think pacmod could be moved out easily, but as you say Velodyne is essential to our architecture, so I'm not sure if it should be moved out. The Tamagawa IMU driver doesn't seem to be used much, though. Only two packages depend on it: sample_sensor_kit_launch and awsim_sensor_kit_launch

@xmfcx xmfcx added meeting:core-universe-planning Core - Universe planning working group Type: improvement and removed meeting:core-universe-planning Core - Universe planning working group labels Apr 10, 2023
@kenji-miyake
Copy link
Contributor

@xmfcx I'm sorry for joining late. Explaining the background, these are from TIER IV's reference vehicle/sensor_kit.

I think splitting repos or just removing some is okay, but there are several things to be discussed:

  1. Which perspective to divide

Do you really want to split drivers? Or do you want to split the entire sensor components? I'm not sure which is better.

Since not every vehicle will use all the drivers, it's better for them to have a separate driver.repos file.

Seeing this purpose, I feel like you want to split the sensor components that aren't related to your vehicle.

This way when people fork autoware, it's easier for them to disable these repositories.

For this, I think users can do it with the current configuration because they are under the sensor_component section. 🤔
Could you please show me some concrete examples?

  1. Dependency

We might need to fix some dependencies before splitting them out.


Anyway, I'd like to clarify the background and issues more.
Honestly, I feel that the current proposal might be a bit short-sighted.
So could you share your troubles in more detail with me?

@mitsudome-r
Copy link
Member

@xmfcx
Could you share more detail about the purpose of this issue?

  • Is it to create new repos file to list up Autoware-supported sensor drivers?, or
  • Do you just want to remove the need of installing unnecessary drivers?

If latter is the case, I think we can just remove Tamagawa-imu and pacmod-interface drivers since they are not used in tutorials anyways.

@xmfcx xmfcx added type:new-feature New functionalities or additions, feature requests. and removed Type: improvement labels Nov 14, 2023
@xmfcx
Copy link
Contributor Author

xmfcx commented Nov 16, 2023

So, I have thought about this more generally and here are the things I'd like to achieve:

  • Users shouldn't be forced to build drivers that they won't use.
  • The drivers should be tested in the repository through the CI to keep compatible.
  • Some drivers are essential, like the nebula driver.

For this, I will separate out the following:

  • pacmod_interface
  • tamagawa_imu_driver

into a different .repos file called extra-packages.repos.

Will modify the CI to pull them along with the others, build and test as usual.

The users will download the usual autoware.repos without these packages.

If they want to, they can download it with the extra-packages.repos.

In the future, we can think of similar non-essential external packages to this list.

Do you think this is reasonable? @esteve @mitsudome-r ?

@xmfcx xmfcx added type:installation Issues or improvements related to the installation process of the software. type:ci Continuous Integration (CI) processes and testing. labels Nov 16, 2023
@xmfcx xmfcx self-assigned this Nov 16, 2023
@esteve
Copy link
Contributor

esteve commented Nov 16, 2023

@xmfcx I agree with you, the drivers are quite specific to the vehicle where Autoware is running, so it'd be better to move them to a separate .repos file and let users use whatever driver they need for their vehicle.

@xmfcx
Copy link
Contributor Author

xmfcx commented Feb 17, 2025

tamagawa_imu_driver

So far this is only used by sample_sensor_kit_launch/launch/imu.launch.xml

Should we keep it in there or should we remove specific vendor related packages out of sample sensor kit launch file?

🖱️Click here to expand🔛
mfc@bunny:~/projects/autoware$ rg "tamagawa_imu_driver" --hidden --no-ignore -g '!*.{rst,md}' -g '!{build,install,log,**/.git,**/.idea}/**'
src/sensor_component/external/tamagawa_imu_driver/.circleci/config.yml
7:    working_directory: ~/tamagawa_imu_driver

src/sensor_component/external/tamagawa_imu_driver/launch/can.launch.xml
5:  <node pkg="tamagawa_imu_driver" name="tag_can_driver" exec="tag_can_driver" />

src/sensor_component/external/tamagawa_imu_driver/launch/serial_AU7554N.launch.xml
5:  <node pkg="tamagawa_imu_driver" name="tag_serial_driver" exec="tag_serial_driver" args="/dev/ttyUSB0" "noGPS" "100"/>

src/sensor_component/external/tamagawa_imu_driver/launch/serial_TAG250_Nxx40.launch.xml
5:  <node pkg="tamagawa_imu_driver" name="tag_serial_driver" exec="tag_serial_driver" args="/dev/ttyUSB0" "withGPS" "100"/>

src/sensor_component/external/tamagawa_imu_driver/launch/serial_TAG250_Nxx00.launch.xml
5:  <node pkg="tamagawa_imu_driver" name="tag_serial_driver" exec="tag_serial_driver" args="/dev/ttyUSB0 noGPS 100" output="screen"/>

src/sensor_component/external/tamagawa_imu_driver/CMakeLists.txt
2:project(tamagawa_imu_driver)

src/sensor_component/external/tamagawa_imu_driver/launch/serial_TAG264.launch.xml
5:  <node pkg="tamagawa_imu_driver" name="tag_serial_driver" exec="tag_serial_driver" args="/dev/ttyUSB0" "withGPS" "50"/>

src/sensor_component/external/tamagawa_imu_driver/package.xml
4:  <name>tamagawa_imu_driver</name>

src/sensor_kit/awsim_labs_sensor_kit_launch/awsim_labs_sensor_kit_launch/package.xml
16:  <exec_depend>tamagawa_imu_driver</exec_depend>

src/sensor_kit/external/awsim_sensor_kit_launch/awsim_sensor_kit_launch/package.xml
16:  <exec_depend>tamagawa_imu_driver</exec_depend>

src/sensor_kit/single_lidar_sensor_kit_launch/single_lidar_sensor_kit_launch/package.xml
15:  <exec_depend>tamagawa_imu_driver</exec_depend>

src/sensor_kit/sample_sensor_kit_launch/sample_sensor_kit_launch/package.xml
17:  <exec_depend>tamagawa_imu_driver</exec_depend>

src/sensor_kit/sample_sensor_kit_launch/sample_sensor_kit_launch/launch/imu.launch.xml
9:      <node pkg="tamagawa_imu_driver" name="tag_serial_driver" exec="tag_serial_driver" if="$(var launch_driver)">

autoware.repos
88:  sensor_component/external/tamagawa_imu_driver:
90:    url: https://github.com/tier4/tamagawa_imu_driver.git

pacmod_interface

This can be moved without issues since it's not used by the other packages.

🖱️Click here to expand🔛
mfc@bunny:~/projects/autoware$ rg "pacmod_interface" --hidden --no-ignore -g '!*.{rst,md,svg}' -g '!{build,install,log,**/.git,**/.idea}/**'
src/vehicle/external/pacmod_interface/pacmod_interface/CMakeLists.txt
2:project(pacmod_interface)
26:ament_auto_add_executable(pacmod_interface
27:  src/pacmod_interface/pacmod_interface.cpp
28:  src/pacmod_interface/pacmod_interface_node.cpp
32:  src/pacmod_interface/pacmod_diag_publisher.cpp
33:  src/pacmod_interface/pacmod_diag_publisher_node.cpp

src/vehicle/external/pacmod_interface/pacmod_interface/package.xml
4:  <name>pacmod_interface</name>

src/vehicle/external/pacmod_interface/pacmod_interface/launch/pacmod_interface.launch.xml
3:  <arg name="pacmod_param_path" default="$(find-pkg-share pacmod_interface)/config/pacmod.param.yaml"/>
4:  <arg name="pacmod_extra_param_path" default="$(find-pkg-share pacmod_interface)/config/pacmod_extra.param.yaml"/>
5:  <arg name="pacmod_diag_publisher_param_path" default="$(find-pkg-share pacmod_interface)/config/pacmod_diag_publisher.param.yaml"/>
11:  <node pkg="pacmod_interface" exec="pacmod_interface" name="pacmod_interface" output="screen">
18:  <node pkg="pacmod_interface" exec="pacmod_diag_publisher" name="pacmod_diag_publisher" output="screen">
23:  <node pkg="pacmod_interface" exec="pacmod_dynamic_parameter_changer" name="pacmod_dynamic_parameter_changer" output="screen">

src/vehicle/external/pacmod_interface/pacmod_interface/launch/pacmod_additional_debug_publisher.launch.xml
8:  <node pkg="pacmod_interface" exec="pacmod_additional_debug_publisher" name="pacmod_additional_debug_publisher" output="screen">

src/vehicle/external/pacmod_interface/pacmod_interface/launch/pacmod_steer_test.launch.xml
6:  <arg name="pacmod_param_path" default="$(find-pkg-share pacmod_interface)/config/pacmod_steer_test.param.yaml"/>
15:  <node pkg="pacmod_interface" exec="pacmod_steer_test" name="pacmod_steer_test" output="screen">

src/vehicle/external/pacmod_interface/pacmod_interface/src/pacmod_interface/pacmod_interface_node.cpp
15:#include <pacmod_interface/pacmod_interface.hpp>

src/vehicle/external/pacmod_interface/pacmod_interface/src/pacmod_interface/pacmod_diag_publisher_node.cpp
15:#include <pacmod_interface/pacmod_diag_publisher.hpp>

src/vehicle/external/pacmod_interface/pacmod_interface/src/pacmod_interface/pacmod_diag_publisher.cpp
15:#include <pacmod_interface/pacmod_diag_publisher.hpp>

src/vehicle/external/pacmod_interface/pacmod_interface/src/pacmod_interface/pacmod_interface.cpp
15:#include <pacmod_interface/pacmod_interface.hpp>
23:: Node("pacmod_interface"),

autoware.repos
132:  vehicle/external/pacmod_interface:
134:    url: https://github.com/tier4/pacmod_interface.git

@mitsudome-r
Copy link
Member

@xmfcx
At least for pacmod, we can remove it since it's not being used.
For Tamagawa IMU, I think we can remove it as well. For the sample_sensor_kit_launch, we can replace the driver part with a comment with psuedo code like the following:

<!--
      <node pkg="imu_driver" name="imu_driver" exec="imu_driver" if="$(var launch_driver)">
        <remap from="data_raw" to="imu_raw"/>
        <param name="port" value="/dev/imu"/>
        <param name="imu_frame_id" value="imu_link"/>
      </node>
-->

@xmfcx
Copy link
Contributor Author

xmfcx commented Feb 17, 2025

Ok, I'll create PRs that will move these 2 into their own .repos file then 👍

@esteve
Copy link
Contributor

esteve commented Feb 17, 2025

@xmfcx do you want me to move the repos? I'm happy to work on that, feel free to assign me this ticket.

@xmfcx
Copy link
Contributor Author

xmfcx commented Feb 18, 2025

@esteve Sure, you may work on it if you are available 👍

The tasks are in order:

  1. Move the pacmod_interface into extra-packages.repos
    • The Autoware doesn't have any reference to this package, easy to just move.
  2. Update the following to also vcs import src < extra-packages.repos
  3. Remove references of tamagawa_imu_driver from:
  4. Move the tamagawa_imu_driver into extra-packages.repos
  5. Update the installation documentation to include information about extra-packages.repos

Additional points:

  • For now, extra-packages.repos doesn't need to have a -nightly version.

I'll update the main post with these steps.

@xmfcx xmfcx assigned esteve and unassigned xmfcx Feb 18, 2025
@esteve
Copy link
Contributor

esteve commented Feb 18, 2025

@xmfcx thanks for the detailed explanation

  1. Move the pacmod_interface into extra-packages.repos

    • The Autoware doesn't have any reference to this package, easy to just move.

In that case, should the packages in that repos file be built? If they're not used, we can just submit them to the ROS buildfarm and we wouldn't have to deal with them.

@xmfcx
Copy link
Contributor Author

xmfcx commented Feb 18, 2025

I think that would also work. @mitsudome-r if you don't have objections, I'll change the task to move the pacmod_interface to the ros build farm.

@esteve
Copy link
Contributor

esteve commented Feb 18, 2025

@xmfcx when you have a moment, could you create repositories for each of these two packages? At first, could you allow pushes to the main branch, I'll push the code from here and will keep the commit history with git-filter-repo. Once I've pushed the history, we can protect the main branch against pushes. Thanks.

@esteve
Copy link
Contributor

esteve commented Feb 18, 2025

@xmfcx sorry, I thought I didn't have permission to create new repositories, I'll create them myself.

@esteve
Copy link
Contributor

esteve commented Feb 18, 2025

@xmfcx I just realized that the packages are already in their own repositories 🤦 Sorry for all the noise

@xmfcx
Copy link
Contributor Author

xmfcx commented Feb 18, 2025

I just realized that the packages are already in their own repositories

Same here 💀

@esteve
Copy link
Contributor

esteve commented Feb 19, 2025

@xmfcx I believe all the PRs are ready for review, except for the changes in awsim_sensor_kit_launch , because it's a fork of awsim_labs_sensor_kit_launch and I can't fork that repo twice. I'll open a new PR once the one for awsim_sensor_kit_launch is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:ci Continuous Integration (CI) processes and testing. type:installation Issues or improvements related to the installation process of the software. type:new-feature New functionalities or additions, feature requests.
Projects
None yet
Development

No branches or pull requests

4 participants