Add starting_up_state parameter to Updater#354
Conversation
ct2034
added
bug
|
Hi! Can I get an update on this one? If it wasn't merged because of this test case failing, can I get some help understanding the error? I'm sure a few minutes of someone who already understands these template-based tests can save me hours of trial and error. |
|
Hi @redvinaa.
On the other hand, I also would not like my system starting in an error state. Your issue could also be solved by additional state, such as proposed in ros2/common_interfaces#268. What is your opinion about that? |
ct2034
added
enhancement
|
Hi! Sorry for the long delay. It's possible that it would make sense to add new states, but that's a huge change and it appears to be moving very slowly. This PR on the other hand is not breaking, and I think is still useful until the new states get added. |
redvinaa
force-pushed
the
starting-up-state
branch
from
692fd7c to
687c0c4
Compare
| if (starting_up_status_ < 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
what is the purpose of this line?
There was a problem hiding this comment.
I don't remember anymore, removing
ct2034
added
the
backport
| explicit Updater(NodeT node, double period = 1.0) | ||
| explicit Updater( | ||
| NodeT node, double period = 1.0, | ||
| int starting_up_status = diagnostic_msgs::msg::DiagnosticStatus::OK) |
There was a problem hiding this comment.
| int starting_up_status = diagnostic_msgs::msg::DiagnosticStatus::OK) | |
| unsigned char starting_up_status = diagnostic_msgs::msg::DiagnosticStatus::OK) |
| std::shared_ptr<rclcpp::node_interfaces::NodeTopicsInterface> topics_interface, | ||
| double period = 1.0); | ||
| double period = 1.0, | ||
| int starting_up_status = diagnostic_msgs::msg::DiagnosticStatus::OK); |
There was a problem hiding this comment.
| int starting_up_status = diagnostic_msgs::msg::DiagnosticStatus::OK); | |
| unsigned char starting_up_status = diagnostic_msgs::msg::DiagnosticStatus::OK); |
| rclcpp::Publisher<diagnostic_msgs::msg::DiagnosticArray>::SharedPtr publisher_; | ||
| rclcpp::Logger logger_; | ||
|
|
||
| int starting_up_status_; |
There was a problem hiding this comment.
| int starting_up_status_; | |
| unsigned char starting_up_status_; |
| std::shared_ptr<rclcpp::node_interfaces::NodeTopicsInterface> topics_interface, double period) | ||
| std::shared_ptr<rclcpp::node_interfaces::NodeTopicsInterface> topics_interface, | ||
| double period, | ||
| int starting_up_status) |
There was a problem hiding this comment.
| int starting_up_status) | |
| unsigned char starting_up_status) |
ct2034
added
the
needs more work
2 participants
In the current implementation, when the Updater object is constructed, it sends off an "OK" signal. This could be problematic if we consider "Everything is running as expected" (definition of "OK" status as per REP107), that running is already initialized.
The default behavior doesn't change with this PR, so it wouldn't break anything.
Also, in the python implementation, None can be passed to skip "Node starting up" status publishing altogether.