autoware.universe
autoware.universe copied to clipboard
refactor(emergency_handler, mrm_handler) change MRM operators to classes inside Handler Nodes
Description
Delete MRM operators and implemented those operators inside Handlers (Emergency Handler, MRM Handler) as classes based on https://github.com/autowarefoundation/autoware.universe/issues/6539 .
Related links
https://github.com/autowarefoundation/autoware.universe/issues/6539
Tests performed
Not yet.
Notes for reviewers
Please review especially in terms of:
- namespace structure
-
update_rate
of handlers- Previously, it was 10 Hz in
emergency_handler
, while 33 Hz inemergency_stop_operator
.
- Previously, it was 10 Hz in
- how to pass parameters to each operator
- When to publish emergency commands in emergency_stop operator class
- only in
operate()
or bothoperate()
andonTimer()
?
- only in
- Are commands
gear
andhazard_light
used only whenemergency_stop
is enabled?- If so, should they included in
emergency_stop
class, not in handlers?
- If so, should they included in
Interface changes
We cannot access MRM operators directly. All inputs/outputs of operators are included in the Handler's interfaces.
Effects on system behavior
Handlers do not have to call ROS 2 services anymore to invoke mrm oprations. It changes to function calls and thus mrm operations can work more quickly in terms of:
- no need to wait next timer callback
- no need to wait ROS 2 scheduler to schedule
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
- [x] I've confirmed the contribution guidelines.
- [x] The PR follows the pull request guidelines.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
- [ ] The PR follows the pull request guidelines.
- [ ] The PR has been properly tested.
- [ ] The PR has been reviewed by the code owners.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
- [ ] There are no open discussions or they are tracked via tickets.
- [ ] The PR is ready for merge.
After all checkboxes are checked, anyone who has write access can merge the PR.
As soon as this node is started up, the process dies. I don't know where the bug is, but you should look into the code.
@TetsuKawa
Thank you for your comment.
If the error message is what(): Statically typed parameter 'hoge' must be initialized.
or something like this, then adding some parameters to the top-level configuration file (autoware/src/launcher/autoware_launch/autoware_launch/config/system/emergency_handler/emergency_handler.param.yaml
) works well.
@veqcc I'm sorry. You are right, the higher level parameter file was loaded.
@isamu-takagi Thank you for your helpful comments!
namespace structure
Do you mean comfortable_stop_operator in emergency_handler? It looks fine. FYI: It can be written as namespace emergency_handler::comfortable_stop_operator
Yes I meant it. I have fixed the namespace following it.
emergency_stop_operator. how to pass parameters to each operator
Where is this about? I think it would be better to use declare_parameter in each operator class, as is the current implementation.
Yes I meant the parameters in configuration files. Thank you for your review.
@isamu-takagi
Are commands gear and hazard_light used only when emergency_stop is enabled? If so, should they included in emergency_stop class, not in handlers?
Yes. At the meeting the other day, we decided to publish a command for each MRM, so please move it in the operator class. It should be needed for comfortable_stop as well.
I see. I will move these parameters and functions to operators from handlers.
One question here is, is it neccesary to modify vehicle_cmd_gate
to enable these commands even when comfortable_stop
?
It seems the vehicle_cmd_gate
chooses emergency commands only when emergency_stop
is being operated.
If so, I will make another Pull Request for it after this PR is merged.
One question here is, is it neccesary to modify vehicle_cmd_gate to enable these commands even when comfortable_stop? It seems the vehicle_cmd_gate chooses emergency commands only when emergency_stop is being operated. If so, I will make another Pull Request for it after this PR is merged.
@veqcc That's right. So for this PR, it is enough to move the command publishers to the emergency_stop operator class.
update_rate of handlers Previously, it was 10 Hz in emergency_handler, while 33 Hz in
Since it changes the publishing rate of control commands, please check whether it has any effect on vehicle control. Also, it is better to treat the topic rate and processing rate separately.
I have changed following your advice. Now emergency_handler
works in 10Hz, while emergency_stop_operator
works in 30Hz as before.
When to publish emergency commands in emergency_stop operator class. Only in operate() or both operate() and onTimer() ?
I think it's better to always publish if there are no performance problems.
Both the current implementaion and my modification always publish them. There are no performance problem in Psim on my computer, though I don't know in full Autoware on actual vehicles. Until any performance problem happens, I will keep the current one.
This pull request has been automatically marked as stale because it has not had recent activity.
I willl close it.