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(concatedate_data): add error handling when sensor points is empty (backport #3814) #526

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

h-ohta
Copy link

@h-ohta h-ohta commented May 26, 2023

Description

autowarefoundation#3814 のbackport

Tests performed

同じ内容の修正になっていること
このPRにより点群のwidthが0の場合にsensingにおいてノードが落ちないことを確認

Effects on system behavior

Not applicable.

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.

…autowarefoundation#3814)

fix(concatedate_date): add error handling when sensor points are empty
@codecov-commenter
Copy link

codecov-commenter commented May 26, 2023

Codecov Report

❗ No coverage uploaded for pull request base (beta/v0.3.16@774bd17). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@              Coverage Diff               @@
##             beta/v0.3.16    #526   +/-   ##
==============================================
  Coverage                ?   0.00%           
==============================================
  Files                   ?      36           
  Lines                   ?    2825           
  Branches                ?       0           
==============================================
  Hits                    ?       0           
  Misses                  ?    2825           
  Partials                ?       0           
Flag Coverage Δ
differential 0.00% <0.00%> (?)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@h-ohta h-ohta changed the title fix(concatedate_date): add error handling when sensor points is empty (#3814) fix(concatedate_data): add error handling when sensor points is empty (#3814) May 26, 2023
@h-ohta h-ohta changed the title fix(concatedate_data): add error handling when sensor points is empty (#3814) fix(concatedate_data): add error handling when sensor points is empty (backport #3814) Jun 9, 2023
@h-ohta h-ohta enabled auto-merge (squash) June 26, 2023 03:00
Copy link

@yn-mrse yn-mrse left a comment

Choose a reason for hiding this comment

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

@h-ohta

  • sensor points is emptyな状況は正常系で発生する理解でしょうか?(この事象は車両を停車させる必要はないか?)
    もしシステム異常として発生し、かつそれが危険事象をもたらすものとすると、このノードないし後段のノードでシステムエラーとして検知する必要があると思われます。
    そうでないのであれば特に問題ないかと思います。
  • fix(concatedate_date): add error handling when sensor points is empty autowarefoundation/autoware.universe#3814 では単体テストは実施されているように読み取れますが、システムレイヤでのテストが実施されているかどうかが不明です。こちらはテストされていますでしょうか?(あるいはテストの計画がありますでしょうか?)
    後段のノードでも同様のエラーが発生してしまう懸念があると思われますので、その可能性があるのか否かを明確にしたいです。

@h-ohta
Copy link
Author

h-ohta commented Jun 26, 2023

@yn-mrse

sensor points is emptyな状況は正常系で発生する理解でしょうか?(この事象は車両を停車させる必要はないか?)
もしシステム異常として発生し、かつそれが危険事象をもたらすものとすると、このノードないし後段のノードでシステムエラーとして検知する必要があると思われます。
そうでないのであれば特に問題ないかと思います。

正常系では発生しない理解です。発生する場合はセンサ故障が疑われる事象かと思います。
どちらかというと、センサ側で検知する必要があるのは同意です
あくまで今回の修正は、そういった場合にノードが落ちることを防ぐものになります。

autowarefoundation#3814 では単体テストは実施されているように読み取れますが、システムレイヤでのテストが実施されているかどうかが不明です。こちらはテストされていますでしょうか?(あるいはテストの計画がありますでしょうか?)
後段のノードでも同様のエラーが発生してしまう懸念があると思われますので、その可能性があるのか否かを明確にしたいです。

システムレイヤとしてのテストがどこまでを指すかが不明確なので回答が難しいのですが、sensing全体を通した動作確認を指すのであれば、そちらは一度確認しています。
その中でこのノードが落ちたため、対処を施して再度確認しています。

@yn-mrse
Copy link

yn-mrse commented Jun 26, 2023

システムレイヤとしてのテストがどこまでを指すかが不明確なので回答が難しいのですが、sensing全体を通した動作確認を指すのであれば、そちらは一度確認しています。
その中でこのノードが落ちたため、対処を施して再度確認しています。

ありがとうございます!
Test performed欄にsensing全体に対する効果確認済みと記載いただければ大丈夫かと思います。

@h-ohta
Copy link
Author

h-ohta commented Jun 26, 2023

Test performed欄にsensing全体に対する効果確認済みと記載いただければ大丈夫かと思います。

これは今回のPRについて、でしょうか?

@h-ohta
Copy link
Author

h-ohta commented Jun 26, 2023

@yn-mrse 記載をしてみたので確認お願いします

@yn-mrse
Copy link

yn-mrse commented Jun 27, 2023

@h-ohta

正常系では発生しない理解です。発生する場合はセンサ故障が疑われる事象かと思います。
どちらかというと、センサ側で検知する必要があるのは同意です
あくまで今回の修正は、そういった場合にノードが落ちることを防ぐものになります。

少なからず変更前については、(意図していないにせよ)センサ故障により発生するリスクに対してMRMで対処できていたと思われます。
したがって、このPRではこのリスクに対する何かしらの言及(リスクは十分小さいのか、対応価値があるか、など)が必要と思われます。

@yn-mrse
Copy link

yn-mrse commented Jun 27, 2023

記載をしてみたので確認お願いします

このPRにより点群のwidthが0の場合にsensingにおいてノードが落ちないことを確認

こちらの記載については問題ありません!

@h-ohta
Copy link
Author

h-ohta commented Jun 27, 2023

懸念が残るため導入を見送ります

@h-ohta h-ohta closed this Jun 27, 2023
auto-merge was automatically disabled June 27, 2023 01:29

Pull request was closed

@h-ohta h-ohta deleted the backport/3814 branch June 27, 2023 01:29
@h-ohta h-ohta restored the backport/3814 branch June 28, 2023 03:05
@h-ohta h-ohta reopened this Jun 28, 2023
@h-ohta
Copy link
Author

h-ohta commented Jun 28, 2023

@yn-mrse
こちらのPRについて取得される点群が0になるのは故障時以外にもありえると思われます。
例えば、正常時にセンサが点群を取得できないようなときは以下が考えられそうです

  • 物体が遠方にある場合
  • 物体が至近距離にある場合
  • 初期化時に点群なしでトピックを出力するセンサがある場合

こういった場合に正常なのにノードが落ちてしまうのは問題なので、導入する方向で進めたいです

@h-ohta h-ohta enabled auto-merge (squash) June 28, 2023 03:44
Copy link

@yn-mrse yn-mrse left a comment

Choose a reason for hiding this comment

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

そもそも故障を一意に特定できるエラー事象ではなく、これによる可用性損失が生じているため、本機能を入れることは妥当と判断します。

本機能を入れることによる安全性への影響は気になりますが、そもそもFMEAを実施のうえ各種監視モジュールを動作させている背景があるため、監視機能としては既存の監視モジュールで賄われているものと考えます。

@h-ohta h-ohta merged commit 3fafc95 into beta/v0.3.16 Jun 28, 2023
7 of 9 checks passed
@h-ohta h-ohta deleted the backport/3814 branch June 28, 2023 03:45
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.

3 participants