-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(components): improve component error handling #138
base: main
Are you sure you want to change the base?
Conversation
We could also tackle this differently: We could keep the implementation of the |
I like the second suggestion, but I would add some thoughts. Normal components do not really have discrete states and being "in error state" carries no meaning outside of the predicate being set to true. So, for it to be meaningful, we could also stop the step timer or skip the step callback once an error has been raised. We might also consider if it's meaningful to "reset" an error but I think that's very implementation specific and not suited for a component (rather, use a lifecycle component for that). The error predicate should just be a way for components to stop themselves, and the predicate can correspondingly be used to unload them if we want. And by stopping the step callback, it has actual value in being defined here in modulo, since it would also stop publishers. And regarding lifecycle components, raising an error should put the component into the ERROR_PROCESSING transition state, which in turn triggers the on_error callback. This can be entered either through transition callbacks (i.e. if we raise an exception during So overall, when it comes to the necessary change, I would:
|
c05e4eb
to
0d1b840
Compare
So I tested that with lifecycle components and what happens is that
The component is in state |
0d1b840
to
6fda888
Compare
In general, subscriptions will survive these steps, especially in a Component. Do we want to clear those as well? |
That's what we want, yes. The |
Actually the default virtual on_error_callback in the base class should return false. Only if it is overridden to implement actual error recovery should it return true |
That is quite important for us as well though, we need to carefully see what we should reset and what not. |
source/modulo_components/include/modulo_components/Component.hpp
Outdated
Show resolved
Hide resolved
source/modulo_components/include/modulo_components/LifecycleComponent.hpp
Outdated
Show resolved
Hide resolved
if (!this->handle_shutdown()) { | ||
break; | ||
} | ||
// TODO: reset and finalize all needed properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're already here we do also need to think about that quickly
source/modulo_components/modulo_components/lifecycle_component.py
Outdated
Show resolved
Hide resolved
@@ -13,7 +13,8 @@ LifecycleComponent::LifecycleComponent(const rclcpp::NodeOptions& node_options, | |||
LifecycleNode::get_node_parameters_interface(), LifecycleNode::get_node_services_interface(), | |||
LifecycleNode::get_node_time_source_interface(), LifecycleNode::get_node_timers_interface(), | |||
LifecycleNode::get_node_topics_interface(), LifecycleNode::get_node_type_descriptions_interface(), | |||
LifecycleNode::get_node_waitables_interface())) {} | |||
LifecycleNode::get_node_waitables_interface())), | |||
has_error_(true) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_error_(true) {} | |
has_error_(false) {} |
b3005ec
to
7f8422b
Compare
Description
I wouldn't say that this PR solves the parent issue but at least it removes a feature that is not fully supported, desired and thought through.
Review guidelines
Estimated Time of Review: 5 minutes
Checklist before merging: