Fixes action ClientGoalHandler getResult() and status update bug, #813

example/action-client-example.js

@@ -42,31 +42,28 @@ class FibonacciActionClient {
       this.feedbackCallback(feedback)
     );
 
-    if (!goalHandle.accepted) {
+    if (!goalHandle.isAccepted()) {
       this._node.getLogger().info('Goal rejected');
       return;
     }
 
     this._node.getLogger().info('Goal accepted');
 
     const result = await goalHandle.getResult();
-    const status = result.status;
 
-    if (status === GoalStatus.STATUS_SUCCEEDED) {
+    if (goalHandle.isSucceeded()) {
       this._node
         .getLogger()
-        .info(`Goal suceeded with result: ${result.result.sequence}`);
+        .info(`Goal suceeded with result: ${result.sequence}`);
     } else {
       this._node.getLogger().info(`Goal failed with status: ${status}`);
     }
 
     rclnodejs.shutdown();
   }
 
-  feedbackCallback(feedbackMessage) {
-    this._node
-      .getLogger()
-      .info(`Received feedback: ${feedbackMessage.feedback.sequence}`);
+  feedbackCallback(feedback) {
+    this._node.getLogger().info(`Received feedback: ${feedback.sequence}`);
   }
 }
 

lib/action/client.js

@@ -153,7 +153,7 @@ class ActionClient extends Entity {
     let uuid = ActionUuid.fromBytes(message.goal_id.uuid).toString();
     if (this._feedbackCallbacks.has(uuid)) {
       this._feedbackCallbacks.get(uuid)(
-        message.toPlainObject(this.typedArrayEnabled)
+        message.toPlainObject(this.typedArrayEnabled).feedback
       );
     }
   }
@@ -166,10 +166,10 @@ class ActionClient extends Entity {
       ).toString();
       let status = statusMessage.status;
 
-      if (!this._goalHandles.has(uuid)) {
+      if (this._goalHandles.has(uuid)) {
         let goalHandle = this._goalHandles.get(uuid);
         if (goalHandle) {
-          goalHandle._status = status;
+          goalHandle.status = status;
 
           // Remove done handles from the list
           // eslint-disable-next-line max-depth
@@ -332,12 +332,16 @@ class ActionClient extends Entity {
     }
 
     let deferred = new Deferred();
-    this._pendingResultRequests.set(sequenceNumber, deferred);
-
+    deferred.beforeSetResultCallback((result) => {
+      goalHandle.status = result.status;
+      return result.result;
+    });
     deferred.setDoneCallback(() =>
       this._removePendingResultRequest(sequenceNumber)
     );
 
+    this._pendingResultRequests.set(sequenceNumber, deferred);
+
     return deferred.promise;
   }
 

lib/action/client_goal_handle.js

@@ -26,7 +26,9 @@ class ClientGoalHandle {
     this._actionClient = actionClient;
     this._goalId = goalId;
     this._goalResponse = goalResponse;
-    this._status = ActionInterfaces.GoalStatus.STATUS_UNKNOWN;
+    this._status = this.accepted
+      ? ActionInterfaces.GoalStatus.STATUS_ACCEPTED
+      : ActionInterfaces.GoalStatus.STATUS_UNKNOWN;
   }
 
   /**
@@ -45,18 +47,82 @@ class ClientGoalHandle {
 
   /**
    * Gets if the goal response was accepted.
+   * @deprecated use isAccepted()
    */
   get accepted() {
+    return this.isAccepted();
+  }
+
+  /**
+   * Determine if goal is currently executing
+   * @returns {bool} - True if goal is executing; otherwise return false.
+   */
+  isAccepted() {
     return this._goalResponse.accepted;
   }
 
+  /**
+   * Determine if goal is currently executing
+   * @returns {bool} - True if goal is executing; otherwise return false.
+   */
+  isExecuting() {
+    return this.status === ActionInterfaces.GoalStatus.STATUS_EXECUTING;
+  }
+
+  /**
+   * Determine if goal is in the process of canceling.
+   * @returns {bool} - True if goal is canceling; otherwise return false.
+   */
+  isCanceling() {
+    return this.status === ActionInterfaces.GoalStatus.STATUS_CANCELING;
+  }
+
+  /**
+   * Determine if goal completed successfullly.
+   * @returns {bool} - True if goal completed successfully; otherwise return false.
+   */
+  isSucceeded() {
+    return this.status === ActionInterfaces.GoalStatus.STATUS_SUCCEEDED;
+  }
+
+  /**
+   * Determine if goal has been canceled.
+   * @returns {bool} - True if goal has been aborted; otherwise return false.
+   */
+  isCanceled() {
+    return this.status === ActionInterfaces.GoalStatus.STATUS_CANCELED;
+  }
+
+  /**
+   * Determine if goal has been aborted.
+   * @returns {bool} - True if goal was aborted; otherwise return false.
+   */
+  isAborted() {
+    return this.status === ActionInterfaces.GoalStatus.STATUS_ABORTED;
+  }
+
   /**
    * Gets the goal status.
    */
   get status() {
     return this._status;
   }
 
+  /**
+   * Update status to the latest state of goal computation.
+   * When status is in a final state it can not be revered to an
+   * earlier state, e.g., can not change from SUCCEEDED to ACCEPTED.
+   * @param {number} newStatus - The new status of this goal.
+   */
+  set status(newStatus) {
+    if (
+      this._status < ActionInterfaces.GoalStatus.STATUS_SUCCEEDED &&
+      newStatus > this._status
+    ) {
+      this._status = newStatus;
+    }
+  }
+
   /**
    * Send a cancel request for the goal.
    * @returns {Promise} - The cancel response.

lib/action/deferred.js

@@ -35,15 +35,30 @@ class Deferred {
     return this._promise;
   }
 
+  /**
+   * Sets a function to be called before result updated
+   * @param {function} callback - Function to be called.
+   * @returns {undefined}
+   */
+  beforeSetResultCallback(callback) {
+    if (typeof callback !== 'function') {
+      throw new TypeError('Invalid parameter');
+    }
+
+    this._beforeSetResultCallback = callback;
+  }
+
   /**
    * Resolves the deferred promise.
    * @param {*} result - The value to resolve the promise with.
    * @returns {undefined}
    */
   setResult(result) {
     if (this._resolve) {
-      this._result = result;
-      this._resolve(result);
+      this._result = this._beforeSetResultCallback
+        ? this._beforeSetResultCallback(result)
+        : result;
+      this._resolve(this._result);
       this._resolve = null;
     }
   }

test/test-action-client.js

@@ -165,7 +165,7 @@ describe('rclnodejs action client', function () {
     assert.ok(result);
 
     let goalHandle = await client.sendGoal(new Fibonacci.Goal());
-    assert.ok(goalHandle.accepted);
+    assert.ok(goalHandle.isAccepted());
 
     result = await goalHandle.getResult();
     assert.ok(result);
@@ -192,9 +192,10 @@ describe('rclnodejs action client', function () {
       feedbackCallback,
       goalUuid
     );
-    assert.ok(goalHandle.accepted);
+    assert.ok(goalHandle.isAccepted());
 
     await goalHandle.getResult();
+    assert.ok(goalHandle.isSucceeded());
     assert.ok(result);
 
     assert.ok(feedbackCallback.calledOnce);
@@ -285,7 +286,7 @@ describe('rclnodejs action client', function () {
     assert.ok(result);
 
     let goalHandle = await client.sendGoal(new Fibonacci.Goal());
-    assert.ok(goalHandle.accepted);
+    assert.ok(goalHandle.isAccepted());
 
     result = await goalHandle.cancelGoal();
     assert.ok(result);

test/test-action-server.js

@@ -411,7 +411,7 @@ describe('rclnodejs action server', function () {
     serverGoalHandle.execute();
 
     result = await handle.getResult();
-    assert.strictEqual(result.status, GoalStatus.STATUS_CANCELED);
+    assert.strictEqual(handle.status, GoalStatus.STATUS_CANCELED);
     assert.strictEqual(serverGoalHandle.status, GoalStatus.STATUS_CANCELED);
 
     server.destroy();
@@ -445,8 +445,8 @@ describe('rclnodejs action server', function () {
 
     let result = await handle.getResult();
     assert.ok(result);
-    assert.ok(result.status, GoalStatus.STATUS_SUCCEEDED);
-    assert.ok(deepEqual(result.result.sequence, testSequence));
+    assert.ok(handle.status, GoalStatus.STATUS_SUCCEEDED);
+    assert.ok(deepEqual(result.sequence, testSequence));
 
     server.destroy();
   });
@@ -479,8 +479,8 @@ describe('rclnodejs action server', function () {
 
     let result = await handle.getResult();
     assert.ok(result);
-    assert.ok(result.status, GoalStatus.STATUS_ABORTED);
-    assert.ok(deepEqual(result.result.sequence, testSequence));
+    assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
+    assert.ok(deepEqual(result.sequence, testSequence));
 
     server.destroy();
   });
@@ -514,8 +514,8 @@ describe('rclnodejs action server', function () {
     let result = await handle.getResult();
     assert.ok(result);
     // Goal status should default to aborted
-    assert.ok(result.status, GoalStatus.STATUS_ABORTED);
-    assert.ok(deepEqual(result.result.sequence, testSequence));
+    assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
+    assert.ok(deepEqual(result.sequence, testSequence));
 
     server.destroy();
   });
@@ -542,8 +542,8 @@ describe('rclnodejs action server', function () {
     let result = await handle.getResult();
     assert.ok(result);
     // Goal status should default to aborted
-    assert.ok(result.status, GoalStatus.STATUS_ABORTED);
-    assert.ok(deepEqual(result.result.sequence, []));
+    assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
+    assert.ok(deepEqual(result.sequence, []));
 
     server.destroy();
   });
@@ -660,7 +660,7 @@ describe('rclnodejs action server', function () {
     await assertUtils.createDelay(50);
 
     assert.ok(feedbackMessage);
-    assert.ok(deepEqual(sequence, feedbackMessage.feedback.sequence));
+    assert.ok(deepEqual(sequence, feedbackMessage.sequence));
 
     server.destroy();
   });
@@ -704,7 +704,7 @@ describe('rclnodejs action server', function () {
     await assertUtils.createDelay(50);
 
     assert.ok(feedbackMessage);
-    assert.ok(deepEqual(sequence, feedbackMessage.feedback.sequence));
+    assert.ok(deepEqual(sequence, feedbackMessage.sequence));
 
     server.destroy();
   });

test/types/main.ts

@@ -462,22 +462,40 @@ const goalHandlePromise = actionClient.sendGoal(new Fibonacci.Goal());
 
 goalHandlePromise.then((goalHandle) => {
   // $ExpectType boolean
-  goalHandle.accepted;
+  goalHandle.accepted; // deprecated
 
   // $ExpectType UUID
   goalHandle.goalId;
 
   // $ExpectType Time
   goalHandle.stamp;
 
-  // $ExpectType string
+  // $ExpectType number
   goalHandle.status;
 
   // $ExpectType Promise<CancelGoal_Response>
   goalHandle.cancelGoal();
 
   // $ExpectType Promise<Fibonacci_Result>
   goalHandle.getResult();
+
+  // $ExpectType boolean
+  goalHandle.isAccepted();
+
+  // $ExpectType boolean
+  goalHandle.isExecuting();
+
+  // $ExpectType boolean
+  goalHandle.isSucceeded();
+
+  // $ExpectType boolean
+  goalHandle.isCanceling();
+
+  // $ExpectType boolean
+  goalHandle.isCanceled();
+
+  // $ExpectType boolean
+  goalHandle.isAborted();
 });
 
 // ---- ActionServer -----