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

feat(tier4_system_rviz_plugin): improve visualization results and logics #5222

Conversation

Owen-Liuyuxuan
Copy link
Contributor

@Owen-Liuyuxuan Owen-Liuyuxuan commented Oct 4, 2023

Description

The great PR #4945 added the visualization functionality for HazardStatus messages, which is important in daily testing. However, there are two problems that make the RVIz plugin not perfect enough for everyday usage.

  1. The plugin will print out a large amount of errors before we provide the initial localization and mission planning.
  2. The plugin will keep printing out information when there are no error messages, which occupies quite a part of the window while providing limited information.

In this PR, we

  1. Completly clean up text outputs when there is simply no error received.
  2. When the system_error_monitor does not consider the situation to be fault one (the hazard_level of the message is NO_FAULT), we do not display any error messages.

Related links

Tests performed

PSim

  1. No text print out when running normally or just started
    cleanedup_when_no_error

  2. Print out text as the original PR when errors/warnings received.
    onlyDisplay_when_needed

Notes for reviewers

Interface changes

NA

Effects on system behavior

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:common Common packages from the autoware-common repository. (auto-assigned) label Oct 4, 2023
@Owen-Liuyuxuan Owen-Liuyuxuan changed the title Feat(tier4_system_rviz_plugin): improve visualization results / logics Feat(tier4_system_rviz_plugin): improve visualization results and logics Oct 4, 2023
@Owen-Liuyuxuan Owen-Liuyuxuan changed the title Feat(tier4_system_rviz_plugin): improve visualization results and logics feat(tier4_system_rviz_plugin): improve visualization results and logics Oct 4, 2023
Owen-Liuyuxuan and others added 5 commits October 4, 2023 18:02
Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Understood the idea behind this, and I am definitely for it.

However, IMHO, using the diag topics for detecting if the planning and localization is initialized or not is a bit hacky, making the system overcomplicated. For example if the diag name in the subscribed topic changes, this plugin may not work properly.

Instead, I would recommend using ADAPI instead, which is a formal way of knowing the state of the Autoware system. You may refer to https://autowarefoundation.github.io/autoware-documentation/main/design/autoware-interfaces/ad-api/.

@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Oct 6, 2023
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 
@kminoda How about you?

@kminoda
Copy link
Contributor

kminoda commented Oct 11, 2023

Sorry, let me review this by tomorrow. Thanks!

@kminoda kminoda self-requested a review October 11, 2023 07:17
Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM except the following minor comment 👍

@kminoda kminoda self-requested a review October 12, 2023 08:57
@kminoda kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Oct 12, 2023
Signed-off-by: Owen-Liuyuxuan <[email protected]>
Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

LGTM

@shmpwk shmpwk enabled auto-merge (squash) October 12, 2023 12:19
@shmpwk shmpwk merged commit 4ba8923 into autowarefoundation:main Oct 12, 2023
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants