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

fix: add blockage for xx1 #191

Merged
merged 13 commits into from
Mar 19, 2024

Conversation

badai-nguyen
Copy link
Contributor

@badai-nguyen badai-nguyen commented Dec 21, 2023

This to update aip_launch param as change in autowarefoundation/autoware.universe#5932 and enable blockage_diag for xx1.

Signed-off-by: badai-nguyen <[email protected]>
Signed-off-by: badai-nguyen <[email protected]>
@badai-nguyen badai-nguyen marked this pull request as ready for review December 26, 2023 11:22
aip_x2_launch/launch/lidar.launch.xml Outdated Show resolved Hide resolved
aip_x2_launch/launch/pandar_node_container.launch.py Outdated Show resolved Hide resolved
@@ -24,6 +25,12 @@
<arg name="vehicle_mirror_param_file" value="$(var vehicle_mirror_param_file)"/>
<arg name="use_pointcloud_container" value="$(var use_pointcloud_container)"/>
<arg name="container_name" value="$(var pointcloud_container_name)"/>
<arg name="angle_range" value="[0.0, 360.0]"/>
<arg name="vertical_bins" value="128"/>
<arg name="horizontal_ring_id" value="64"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q]
Is ring_id: 64 correct?
Has this algorithm taken into account the arrangement pattern of the lasers?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miursh
As confirmation in Rviz, the velodyne ring IDs are arranged in the order as the assumption of the current algorithm that ring_id will be whether Top to Bottom or Bottom to Top.
In VLS128 the order is from the bottom.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@badai-nguyen
Ah, I see. Understood.
However, is 64 is really horizontal?
Typically, there are more downward-facing lasers than upward-facing ones, so I doubt that the center would be horizontal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miursh Thank you for your comment.
Currently, I checked on the kind of empty environment and I think 64 is acceptable.
image
image

I think it also could be adjusted if any feedback after the function is enabled for operation.

@shmpwk
Copy link
Contributor

shmpwk commented Jan 22, 2024

@badai-nguyen Could you resolve conflicts?

@badai-nguyen
Copy link
Contributor Author

badai-nguyen commented Feb 27, 2024

@miursh @kyoichi-sugahara Since the nebula azimuth correction bug still under investigation, so I change the default of xx1 blockage to false here and hope can merge together with autowarefoundation/autoware.universe#5932.
Could you approve this PR?

Here is CI test result: https://evaluation.tier4.jp/evaluation/reports/44e36320-0062-5b34-9533-968fec2a958b?project_id=prd_jt

@kyoichi-sugahara kyoichi-sugahara self-assigned this Feb 28, 2024
Copy link
Contributor

@kyoichi-sugahara kyoichi-sugahara left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: badai-nguyen <[email protected]>
@badai-nguyen
Copy link
Contributor Author

@kyoichi-sugahara As our internal discusion, I also added the parameters for velodyne_xxx.launch.xml to make it more clearly here 57612b6
Could you approve the PR again.

@badai-nguyen
Copy link
Contributor Author

@kyoichi-sugahara I have separated aip_x2_launch's parameters into PR #223
Could you also review it?

Copy link
Contributor

@kyoichi-sugahara kyoichi-sugahara left a comment

Choose a reason for hiding this comment

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

LGTM!

@badai-nguyen badai-nguyen merged commit c47e3e8 into tier4:tier4/universe Mar 19, 2024
7 of 9 checks passed
badai-nguyen added a commit to badai-nguyen/aip_launcher that referenced this pull request Mar 21, 2024
* fix: enable blocakge diag for xx1

Signed-off-by: badai-nguyen <[email protected]>

* fix: xx1 launch

Signed-off-by: badai-nguyen <[email protected]>

* fix: x2 launch

Signed-off-by: badai-nguyen <[email protected]>

* ci(pre-commit): autofix

* fix: remove duplicated angle_range param

Signed-off-by: badai-nguyen <[email protected]>

* fix: remove x2 redundant param

Signed-off-by: badai-nguyen <[email protected]>

* chore: update config file

Signed-off-by: badai-nguyen <[email protected]>

* chore: disable xx1 blockage as default

Signed-off-by: badai-nguyen <[email protected]>

* fix: velodyne launch

Signed-off-by: badai-nguyen <[email protected]>

* Revert "fix: remove x2 redundant param"

This reverts commit fa93588.

* Revert "fix: x2 launch"

This reverts commit 4748c45.

---------

Signed-off-by: badai-nguyen <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

4 participants