Fix battery drain crash for GoToPlace #94
Conversation
Signed-off-by: Yadunund <yadunund@openrobotics.org>
There was a problem hiding this comment.
Thanks for jumping on this problem.
I think for both cases we should do
const auto new_battery_soc = finish.battery_soc().value() - travel->change_in_charge();
if (new_battery_soc < 0.0)
return std::nullopt;
finish.battery_soc(new_battery_soc);
instead of doing finish.battery_soc(std::max(0.0, ...)).
In the PR as it currently stands, there's a very narrow edge case where the user might have specified a battery_threshold <= 0.0, in which case a negative final battery charge would get disguised as a charge of 0.0 which is technically within the threshold, so the task would be performed even though it should be discarded.
Admittedly if the user is specifying a battery threshold of <= 0.0 then they're probably doing something very wrong already, but we should avoid making that bad situation even worse by allowing tasks to be performed when they won't even be able to finish.
Signed-off-by: Yadunund <yadunund@openrobotics.org>
|
|
||
| if (state.battery_soc().value() <= _invariant_battery_drain) | ||
| if (state.battery_soc().value() <= constraints.threshold_soc()) |
There was a problem hiding this comment.
Corrected this bug too.
|
Thanks for the quick feedback and catching the edge case. I've updated the code as suggested. Also addressed this other issue in |
| @@ -115,9 +115,13 @@ WaitFor::Model::Model( | |||
| { | |||
| if (parameters.ambient_sink()) | |||
| { | |||
| // Handle cases where duration is invalid. | |||
| const auto duration = | |||
| _duration.count() < 0 ? rmf_traffic::Duration(0) : _duration; | |||
There was a problem hiding this comment.
This is a really great catch!
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Addresses open-rmf/rmf#359