Add finished() API and fix bug for cancelling finished tasks (#58)

Signed-off-by: Michael X. Grey <[email protected]>

rmf_task/include/rmf_task/Task.hpp

@@ -201,6 +201,10 @@ class Task::Active
   /// Get a quick overview status of how the task is going
   virtual Event::Status status_overview() const = 0;
 
+  /// Check if this task is finished, which could include successful completion
+  /// or cancellation.
+  virtual bool finished() const = 0;
+
   /// Descriptions of the phases that have been completed
   virtual const std::vector<Phase::ConstCompletedPtr>&
   completed_phases() const = 0;

rmf_task/test/mock/MockTask.cpp

@@ -28,6 +28,15 @@ rmf_task::Event::Status MockTask::Active::status_overview() const
   return rmf_task::Event::Status::Completed;
 }
 
+//==============================================================================
+bool MockTask::Active::finished() const
+{
+  if (_active_phase)
+    _active_phase->final_event()->finished();
+
+  return true;
+}
+
 //==============================================================================
 auto MockTask::Active::completed_phases() const
 -> const std::vector<Phase::ConstCompletedPtr>&

rmf_task/test/mock/MockTask.hpp

@@ -37,6 +37,8 @@ class MockTask : public rmf_task::Task
 
     rmf_task::Event::Status status_overview() const override;
 
+    bool finished() const override;
+
     const std::vector<Phase::ConstCompletedPtr>&
     completed_phases() const override;
 

rmf_task_sequence/schemas/backup/backup_PhaseSequenceTask_v0_1.schema.json

@@ -3,38 +3,57 @@
   "$id": "https://open-rmf.org/rmf_task_sequence/backup_PhaseSequenceTask/0.1",
   "title": "Phase Sequence Task Backup",
   "description": "A backup state for a task which is defined by a fixed sequence of phases",
