fix(ntp_monitor): move chronyc command execution to a timer #4634

ito-san · 2023-08-15T09:20:19Z

Description

The diagnostic topic of ntp_monitor sometimes may not be sent periodically, every 1 second.
The root cause is that the execution of the chronyc tracking command gets stuck when the network connection is poor.

So the execution of the chronyc will be separated from the main thread using a separate thread along with a timer.
In addition, a feature will be added to monitor timeouts in chronyc and to send a WARNING notification when a timeout occurs.
The timeout duration is set to 5 seconds by default.

Tests performed

Run Autoware.

Run rqt_runtime_monitor.

ros2 run rqt_runtime_monitor rqt_runtime_monitor

Verify OK is reported during normal operation.
Verify Warning is reported when chronyc tracking gets stuck.

Notes for reviewers

None

Interface changes

None

Effects on system behavior

None

Pre-review checklist for the PR author

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

I've confirmed the contribution guidelines.
The PR follows the pull request guidelines.

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.

Signed-off-by: ito-san <fumihito.ito@tier4.jp>

codecov · 2023-08-23T02:34:35Z

Codecov Report

Patch coverage has no change and project coverage change: -0.25% ⚠️

Comparison is base (bf03fe3) 15.02% compared to head (93ca423) 14.77%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4634      +/-   ##
==========================================
- Coverage   15.02%   14.77%   -0.25%     
==========================================
  Files        1609     1546      -63     
  Lines      111684   106282    -5402     
  Branches    34605    32472    -2133     
==========================================
- Hits        16784    15707    -1077     
+ Misses      76201    73338    -2863     
+ Partials    18699    17237    -1462

Flag	Coverage Δ		*Carryforward flag
differential	`0.00% <0.00%> (?)`
total	`14.78% <ø> (-0.25%)`	⬇️	Carriedforward from 4c33a4a

*This pull request uses carry forward flags. Click here to find out more.

Files Changed	Coverage Δ
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp	`0.00% <ø> (ø)`
...tem/system_monitor/src/ntp_monitor/ntp_monitor.cpp	`0.00% <0.00%> (ø)`

... and 419 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

h-ohta · 2023-09-19T08:33:15Z

Could you add parameter to param.yaml with detail comments?
https://github.com/autowarefoundation/autoware.universe/blob/4c33a4a3011a2ac840922ead44d5dce5f14fcf99/system/system_monitor/config/ntp_monitor.param.yaml

ito-san · 2023-09-19T09:45:19Z

Could you add parameter to param.yaml with detail comments? https://github.com/autowarefoundation/autoware.universe/blob/4c33a4a3011a2ac840922ead44d5dce5f14fcf99/system/system_monitor/config/ntp_monitor.param.yaml

@h-ohta
You mean the newly added parameter timeout?

ito-san · 2023-09-22T06:38:54Z

@h-ohta
You mean the newly added parameter timeout?

@h-ohta
Would you please answer the question?

h-ohta · 2023-09-22T06:40:57Z

@ito-san Yes

ito-san · 2023-09-22T09:01:00Z

@ito-san Yes

@h-ohta Fixed in 94d4e90

h-ohta · 2023-09-25T02:23:24Z

I checked this PR with following procedures.

Run system_monitor with ros2 launch

$ ros2 launch system_monitor system_monitor.launch.xml # for working in multi process

Run rqt_runtime_monitor.

$ ros2 run rqt_runtime_monitor rqt_runtime_monitor

Confirm during normal operation

Confirm during abnormal operation, that is command stucking

I changed the command chronyc tracking to sleep 10.

autoware.universe/system/system_monitor/src/ntp_monitor/ntp_monitor.cpp

Line 188 in 93ca423

bp::child c("chronyc tracking", bp::std_out > is_out);

And I found out that ntp_monitor turned to warning state.

ito-san · 2023-09-25T02:26:54Z

@asana17 Could you review this PR as the code owner?

asana17

LGTM.

…foundation#4634) * fix(ntp_monitor): move chronyc command execution to a timer Signed-off-by: ito-san <fumihito.ito@tier4.jp> * add newly added parameter timeout to config --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: Hiroki OTA <hiroki.ota@tier4.jp>

