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_adapi_rviz_plugin): add legacy state panel #7494

Merged

Conversation

isamu-takagi
Copy link
Contributor

@isamu-takagi isamu-takagi commented Jun 14, 2024

Description

Adding legacy state panel because #7036 reduces debug functionality.

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Interface changes

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.

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.

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

@isamu-takagi isamu-takagi self-assigned this Jun 14, 2024
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:common Common packages from the autoware-common repository. (auto-assigned) labels Jun 14, 2024
@isamu-takagi isamu-takagi added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jun 17, 2024
@isamu-takagi isamu-takagi merged commit a6b5f27 into autowarefoundation:main Jun 17, 2024
38 checks passed
@isamu-takagi isamu-takagi deleted the feat/legacy-state-panel branch June 17, 2024 02:47
simon-eisenmann-driveblocks pushed a commit to simon-eisenmann-driveblocks/autoware.universe that referenced this pull request Jun 26, 2024
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Sep 20, 2024

@isamu-takagi I've just realized this was added back.

Why do you think the current one is reducing the debug functionality?

image

Could you provide feedback?

@xmfcx
Copy link
Contributor

xmfcx commented Sep 20, 2024

Which information is missing from the new panel?

@armaganarsln
Copy link

I also don’t understand why, instead of addressing the issue of what changes are required for better debug functionality, the state panel was added back without consulting.

@isamu-takagi
Copy link
Contributor Author

@xmfcx @armaganarsln
All I really needed was the operation_mode part, but I copied the old source code because I could use it as is. Sorry for the confusion of having two panels with same functions.

This is redundant in normal use and is not necessary for the tier4_state_rviz_plugin, but we needed to display the following to check the correctness of the implementation. I will later extract the relevant part as a panel dedicated to debugging.

  • The current mode when is_mode_available is false.

Screenshot from 2024-09-21 09-22-50
Screenshot from 2024-09-21 09-23-12
Screenshot from 2024-09-21 09-24-11

  • The destination mode during transition.

Screenshot from 2024-09-21 09-26-29
Screenshot from 2024-09-21 09-26-56

@xmfcx
Copy link
Contributor

xmfcx commented Sep 23, 2024

I see, these cases are not represented correctly in the new version.

(I also understand why you needed the old version now, thanks. I've left a comment in #8933 as well.)

cc. @KhalilSelyan @isamu-takagi

Could you confirm if the proposed changes would make it better?

⚠️ Current mode should be always the selected button on the segmented buttons control.

Stop button should be selected, rest should be disabled

image

Stop button should be selected, rest should be enabled

image

Auto button should be selected, rest should be disabled

image

Auto button should be selected, rest should be enabled

image


As an additional question:

Should we allow users to press buttons during the transitions?

In your images, during:

  • Stop(Transition)
  • Autonomous(Transition)
    The user can press the buttons.

@isamu-takagi
Copy link
Contributor Author

@xmfcx

Could you confirm if the proposed changes would make it better?

Yes, it's good that these are visible to the user.

  • Stop button should be selected, rest should be disabled
  • Stop button should be selected, rest should be enabled
  • Auto button should be selected, rest should be disabled
  • Auto button should be selected, rest should be enabled

Should we allow users to press buttons during the transitions?

No, changing modes during a transition is not an expected usage. It is better to allow this operation only from the debug panel.

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) tag: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.

4 participants