-  "properties": {
-    "schema_version": {
-      "description": "The version of the Phase Sequence Task Backup schema being used",
-      "const": "0.1"
-    },
-    "current_phase": {
-      "description": "The current phase of the task when the backup occurred",
+  "oneOf": [
+    {
       "properties": {
-        "id": {
-          "description": "The integer ID of the phase",
+        "schema_version": {
+          "description": "The version of the Phase Sequence Task Backup schema being used",
           "type": "integer",
-          "minimum": 0
+          "enum": [1]
         },
-        "cancelled_from": {
-          "description": "The integer ID of the phase that was cancelled to reach the current phase",
-          "type": "integer",
-          "minimum": 0
+        "current_phase": {
+          "description": "The current phase of the task when the backup occurred",
+          "properties": {
+            "id": {
+              "description": "The integer ID of the phase",
+              "type": "integer",
+              "minimum": 0
+            },
+            "cancelled_from": {
+              "description": "The integer ID of the phase that was cancelled to reach the current phase",
+              "type": "integer",
+              "minimum": 0
+            },
+            "state": {
+              "description": "The serialized state of the backed up current phase"
+            }
+          },
+          "required": [ "id", "state" ]
         },
-        "state": {
-          "description": "The serialized state of the backed up current phase"
+        "skip_phases": {
+          "description": "A list of pending phases that are supposed to be skipped",
+          "type": "array",
+          "items": {
+            "type": "integer",
+            "minimum": 0
+          }
         }
       },
-      "required": [ "id", "state" ]
+      "required": [ "schema_version", "current_phase" ]
     },
-    "skip_phases": {
-      "description": "A list of pending phases that are supposed to be skipped",
-      "type": "array",
-      "items": {
-        "type": "integer",
-        "minimum": 0
-      }
+    {
+      "properties": {
+        "schema_version": {
+          "description": "The version of the Phase Sequence Task Backup schema being used",
+          "type": "integer",
+          "enum": [1]
+        },
+        "finished": {
+          "description": "True if the task is finished, or false if the task has not started",
+          "type": "boolean"
+        }
+      },
+      "required": [ "schema_version", "finished" ]
     }
-  },
-  "required": [ "schema_version", "current_phase" ]
+  ]
 }

rmf_task_sequence/src/rmf_task_sequence/Task.cpp

@@ -167,6 +167,9 @@ class Task::Active
   // Documentation inherited
   Event::Status status_overview() const final;
 
+  // Documentation inherited
+  bool finished() const final;
+
   // Documentation inherited
   const std::vector<Phase::ConstCompletedPtr>& completed_phases() const final;
 
@@ -231,6 +234,8 @@ class Task::Active
     Phase::Tag::Id current_phase_id,
     Phase::Active::Backup phase_backup) const;
 
+  Backup _empty_backup() const;
+
   Active(
     Phase::ConstActivatorPtr phase_activator,
     std::function<rmf_traffic::Time()> clock,
@@ -286,6 +291,7 @@ class Task::Active
   std::optional<Resume> _resume_interrupted_phase;
   std::optional<Phase::Tag::Id> _cancelled_on_phase = std::nullopt;
   bool _killed = false;
+  bool _finished = false;
 
   mutable std::optional<uint64_t> _last_phase_backup_sequence_number;
   mutable uint64_t _next_task_backup_sequence_number = 0;
@@ -439,6 +445,12 @@ rmf_task::Event::Status Task::Active::status_overview() const
   return Event::Status::Underway;
 }
 
+//==============================================================================
+bool Task::Active::finished() const
+{
+  return _finished;
+}
+
 //==============================================================================
 const std::vector<Phase::ConstCompletedPtr>&
 Task::Active::completed_phases() const
@@ -473,6 +485,9 @@ const Task::ConstTagPtr& Task::Active::tag() const
 //==============================================================================
 rmf_traffic::Duration Task::Active::estimate_remaining_time() const
 {
+  if (_finished)
+    return rmf_traffic::Duration(0);
+
   auto remaining_time =
     _active_phase ? _active_phase->estimate_remaining_time() :
     rmf_traffic::Duration(0);
@@ -485,6 +500,9 @@ rmf_traffic::Duration Task::Active::estimate_remaining_time() const
 //==============================================================================
 auto Task::Active::backup() const -> Backup
 {
+  if (!_active_phase || _finished)
+    return _empty_backup();
+
   return _generate_backup(
     _active_phase->tag()->id(),
     _active_phase->backup());
@@ -522,6 +540,13 @@ void Task::Active::cancel()
     return;
   }
 
+  if (_finished)
+  {
+    // If the task is already finished, then there is no need to do anything
+    // for the cancellation.
+    return;
+  }
+
   _cancelled_on_phase = _active_phase->tag()->id();
   _prepare_cancellation_sequence(_active_stage->cancellation_sequence);
   _active_phase->cancel();
@@ -618,6 +643,21 @@ void Task::Active::_load_backup(std::string backup_state_str)
     return failed_to_restore();
   }
 
+  const auto finished_it = backup_state.find("finished");
+  if (finished_it != backup_state.end())
+  {
+    auto finished = finished_it.value().get<bool>();
+    if (finished)
+    {
+      _task_finished();
+      return;
+    }
+
+    _generate_pending_phases();
+    _begin_next_stage();
+    return;
+  }
+
   const auto& current_phase_json = backup_state["current_phase"];
   const auto& cancelled_from_json = current_phase_json["cancelled_from"];
   if (cancelled_from_json)
@@ -823,6 +863,7 @@ void Task::Active::_begin_next_stage(std::optional<nlohmann::json> restore)
 //==============================================================================
 void Task::Active::_finish_task()
 {
+  _finished = true;
   _task_finished();
 }
 
@@ -901,6 +942,20 @@ auto Task::Active::_generate_backup(
   return Backup::make(_next_task_backup_sequence_number++, root.dump());
 }
 
+//==============================================================================
+auto Task::Active::_empty_backup() const -> Backup
+{
+  // Either the task is finished or the first phase has not started. Either way
+  // there is no phase information to provide for the backup. This special case
+  // is handled by providing a "finished" property where true means the task
+  // finished while false means the task has not yet started.
+  nlohmann::json root;
+  root["schema_version"] = 1;
+  root["finished"] = _finished;
+
+  return Backup::make(_next_task_backup_sequence_number++, root.dump());
+}
+
 //==============================================================================
 auto Task::make_activator(
   Phase::ConstActivatorPtr phase_activator,