ros2 / rcutils

Common C functions and data structures used in ROS 2
Apache License 2.0
58 stars 101 forks source link

Logging macro conditional on a latch/flipflop #456

Open russkel opened 8 months ago

russkel commented 8 months ago

A common pattern of logging I have come across in hardware integration is a flipflop kind of pattern:

e.g. Hardware callback reports at 10Hz "BAD" for X seconds, and then reports "GOOD" when hardware state good:

void hardware_callback(const std::string state) {
  if (state == "BAD") {
  RCLCPP_INFO(get_logger(), "hardware not ready yet");
 }
  if (state == "GOOD") {
  RCLCPP_INFO(get_logger(), "System in good state, ready.");
 }
}

Would spam debug msgs:

hardware not ready yet
hardware not ready yet
hardware not ready yet
hardware not ready yet
// and finally system swaps over
System in good state, ready.
System in good state, ready.
System in good state, ready.
System in good state, ready.
System in good state, ready.
// and then back again
hardware not ready yet
hardware not ready yet
hardware not ready yet
hardware not ready yet
hardware not ready yet

Ideally it would be something like:

void hardware_callback(const std::string state) {
  if (state == "BAD") {
  RCLCPP_INFO_LATCH(get_logger(), hw_state_latch, true, "hardware not ready yet");
 }
  if (state == "GOOD") {
  RCLCPP_INFO_LATCH(get_logger(), hw_state_latch, false, "System in good state, ready.");
 }
}

Yielding:

hardware not ready yet
// and finally system swaps over
System in good state, ready.
// and then back again
hardware not ready yet

I'd be willing to write this feature if someone could give me instructions on how it would be implemented in the rcutils framework, and if it would actually be merged.

I am not too sure on how to define the static/global that can be referenced in other functions.

Barry-SH commented 8 months ago

Currently, the log provides the function RCLCPP_INFO_ONCE(), but it doesn't fully meet your needs.
If you want to implement it, you can refer to the implementation of RCLCPP_INFO_ONCE()

I'm curious why not implement it with code instead of relying on log functions? Implementing it by the user would also be simple.

russkel commented 8 months ago

Thanks.

I'm curious why not implement it with code instead of relying on log functions? Implementing it by the user would also be simple.

You could say that about all the log macros.

We have ended up implementing many "simple" things throughout our code base in relation to rclcpp functionality and it get tiring writing out the same boilerplate. I intend to follow the same approach here but I wanted to see if it was possible using log macros. If it can, then great I will create a PR, if not, it goes into our private libraries never seeing the light of day.

Barry-SH commented 8 months ago

I have no reason to oppose adding such a feature to the log function. It brings convenience to the users. Let's hear the opinions of other community members.

fujitatomoya commented 8 months ago

@russkel thanks for creating issue.

hardware not ready yet
// and finally system swaps over
System in good state, ready.
// and then back again
hardware not ready yet

i guess RCLCPP_INFO_ONCE would not be useful with above use case, that state gets back to the previous state.

maybe there are some constraints, but i would not use topics to share the hardware or sensor state.

instead,

russkel commented 8 months ago

maybe there are some constraints, but i would not use topics to share the hardware or sensor state.

This was an example (and it's dealing with the data stream such as serial from a hardware sensor), this is pretty common where sensors will report their state at > 1Hz. There's no publishing in the example.

I imagine there's a few use cases outside of this example too.