search

open-rmf / rmf_task

RMF library for managing task allocations
Apache License 2.0
22 stars 22 forks source link

Fix battery consumption modeling for perform_action tasks #76

Closed luca-della-vedova closed 1 year ago

luca-della-vedova commented 1 year ago

Bug fix

Fixed bug

Battery state of charge update in perform_action tasks didn't do boundary check to make sure it was setting a valid level (> 0.0). For this reason, if a perform_action task that would drain the battery below 0% was submitted, rmf_task would throw an exception:

Battery State of Charge needs to be between 0.0 and 1.0.

instead of just inserting a charging task. This PR (actual fix by @Yadunund) adds a unit test that failed before and works now, to act as a regression test in the future as well. The test for checking charge battery tasks also had its logic flipped and wasn't actually testing anything, fixed that as well but happy to move it in another PR if it's out of scope.

Fix applied

Make sure we set the battery level to a valid value when updating it in perform_action

luca-della-vedova commented 1 year ago

Thanks for tracking down this bug and adding a helpful test!

Couple requested changes before merge

1. Update rmf_task_sequence/CMakeLists.txt with a [BUILD_TESTING](https://github.com/open-rmf/rmf_task/blob/4e3fcc0765e72f5408f18a3b125050074cc5a665/rmf_task/CMakeLists.txt#L54-L80) section that will run this test as part of `colcon test`.

2. Nit: Can we rename `test_Compose.cpp` to `test_ComposedTaskPlanning.cpp` for better clarity?

3. Update CHANGELOG

Renamed the file and updated the CHANGELOG. I believe the test is already ran as part of colcon test, the rmf_task_sequence CMakeLists file has a BUILD_TESTING section that adds recursively all .cpp files here