issue 815 hardware interface add return value #933
Conversation
There was a problem hiding this comment.
Thanks for your contribution!
Firstly, sorry for the poorly described issue. Looking more into this, I am not sure anymore what's the best solution here.
Besides that, what you are missing is actually returning “SKIPPING” from the hardware interfaces. Currently, an error is returned. To have better clarity on the issue, we should actually start implementing this proposal about command interfaces only available when active. This would lead to the following implementations:
- Add URDF parser for those interfaces – make command interfaces being not available when
inactiveby default - Add additional
write_when_activemethod for hardware interfaces that is called inactivestate.
This would give us much more perspective and idea if this additional return value is needed and how it should be implemented.
Besides that, if you are extending the range of return values, then please consider #884. To make the extension (ABI braking) into one batch.
| else if (failed_hardware_names.size() > 0) | ||
| { | ||
| // Status is ok but some hardware is not ok (SKIPPED) | ||
| // Determine controllers to skip | ||
| for (const auto & hardware_name : failed_hardware_names) | ||
| { | ||
| auto controllers = resource_manager_->get_cached_controllers_to_hardware(hardware_name); | ||
| controllers_to_skip.insert(controllers_to_skip.end(), controllers.begin(), controllers.end()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is needed here. Hardware interface is not failed. The issue here is that we are calling read and write from HW interfaces constantly, and in some cases we skip it (when not inactive or active). But then the controllers cannot run at all since they don't have interfaces to connect to – so I doubt that we have to do anything with them here.
| @@ -1074,9 +1074,13 @@ HardwareReadWriteStatus ResourceManager::read( | |||
| { | |||
| if (component.read(time, period) != return_type::OK) | |||
There was a problem hiding this comment.
Why not simply change this?
| if (component.read(time, period) != return_type::OK) | |
| if (component.read(time, period) == return_type::ERROR) |
|
Hi @destogl ! Thanks for your answer! My attempt here is to try and solve problem I am facing with the best practice from hardware_interface `hardware_interface::return_type MySystemHardware::read(const rclcpp::Time & time, const rclcpp::Duration & period) bool hardware_go = main_loop_update_rate_ == 0 || if (hardware_go){ } // update counter Using this I need to from here comes the This PR is an attempt to stop the ros2_controller to publish the data as long as there is no new data available at the rate where the It appears that I misunderstood the issue here. Maybe I should open another issue? |
…terface-add-return-value
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #933 +/- ##
==========================================
- Coverage 31.83% 31.80% -0.03%
==========================================
Files 94 94
Lines 10483 10494 +11
Branches 7136 7144 +8
==========================================
+ Hits 3337 3338 +1
- Misses 801 805 +4
- Partials 6345 6351 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…terface-add-return-value
…terface-add-return-value
…terface-add-return-value
|
Closing because doesn't solve the mentioned issues, and it is not a clean approach to solve the issue user has. If there are the problems with hardware so that data is not written, then the controllers have to be stopped. |
In my Opinion this solve the issue #815