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(aip_x2): gen2 sensor_kit #244

Merged
merged 9 commits into from
Jul 5, 2024
Merged

feat(aip_x2): gen2 sensor_kit #244

merged 9 commits into from
Jul 5, 2024

Conversation

h-ohta
Copy link
Contributor

@h-ohta h-ohta commented Jun 24, 2024 •
edited
Loading

Descriptions

以下資料を元に、センサのTFを設定する
https://drive.google.com/file/d/1ZbleWG9UrZwdZxrJE5kHrq1eDmQCKw1M/view
https://drive.google.com/file/d/1lBT2S4Rmfv_YBe0LHSFkkFRoTz_NMUIg/view

Remarks

  • センサ基準（sensor_kit_base_link）はtop leftのLiDARとし、姿勢はbase_linkに合わせる
  • LiDARはコネクタを車両側に来るように姿勢を調整
  • レーダーはbase_linkに接続する

Related Links

https://tier4.atlassian.net/browse/RT0-31480

Tests Performed

目視にて全センサTFを確認した
Rvizにて位置が概ね正しいことも確認した

image

@h-ohta h-ohta force-pushed the feat/gen2_sensor_kit branch from 66322bf to 6e15eda Compare June 25, 2024 02:06
@h-ohta h-ohta changed the title feat: add sensor_kit feat(aip_x2): gen2 sensor_kit Jun 25, 2024
@TomohitoAndo TomohitoAndo changed the base branch from x2-v3.0.0-gen2 to x2-gen2-v3.0.0 June 25, 2024 04:23
@h-ohta h-ohta force-pushed the feat/gen2_sensor_kit branch from 3e36267 to c41709f Compare June 25, 2024 04:56
@h-ohta h-ohta force-pushed the feat/gen2_sensor_kit branch from b1f1064 to 0c4af2d Compare June 25, 2024 05:33
@h-ohta h-ohta requested a review from TomohitoAndo June 25, 2024 05:38
@h-ohta h-ohta marked this pull request as ready for review June 25, 2024 05:39
@TomohitoAndo TomohitoAndo requested review from yabuta and knzo25 June 26, 2024 12:19
@knzo25
Copy link
Contributor

knzo25 commented Jun 27, 2024

センサーのモデル・vendorを使わない方がいいと思います。
この変更は絶対にtier4/universe (branch)にマージされないですよね？

@h-ohta
Copy link
Contributor Author

h-ohta commented Jun 28, 2024

モデル、vendor名が入らないように修正します
この変更はgen2向けなので、そちらにはマージされる予定です

@knzo25
Copy link
Contributor

knzo25 commented Jul 1, 2024

@h-ohta
すみません、個人的に 548 に入って違和感したのは548が変わる可能性があります（540 -> 548 -> ?）ので避けたほうがいいと思いました。
しかし、名前を選ぶのは今のリストの方々ではないと思います。自分たち意見出せるけれど、それまでです。

昔、自分が似たような作業したとき、 @aohsato が相談に乗っていただきました。現在だと、適任者わからないです（前回は入社の直後なのでよく覚えていないです）

@h-ohta
Copy link
Contributor Author

h-ohta commented Jul 1, 2024

@Kenzo ありがとうございます。確かに誰が命名規則を決めるのかと言う問題はありますね

@TomohitoAndo 命名規則について確認してもらってもいいでしょうか。安藤くんで決めていいならみてもらえると

TomohitoAndo
TomohitoAndo reviewed Jul 4, 2024
View reviewed changes
aip_x2_description/urdf/sensor_kit.xacro Outdated
</link>

<!-- left upper-->
<xacro:PandarQT
Copy link
Contributor

Choose a reason for hiding this comment

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

PandarQT -> PandarOT-128
他のupper lidarも同様です

Copy link
Contributor

Choose a reason for hiding this comment

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

@h-ohta
こちらもPandarOT-128のxacroはすでにあるので修正してもらってもよいでしょうか？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c842492

TomohitoAndo
TomohitoAndo reviewed Jul 4, 2024
View reviewed changes
aip_x2_description/urdf/sensor_kit.xacro Outdated
yaw="${calibration['sensor_kit_base_link']['top_left/lidar_base_link']['yaw']}"
/>

<xacro:PandarQT
Copy link
Contributor

@TomohitoAndo TomohitoAndo Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

PandarQT -> PandarQT-128
他のlower lidarも同様です

PandarQT-128はこちらのPRで作成
tier4/sensor_component_description#44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e494a9b

@TomohitoAndo
Copy link
Contributor

TomohitoAndo commented Jul 4, 2024 •
edited
Loading

tfの命名に関してですが、 @aohsato さんからはキャリブレーションツール対応側で問題なければ特に懸念ないとコメントいただいているので、SIのほうで決めます。

@aohsato もしtfの命名についても懸念あるようなら、コメントいただけますと幸いです。

@TomohitoAndo
Copy link
Contributor

@h-ohta
LiDARの命名はaip_launcherと同様に、以下のようにしたいです。

  • [front, right, left, rear]_upper (OT128)
  • [front, right, left, rear]_lower (QT128

こうするとleftとrightのsensorsでlidarだけtopがついてないことになるので、
以下のようにtop_left -> left, top_right -> rightとなるよう修正いただいてもよいでしょうか？ 🙇
image

@h-ohta
Copy link
Contributor Author

h-ohta commented Jul 4, 2024

@TomohitoAndo
4b8659c
こういうことであってます？

@TomohitoAndo
Copy link
Contributor

TomohitoAndo commented Jul 4, 2024 •
edited
Loading

@h-ohta
合ってます！
それに加えて、lidarもupper（名前後ろに追加）とlower (bottom置き換え)で分けたいです。

  • [front, right, left, rear]_upper (OT128)
  • [front, right, left, rear]_lower (QT128

@h-ohta
Copy link
Contributor Author

h-ohta commented Jul 4, 2024

@TomohitoAndo 対応しました

@TomohitoAndo TomohitoAndo self-requested a review July 5, 2024 01:36
TomohitoAndo
TomohitoAndo approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@TomohitoAndo TomohitoAndo left a comment

Choose a reason for hiding this comment

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

LGTM!

image
image

@TomohitoAndo TomohitoAndo merged commit ca35839 into x2-gen2-v3.0.0 Jul 5, 2024
9 of 10 checks passed
@TomohitoAndo TomohitoAndo deleted the feat/gen2_sensor_kit branch July 5, 2024 01:44
@TomohitoAndo TomohitoAndo mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomohitoAndo TomohitoAndo TomohitoAndo approved these changes

@yabuta yabuta Awaiting requested review from yabuta

@knzo25 knzo25 Awaiting requested review from knzo25

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
@h-ohta @knzo25 @TomohitoAndo