jsk-ros-pkg / jsk_common

common programs for jsk-ros-pkg
43 stars 81 forks source link

[jsk_topic_tools/timered_diagnostics_updater] Use force_update instead of update because of update called using TimeredDiagnosticUpdater's timer #1752

Closed iory closed 2 years ago

iory commented 2 years ago

What is this?

The timed diagnostic updater used in DiagnosticNodelet and relaynodelet updates the diagnostic for a predetermined duration using timer. However, the diagnostic_updater that TimeredDiagnosticUpdater has determines whether an update is actually performed based on its own timer in the update function. https://github.com/ros/diagnostics/blob/dbc73ff508813ec13eaf23594006ccb9b61d083c/diagnostic_updater/include/diagnostic_updater/diagnostic_updater.h#L383-L392

      void update()
      {
        ros::Time now_time = ros::Time::now();
        if (now_time < next_time_) {
          // @todo put this back in after fix of #2157 update_diagnostic_period(); // Will be checked in force_update otherwise.
          return;
        }

        force_update();
      }

Since TimedDiagnosticUpdater is already managed as a timer, diagnosticupdater->force_update should be executed instead of diagnosticupdater->update.

cc: @708yamaguchi

708yamaguchi commented 2 years ago

fetch15 branch https://github.com/knorth55/jsk_common/pull/24

The original update() function definition. ros/diagnostics@dbc73ff/diagnostic_updater/include/diagnostic_updater/diagnostic_updater.h#L383-L392

Without this PR, diagnostic update function (e.g. jsk-ros-pkg/jsk_common@505fb0a/jsk_topic_tools/src/relay_nodelet.cpp#L70-L71) is sometimes not called at given timer callback period.

iory commented 2 years ago

Thanks for your review!