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_msgs): add emergency holding msg #153

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

TetsuKawa
Copy link
Contributor

@TetsuKawa TetsuKawa commented Nov 11, 2024

Related Links

Purpose of the message

https://star4.slack.com/archives/C017VB9UG1L/p1729675438504999

↑Details of this chat

Currently, on X1, there is an issue where Autoware is in an Emergency state, but an Error notification is not displayed on the FMS. The condition for displaying an Error on the FMS is that the emergency_holding value within the hazard_status topic is set to true.

This issue is caused by the fact that when the configuration of system_error_monitor and emergency_handler was changed to the configuration of diagnostic_graph_aggregator, mrm_handler, and hazard_status_converter(discussion), the implementation of hazard_status_converter was set so that the emergency_holding value in the published hazard_status topic is always false.

The emergency_holding function, which previously existed in the system_error_monitor, has now been moved to the mrm_handler. Therefore, to update the emergency_holding value in the hazard_status topic published by the hazard_status_converter, it is necessary to transmit the emergency_holding state from the mrm_handler. This is why the current PR is required to address the issue.

Which node is meant to publish/subscribe the topic?

Pub: mrm_handler(PR
Sub: hazard_status_converter(PR

Does this message has to be in a separate message?

This message is temporary. In the future, both hazard_status_converter and hazard_status will be deprecated, and hazard_status will no longer be used as an API. However, due to the aforementioned issue, a temporary topic addition will be made.

Currently, there is a topic called /system/operation_mode/availability from mrm_handler to hazard_status_converter, but adding the emergency_holding state to this topic would be inappropriate given its intended content. Since this is only a temporary value until hazard_status_converter is deprecated, the topic should be created independently.

Description

This PR adds EmergencyHodingState msg.

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Code is properly formatted
  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • Code is properly formatted
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets
  • Write release notes

CI Checks

  • Build and test for PR: Required to pass before the merge.
  • Check spelling: NOT required to pass before the merge. It is up to the reviewer(s). See here if you want to add some words to the spell check dictionary.

@mitsudome-r
Copy link
Collaborator

Are there any documentation about how this message is used?
The description in the PR is not enough for me to review this.

For example, I would like to know the following information:

  • Which node is meant to publish/subscribe the topic?
  • What does is the purpose of the message? (How does Autoware behave depending on true/false in this message?)
  • Does this message has to be in a separate message?

@TetsuKawa
Copy link
Contributor Author

@mitsudome-r
Apologies for the lack of explanation. I have revised the PR comment based on the example you provided. Please feel free to contact me if there are any points that are unclear.

@mitsudome-r
Copy link
Collaborator

@TetsuKawa Thanks.

This message is temporary. In the future, both hazard_status_converter and hazard_status will be deprecated, and hazard_status will no longer be used as an API. However, due to the aforementioned issue, a temporary topic addition will be made.

If this is the case, it might be better to leave a comment at the beginning of message definition so that the users will know that this message will also be deprecated in the future. (Something like the follwing)

# This message will be deprecated once hazard_status_converter and hazard_status are deprecated. 

@TetsuKawa
Copy link
Contributor Author

@mitsudome-r

If this is the case, it might be better to leave a comment at the beginning of message definition so that the users will know that this message will also be deprecated in the future. (Something like the follwing)

fixed in a89d58a

@mitsudome-r mitsudome-r merged commit b94e3df into tier4/universe Nov 14, 2024
9 of 10 checks passed
@mitsudome-r mitsudome-r deleted the feat/add-emergency-holding-msg branch November 14, 2024 04:23
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.

2 participants