…utowarefoundation#4634) (#880) fix(ntp_monitor): move chronyc command execution to a timer (autowarefoundation#4634) * fix(ntp_monitor): move chronyc command execution to a timer * add newly added parameter timeout to config --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com>

…utowarefoundation#4634) (#880) fix(ntp_monitor): move chronyc command execution to a timer (autowarefoundation#4634) * fix(ntp_monitor): move chronyc command execution to a timer * add newly added parameter timeout to config --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com> (cherry picked from commit 2199f69)

…utowarefoundation#4634) (backport #880) (#885) fix(ntp_monitor): move chronyc command execution to a timer (backport autowarefoundation#4634) (#880) fix(ntp_monitor): move chronyc command execution to a timer (autowarefoundation#4634) * fix(ntp_monitor): move chronyc command execution to a timer * add newly added parameter timeout to config --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com> (cherry picked from commit 2199f69) Co-authored-by: Hiroki OTA <hiroki.ota@tier4.jp>

…utowarefoundation#4634) (#880) fix(ntp_monitor): move chronyc command execution to a timer (autowarefoundation#4634) * fix(ntp_monitor): move chronyc command execution to a timer * add newly added parameter timeout to config --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com>

* feat(system_error_monitor): manual modules (#793) * feat(rviz_plugin): console meter is too large on the Rviz with FHD display, isn't it? (#587) * feat(tier4_planning/vehicle_plugin): make plugins size scalable Signed-off-by: Takamasa Horibe <horibe.takamasa@gmail.com> * remove space Signed-off-by: Takamasa Horibe <horibe.takamasa@gmail.com> * scaling Signed-off-by: Takamasa Horibe <horibe.takamasa@gmail.com> * change diag message Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * fix module name Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * add manual module and ignoring modules Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * Revert "feat(rviz_plugin): console meter is too large on the Rviz with FHD display, isn't it? (#587)" This reverts commit f96169c. * Revert "change diag message" This reverts commit dff01ce. * ci(pre-commit): autofix * fix spell check Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * Revert "fix spell check" This reverts commit 208aa1e. * Revert "fix module name" This reverts commit cec7653. * revert ignore module Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * current_mode check update Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * ci(pre-commit): autofix * delete margin Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> --------- Signed-off-by: Takamasa Horibe <horibe.takamasa@gmail.com> Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> Co-authored-by: Takamasa Horibe <horibe.takamasa@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat(ad_service_state_monitor): change configs name (#876) change configs name Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * feat(plannig_error_monitor): update error sharp angle threshold (#681) * fix error_sharp_angle Signed-off-by: Shigekazu Fukuta <shigekazu.fukuta@tier4.jp> * update readme Signed-off-by: Shigekazu Fukuta <shigekazu.fukuta@tier4.jp> --------- Signed-off-by: Shigekazu Fukuta <shigekazu.fukuta@tier4.jp> * fix: add error handling when path is invalid (#934) * fix(behavior_path): delete duplicated * add error handling * fix: when path size is 1 * fix(detection_area): search collision index only in lanelet (#695) * fix(detection_area): search collision index only in lanelet * ci(pre-commit): autofix --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(detection_area): fix overline function (#930) * fix(detection_area): fix overline function * ci(pre-commit): autofix --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(route_handler): fix threshold for removing overlapping points (#1015) * fix(route_handler): fix threshold for removing overlapping points * fix * fix(ntp_monitor): move chronyc command execution to a timer (backport autowarefoundation#4634) (#880) fix(ntp_monitor): move chronyc command execution to a timer (autowarefoundation#4634) * fix(ntp_monitor): move chronyc command execution to a timer * add newly added parameter timeout to config --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com> * feat(elevation_map_loader): add error handling for std::runtime_error (backport autowarefoundation#4187) (#652) feat(elevation_map_loader): add error handling for std::runtime_error (autowarefoundation#4187) * feat(elevation_map_loader): Add error handling for std::runtime_error * feat(elevation_map_loader): add error message output --------- Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com> Co-authored-by: Shintaro Tomie <58775300+Shin-kyoto@users.noreply.github.com> * fix(system_monitor): extend command line to display (backport autowarefoundation#4553) (#768) fix(system_monitor): extend command line to display (autowarefoundation#4553) Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com> * fix(system_monitor): high-memory process are not provided in MEM order (backport autowarefoundation#4654) (#769) fix(system_monitor): high-memory process are not provided in MEM order (autowarefoundation#4654) * fix(process_monitor): high-memory process are not being provided in %MEM order * changed option from 'g' to 'n' --------- Signed-off-by: ito-san <fumihito.ito@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com> * fix(system_monitor): fix program command line reading (backport autowarefoundation#5191, autowarefoundation#5430) (#995) * perf(system_monitor): fix program command line reading (autowarefoundation#5191) * Fix program command line reading Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> * style(pre-commit): autofix * fix spelling commandline->command_line Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> --------- Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> Co-authored-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * fix(system_monitor): output command line (autowarefoundation#5430) * fix(system_monitor): output command line Signed-off-by: takeshi.iwanari <takeshi.iwanari@tier4.jp> * style(pre-commit): autofix --------- Signed-off-by: takeshi.iwanari <takeshi.iwanari@tier4.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --------- Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> Signed-off-by: takeshi.iwanari <takeshi.iwanari@tier4.jp> Co-authored-by: Yuxuan Liu <619684051@qq.com> Co-authored-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: takeshi-iwanari <takeshi.iwanari@tier4.jp> Co-authored-by: Akihisa Nagata <54956813+asa-naki@users.noreply.github.com> * feat(imu_corrector): add gyro_bias_validator (backport autowarefoundation#4729) (#856) * feat(imu_corrector): add gyro_bias_validator (backport autowarefoundation#4729) * feat(imu_corrector): add gyro_bias_validator Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * update Signed-off-by: kminoda <koji.minoda@tier4.jp> * revert launch Signed-off-by: kminoda <koji.minoda@tier4.jp> * updat Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * add debug publisher Signed-off-by: kminoda <koji.minoda@tier4.jp> * minor fix Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * style(pre-commit): autofix * add gtest Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * updat e readme Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * add diagnostics Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * update Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * validator -> estimator Signed-off-by: kminoda <koji.minoda@tier4.jp> * fix build Signed-off-by: kminoda <koji.minoda@tier4.jp> * update default parameter Signed-off-by: kminoda <koji.minoda@tier4.jp> * update comment Signed-off-by: kminoda <koji.minoda@tier4.jp> * update readme Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * updated Signed-off-by: kminoda <koji.minoda@tier4.jp> * minor update in readme Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * fix pre-commit Signed-off-by: kminoda <koji.minoda@tier4.jp> * update readme Signed-off-by: kminoda <koji.minoda@tier4.jp> * style(pre-commit): autofix * Fix NG -> WARN Signed-off-by: kminoda <koji.minoda@tier4.jp> --------- Signed-off-by: kminoda <koji.minoda@tier4.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * build(imu_corrector): add missing diagnostic_updater dependency (autowarefoundation#4980) Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp> * add gyro_bias estimation in diag ( autowarefoundation#5054) Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * ci(pre-commit): autofix --------- Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp> Co-authored-by: kminoda <44218668+kminoda@users.noreply.github.com> Co-authored-by: Esteve Fernandez <33620+esteve@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat(imu_corrector): add gyro bias log (#918) add gyro_bias log Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * feat(system_error_monitor): add ignore_until_waiting_for_route module (#888) * add ignore_module Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * add description Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * ci(pre-commit): autofix * change name ignore_until_waiting_for_route Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * update description Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * rename function name and delete planning state Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * update description Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * Update --------- Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Hiroki OTA <hiroki.ota@tier4.jp> * feat(system_error_monitor): add ignore hartbeat timeout in initializing state (#972) * add ignore hartbeat timeout in initializing state Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> * fix typo * Update comment * ci(pre-commit): autofix * fix typo * ci(pre-commit): autofix * update comment * ci(pre-commit): autofix --------- Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --------- Signed-off-by: Takamasa Horibe <horibe.takamasa@gmail.com> Signed-off-by: asa-naki <akihisa.nagata@tier4.jp> Signed-off-by: Shigekazu Fukuta <shigekazu.fukuta@tier4.jp> Signed-off-by: ito-san <fumihito.ito@tier4.jp> Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com> Signed-off-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> Signed-off-by: takeshi.iwanari <takeshi.iwanari@tier4.jp> Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp> Co-authored-by: Takamasa Horibe <horibe.takamasa@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Shigekazu Fukuta <107168699+sfukuta@users.noreply.github.com> Co-authored-by: Hiroki OTA <hiroki.ota@tier4.jp> Co-authored-by: ito-san <57388357+ito-san@users.noreply.github.com> Co-authored-by: Shintaro Tomie <58775300+Shin-kyoto@users.noreply.github.com> Co-authored-by: Yuxuan Liu <619684051@qq.com> Co-authored-by: Owen-Liuyuxuan <uken.ryu@tier4.jp> Co-authored-by: takeshi-iwanari <takeshi.iwanari@tier4.jp> Co-authored-by: kminoda <44218668+kminoda@users.noreply.github.com> Co-authored-by: Esteve Fernandez <33620+esteve@users.noreply.github.com>

fix(ntp_monitor): move chronyc command execution to a timer

7179da2

Signed-off-by: ito-san <fumihito.ito@tier4.jp>

ito-san added type:bug Software flaws or errors. component:system System design and integration. (auto-assigned) labels Aug 15, 2023

ito-san requested review from h-ohta and yn-mrse August 15, 2023 09:20

ito-san self-assigned this Aug 15, 2023

ito-san requested a review from asana17 as a code owner August 15, 2023 09:20

h-ohta and others added 2 commits August 23, 2023 10:00

Merge branch 'main' into fix/ntp_monitor

6123943

Merge branch 'main' into fix/ntp_monitor

490e080

ito-san added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 23, 2023

Merge branch 'main' into fix/ntp_monitor

4c33a4a

ito-san added 2 commits September 22, 2023 16:13

add newly added parameter timeout to config

94d4e90

Merge branch 'main' into fix/ntp_monitor

93ca423

h-ohta approved these changes Sep 25, 2023

View reviewed changes

asana17 approved these changes Sep 26, 2023

View reviewed changes

ito-san merged commit 05f8ad2 into autowarefoundation:main Sep 26, 2023
21 of 25 checks passed

ito-san deleted the fix/ntp_monitor branch September 26, 2023 15:03

tier4-autoware-public-bot bot mentioned this pull request Sep 26, 2023

chore: sync upstream tier4/autoware.universe#878

Merged

h-ohta mentioned this pull request Sep 27, 2023

fix(ntp_monitor): move chronyc command execution to a timer (backport #4634) tier4/autoware.universe#880

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ntp_monitor): move chronyc command execution to a timer #4634

fix(ntp_monitor): move chronyc command execution to a timer #4634

ito-san commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

h-ohta commented Sep 19, 2023

ito-san commented Sep 19, 2023 •

edited

Loading

ito-san commented Sep 22, 2023

h-ohta commented Sep 22, 2023

ito-san commented Sep 22, 2023

h-ohta commented Sep 25, 2023

ito-san commented Sep 25, 2023

asana17 left a comment

fix(ntp_monitor): move chronyc command execution to a timer #4634

fix(ntp_monitor): move chronyc command execution to a timer #4634

Conversation

ito-san commented Aug 15, 2023 • edited Loading

Description

Related links

Tests performed

Notes for reviewers

Interface changes

Effects on system behavior

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

Post-review checklist for the PR author

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

h-ohta commented Sep 19, 2023

ito-san commented Sep 19, 2023 • edited Loading

ito-san commented Sep 22, 2023

h-ohta commented Sep 22, 2023

ito-san commented Sep 22, 2023

h-ohta commented Sep 25, 2023

ito-san commented Sep 25, 2023

asana17 left a comment

Choose a reason for hiding this comment

ito-san commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

ito-san commented Sep 19, 2023 •

edited

Loading