Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion diagnostic_updater/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ if(BUILD_TESTING)

find_package(ament_cmake_pytest REQUIRED)
ament_add_pytest_test(diagnostic_updater_test.py "test/diagnostic_updater_test.py")
ament_add_pytest_test(test_DiagnosticStatusWrapper.py "test/test_DiagnosticStatusWrapper.py")
ament_add_pytest_test(test_DiagnosticStatusWrapper.py "test/test_diagnostic_status_wrapper.py")
endif()

ament_python_install_package(${PROJECT_NAME})
Expand Down
68 changes: 31 additions & 37 deletions diagnostic_updater/diagnostic_updater/_diagnostic_updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,47 +222,26 @@ class Updater(DiagnosticTaskVector):
should be called frequently. At some predetermined rate, the update
function will cause all the diagnostic tasks to run, and will collate
and publish the resulting diagnostics. The publication rate is
determined by the "~diagnostic_period" ros parameter.
The class also allows an update to be forced when something significant
has happened, and allows a single message to be broadcast on all the
diagnostics if normal operation of the node is suspended for some
reason.
determined by the "~/diagnostic_updater.period" ros2 parameter.
The force_update function can always be triggered async to the period interval.
"""

def __init__(self, node):
def __init__(self, node, period=1.0):
"""Construct an updater class."""
DiagnosticTaskVector.__init__(self)
self.node = node
self.publisher = self.node.create_publisher(DiagnosticArray, '/diagnostics', 1)
self.clock = Clock()
now = self.clock.now()

self.last_time = now

self.last_time_period_checked = self.last_time
self.period_parameter = 'diagnostic_updater.period'
self.period = self.node.declare_parameter(self.period_parameter, 1.0).value
self.__period = self.node.declare_parameter(self.period_parameter, period).value
self.timer = self.node.create_timer(self.__period, self.update)

self.verbose = False
self.hwid = ''
self.warn_nohwid_done = False

def update(self):
"""Causes the diagnostics to update if the inter-update interval has been exceeded."""
self._check_diagnostic_period()
now = self.clock.now()
if now >= self.last_time:
self.force_update()

def force_update(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did the timer refactoring make it necessary to drop force_update (in python and c++)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the update function didn't really do anything if the period wasn't exceeded yet. So I believe it doesn't make sense to have a force_update function, which basically then just calls update.
If you wanted to force an update, you could always just call update on your diagnostic updater instance

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My understanding of the original implementation was, that force_update() will always update, but update() will only call force_update() when the period was exceeded. See https://github.com/ros/diagnostics/blob/ros2-devel/diagnostic_updater/diagnostic_updater/_diagnostic_updater.py#L254-L255

This would mean that the two methods indeed have a different behavior. Calling update() with twice the configured rate will still only send updates in the configrued rate, calling force_update() with twice the configured rate will actually send updates with twice the configured rate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I see what you mean. In 18f5c77 I have re-introduced the force_update method to make it more explicit. The result of this is that the update function becomes private to avoid any confusion.

"""
Force the diagnostics to update.

Useful if the node has undergone a drastic state change that should be
published immediately.
"""
self.last_time = self.clock.now()

warn_nohwid = len(self.hwid) == 0

status_vec = []
Expand Down Expand Up @@ -297,6 +276,20 @@ def force_update(self):

self.publish(status_vec)

@property
def period(self):
return self.__period

@period.setter
def period(self, period):
self.__period = period
self.timer.reset()
self.timer = self.node.creat_timer(self.__period, self.udpate)

def force_update(self):
"""Force sending out an update for all known DiagnosticStatus."""
self.update()

def broadcast(self, lvl, msg):
"""
Output a message on all the known DiagnosticStatus.
Expand All @@ -318,17 +311,18 @@ def broadcast(self, lvl, msg):
def setHardwareID(self, hwid):
self.hwid = hwid

def _check_diagnostic_period(self):
"""Recheck the diagnostic_period on the parameter server."""
# This was getParamCached() call in the cpp code. i.e. it would throttle
# the actual call to the parameter server using a notification of change
# mechanism.
# This is not available in rospy. Hence I throttle the call to the
# parameter server using a standard timeout mechanism (4Hz)
now = self.clock.now()
if now >= self.last_time_period_checked:
self.period = self.node.get_parameter(self.period_parameter).value
self.last_time_period_checked = now
# TODO(Karsten1987) Re-enable this for eloquent
# def _check_diagnostic_period(self):
# """Recheck the diagnostic_period on the parameter server."""
# # This was getParamCached() call in the cpp code. i.e. it would throttle
# # the actual call to the parameter server using a notification of change
# # mechanism.
# # This is not available in rospy. Hence I throttle the call to the
# # parameter server using a standard timeout mechanism (4Hz)
# now = self.clock.now()
# if now >= self.last_time_period_checked:
# # self.period = self.node.get_parameter(self.period_parameter).value
# self.last_time_period_checked = now

def publish(self, msg):
"""Publish a single diagnostic status or a vector of diagnostic statuses."""
Expand Down
5 changes: 1 addition & 4 deletions diagnostic_updater/diagnostic_updater/example.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ def main():
if not updater.removeByName('Bound check'):
node.get_logger().error('The Bound check task was not found when trying to remove it.')

msg = std_msgs.msg.Bool()
while rclpy.ok():
msg = std_msgs.msg.Bool()
sleep(0.1)

# Calls to pub1 have to be accompanied by calls to pub1_freq to keep
Expand All @@ -244,9 +244,6 @@ def main():
pub1.publish(msg)
pub1_freq.tick()

# We can call updater.update whenever is convenient. It will take care
# of rate-limiting the updates.
updater.update()
rclpy.spin_once(node, timeout_sec=1)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@
* @author Blaise Gassend
*/

#ifndef DIAGNOSTIC_UPDATER__DIAGNOSTICSTATUSWRAPPER_HPP_
#define DIAGNOSTIC_UPDATER__DIAGNOSTICSTATUSWRAPPER_HPP_
#ifndef DIAGNOSTIC_UPDATER__DIAGNOSTIC_STATUS_WRAPPER_HPP_
#define DIAGNOSTIC_UPDATER__DIAGNOSTIC_STATUS_WRAPPER_HPP_

#include <stdarg.h>
#include <cstdio>
Expand Down Expand Up @@ -295,4 +295,4 @@ DiagnosticStatusWrapper::addf(
va_end(va);
}
} // namespace diagnostic_updater
#endif // DIAGNOSTIC_UPDATER__DIAGNOSTICSTATUSWRAPPER_HPP_
#endif // DIAGNOSTIC_UPDATER__DIAGNOSTIC_STATUS_WRAPPER_HPP_
Loading