Skip to content

Commit 5f84174

Browse files
wayneparrottwayneparrott
authored
Fixes action ClientGoalHandler getResult() and status update bug, #813 (#814)
Public API Changes ClientGoalHandler (client_goal_handler.js, action_client.d.ts) deprecated accepting getter added convenience methods: isAccepting(), isExecuting(), isCanceling(), isSucceeded(), isCanceled(), isAborted(). These methods are not essential. Thus will leave it to the reviewer's discretion of whether they stay or go. The same for Initiate the project #1 above. status property is now properly updated with the goal lifecycle state codes, see ActionInterfaces.GoalStatus for codes getResult() now returns a ActionResult. Previously it returned an internal message with structure {status: number, result: ActionResult<T>}. ClientGoalHandle feedback function now only receives a ActionFeedback<T> argument. Previously ActionFallback<T> function was receiving an internal msg with structure {goal_id: UUID, feedback: ActionFeedback}`. Description A bug was submitted where ClientGoalHandler.getResult() was returning an internal message that did not conform to the TypeScript declarations. Additionally the ClientGoalHandler.status was never being updated and always return UNKNOWN. Lastly while fixing the getResult() bug it was discovered that the feedbackCallback function was being sent an internal message and not the expected ActionFeedback<T> type. This fix resolves all 3 issues. I added beforeSetResult(result) method to Deferred.js class. This is used by the ActionClient _getResult(resultMsg) method to update the ClientGoalHandler.status and to return only the msg.result property of type ActionResult<t>. Updated all action related tests and examples. Fixed several prettier/lint issues. Fix #813
1 parent 16acb55 commit 5f84174

File tree

8 files changed

+171
-33
lines changed

8 files changed

+171
-33
lines changed

‎example/action-client-example.js

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,31 +42,28 @@ class FibonacciActionClient {
4242
this.feedbackCallback(feedback)
4343
);
4444

45-
if (!goalHandle.accepted) {
45+
if (!goalHandle.isAccepted()) {
4646
this._node.getLogger().info('Goal rejected');
4747
return;
4848
}
4949

5050
this._node.getLogger().info('Goal accepted');
5151

5252
const result = await goalHandle.getResult();
53-
const status = result.status;
5453

55-
if (status === GoalStatus.STATUS_SUCCEEDED) {
54+
if (goalHandle.isSucceeded()) {
5655
this._node
5756
.getLogger()
58-
.info(`Goal suceeded with result: ${result.result.sequence}`);
57+
.info(`Goal suceeded with result: ${result.sequence}`);
5958
} else {
6059
this._node.getLogger().info(`Goal failed with status: ${status}`);
6160
}
6261

6362
rclnodejs.shutdown();
6463
}
6564

66-
feedbackCallback(feedbackMessage) {
67-
this._node
68-
.getLogger()
69-
.info(`Received feedback: ${feedbackMessage.feedback.sequence}`);
65+
feedbackCallback(feedback) {
66+
this._node.getLogger().info(`Received feedback: ${feedback.sequence}`);
7067
}
7168
}
7269

‎lib/action/client.js

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ class ActionClient extends Entity {
153153
let uuid = ActionUuid.fromBytes(message.goal_id.uuid).toString();
154154
if (this._feedbackCallbacks.has(uuid)) {
155155
this._feedbackCallbacks.get(uuid)(
156-
message.toPlainObject(this.typedArrayEnabled)
156+
message.toPlainObject(this.typedArrayEnabled).feedback
157157
);
158158
}
159159
}
@@ -166,10 +166,10 @@ class ActionClient extends Entity {
166166
).toString();
167167
let status = statusMessage.status;
168168

169-
if (!this._goalHandles.has(uuid)) {
169+
if (this._goalHandles.has(uuid)) {
170170
let goalHandle = this._goalHandles.get(uuid);
171171
if (goalHandle) {
172-
goalHandle._status = status;
172+
goalHandle.status = status;
173173

174174
// Remove done handles from the list
175175
// eslint-disable-next-line max-depth
@@ -332,12 +332,16 @@ class ActionClient extends Entity {
332332
}
333333

334334
let deferred = new Deferred();
335-
this._pendingResultRequests.set(sequenceNumber, deferred);
336-
335+
deferred.beforeSetResultCallback((result) => {
336+
goalHandle.status = result.status;
337+
return result.result;
338+
});
337339
deferred.setDoneCallback(() =>
338340
this._removePendingResultRequest(sequenceNumber)
339341
);
340342

343+
this._pendingResultRequests.set(sequenceNumber, deferred);
344+
341345
return deferred.promise;
342346
}
343347

‎lib/action/client_goal_handle.js

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ class ClientGoalHandle {
2626
this._actionClient = actionClient;
2727
this._goalId = goalId;
2828
this._goalResponse = goalResponse;
29-
this._status = ActionInterfaces.GoalStatus.STATUS_UNKNOWN;
29+
this._status = this.accepted
30+
? ActionInterfaces.GoalStatus.STATUS_ACCEPTED
31+
: ActionInterfaces.GoalStatus.STATUS_UNKNOWN;
3032
}
3133

3234
/**
@@ -45,18 +47,82 @@ class ClientGoalHandle {
4547

4648
/**
4749
* Gets if the goal response was accepted.
50+
* @deprecated use isAccepted()
4851
*/
4952
get accepted() {
53+
return this.isAccepted();
54+
}
55+
56+
/**
57+
* Determine if goal is currently executing
58+
* @returns {bool} - True if goal is executing; otherwise return false.
59+
*/
60+
isAccepted() {
5061
return this._goalResponse.accepted;
5162
}
5263

64+
/**
65+
* Determine if goal is currently executing
66+
* @returns {bool} - True if goal is executing; otherwise return false.
67+
*/
68+
isExecuting() {
69+
return this.status === ActionInterfaces.GoalStatus.STATUS_EXECUTING;
70+
}
71+
72+
/**
73+
* Determine if goal is in the process of canceling.
74+
* @returns {bool} - True if goal is canceling; otherwise return false.
75+
*/
76+
isCanceling() {
77+
return this.status === ActionInterfaces.GoalStatus.STATUS_CANCELING;
78+
}
79+
80+
/**
81+
* Determine if goal completed successfullly.
82+
* @returns {bool} - True if goal completed successfully; otherwise return false.
83+
*/
84+
isSucceeded() {
85+
return this.status === ActionInterfaces.GoalStatus.STATUS_SUCCEEDED;
86+
}
87+
88+
/**
89+
* Determine if goal has been canceled.
90+
* @returns {bool} - True if goal has been aborted; otherwise return false.
91+
*/
92+
isCanceled() {
93+
return this.status === ActionInterfaces.GoalStatus.STATUS_CANCELED;
94+
}
95+
96+
/**
97+
* Determine if goal has been aborted.
98+
* @returns {bool} - True if goal was aborted; otherwise return false.
99+
*/
100+
isAborted() {
101+
return this.status === ActionInterfaces.GoalStatus.STATUS_ABORTED;
102+
}
103+
53104
/**
54105
* Gets the goal status.
55106
*/
56107
get status() {
57108
return this._status;
58109
}
59110

111+
/**
112+
* Update status to the latest state of goal computation.
113+
* When status is in a final state it can not be revered to an
114+
* earlier state, e.g., can not change from SUCCEEDED to ACCEPTED.
115+
* @param {number} newStatus - The new status of this goal.
116+
*/
117+
set status(newStatus) {
118+
if (
119+
this._status < ActionInterfaces.GoalStatus.STATUS_SUCCEEDED &&
120+
newStatus > this._status
121+
) {
122+
this._status = newStatus;
123+
}
124+
}
125+
60126
/**
61127
* Send a cancel request for the goal.
62128
* @returns {Promise} - The cancel response.

‎lib/action/deferred.js

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,15 +35,30 @@ class Deferred {
3535
return this._promise;
3636
}
3737

38+
/**
39+
* Sets a function to be called before result updated
40+
* @param {function} callback - Function to be called.
41+
* @returns {undefined}
42+
*/
43+
beforeSetResultCallback(callback) {
44+
if (typeof callback !== 'function') {
45+
throw new TypeError('Invalid parameter');
46+
}
47+
48+
this._beforeSetResultCallback = callback;
49+
}
50+
3851
/**
3952
* Resolves the deferred promise.
4053
* @param {*} result - The value to resolve the promise with.
4154
* @returns {undefined}
4255
*/
4356
setResult(result) {
4457
if (this._resolve) {
45-
this._result = result;
46-
this._resolve(result);
58+
this._result = this._beforeSetResultCallback
59+
? this._beforeSetResultCallback(result)
60+
: result;
61+
this._resolve(this._result);
4762
this._resolve = null;
4863
}
4964
}

‎test/test-action-client.js

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ describe('rclnodejs action client', function () {
165165
assert.ok(result);
166166

167167
let goalHandle = await client.sendGoal(new Fibonacci.Goal());
168-
assert.ok(goalHandle.accepted);
168+
assert.ok(goalHandle.isAccepted());
169169

170170
result = await goalHandle.getResult();
171171
assert.ok(result);
@@ -192,9 +192,10 @@ describe('rclnodejs action client', function () {
192192
feedbackCallback,
193193
goalUuid
194194
);
195-
assert.ok(goalHandle.accepted);
195+
assert.ok(goalHandle.isAccepted());
196196

197197
await goalHandle.getResult();
198+
assert.ok(goalHandle.isSucceeded());
198199
assert.ok(result);
199200

200201
assert.ok(feedbackCallback.calledOnce);
@@ -285,7 +286,7 @@ describe('rclnodejs action client', function () {
285286
assert.ok(result);
286287

287288
let goalHandle = await client.sendGoal(new Fibonacci.Goal());
288-
assert.ok(goalHandle.accepted);
289+
assert.ok(goalHandle.isAccepted());
289290

290291
result = await goalHandle.cancelGoal();
291292
assert.ok(result);

‎test/test-action-server.js

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ describe('rclnodejs action server', function () {
411411
serverGoalHandle.execute();
412412

413413
result = await handle.getResult();
414-
assert.strictEqual(result.status, GoalStatus.STATUS_CANCELED);
414+
assert.strictEqual(handle.status, GoalStatus.STATUS_CANCELED);
415415
assert.strictEqual(serverGoalHandle.status, GoalStatus.STATUS_CANCELED);
416416

417417
server.destroy();
@@ -445,8 +445,8 @@ describe('rclnodejs action server', function () {
445445

446446
let result = await handle.getResult();
447447
assert.ok(result);
448-
assert.ok(result.status, GoalStatus.STATUS_SUCCEEDED);
449-
assert.ok(deepEqual(result.result.sequence, testSequence));
448+
assert.ok(handle.status, GoalStatus.STATUS_SUCCEEDED);
449+
assert.ok(deepEqual(result.sequence, testSequence));
450450

451451
server.destroy();
452452
});
@@ -479,8 +479,8 @@ describe('rclnodejs action server', function () {
479479

480480
let result = await handle.getResult();
481481
assert.ok(result);
482-
assert.ok(result.status, GoalStatus.STATUS_ABORTED);
483-
assert.ok(deepEqual(result.result.sequence, testSequence));
482+
assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
483+
assert.ok(deepEqual(result.sequence, testSequence));
484484

485485
server.destroy();
486486
});
@@ -514,8 +514,8 @@ describe('rclnodejs action server', function () {
514514
let result = await handle.getResult();
515515
assert.ok(result);
516516
// Goal status should default to aborted
517-
assert.ok(result.status, GoalStatus.STATUS_ABORTED);
518-
assert.ok(deepEqual(result.result.sequence, testSequence));
517+
assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
518+
assert.ok(deepEqual(result.sequence, testSequence));
519519

520520
server.destroy();
521521
});
@@ -542,8 +542,8 @@ describe('rclnodejs action server', function () {
542542
let result = await handle.getResult();
543543
assert.ok(result);
544544
// Goal status should default to aborted
545-
assert.ok(result.status, GoalStatus.STATUS_ABORTED);
546-
assert.ok(deepEqual(result.result.sequence, []));
545+
assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
546+
assert.ok(deepEqual(result.sequence, []));
547547

548548
server.destroy();
549549
});
@@ -660,7 +660,7 @@ describe('rclnodejs action server', function () {
660660
await assertUtils.createDelay(50);
661661

662662
assert.ok(feedbackMessage);
663-
assert.ok(deepEqual(sequence, feedbackMessage.feedback.sequence));
663+
assert.ok(deepEqual(sequence, feedbackMessage.sequence));
664664

665665
server.destroy();
666666
});
@@ -704,7 +704,7 @@ describe('rclnodejs action server', function () {
704704
await assertUtils.createDelay(50);
705705

706706
assert.ok(feedbackMessage);
707-
assert.ok(deepEqual(sequence, feedbackMessage.feedback.sequence));
707+
assert.ok(deepEqual(sequence, feedbackMessage.sequence));
708708

709709
server.destroy();
710710
});

‎test/types/main.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -462,22 +462,40 @@ const goalHandlePromise = actionClient.sendGoal(new Fibonacci.Goal());
462462

463463
goalHandlePromise.then((goalHandle) => {
464464
// $ExpectType boolean
465-
goalHandle.accepted;
465+
goalHandle.accepted; // deprecated
466466

467467
// $ExpectType UUID
468468
goalHandle.goalId;
469469

470470
// $ExpectType Time
471471
goalHandle.stamp;
472472

473-
// $ExpectType string
473+
// $ExpectType number
474474
goalHandle.status;
475475

476476
// $ExpectType Promise<CancelGoal_Response>
477477
goalHandle.cancelGoal();
478478

479479
// $ExpectType Promise<Fibonacci_Result>
480480
goalHandle.getResult();
481+
482+
// $ExpectType boolean
483+
goalHandle.isAccepted();
484+
485+
// $ExpectType boolean
486+
goalHandle.isExecuting();
487+
488+
// $ExpectType boolean
489+
goalHandle.isSucceeded();
490+
491+
// $ExpectType boolean
492+
goalHandle.isCanceling();
493+
494+
// $ExpectType boolean
495+
goalHandle.isCanceled();
496+
497+
// $ExpectType boolean
498+
goalHandle.isAborted();
481499
});
482500

483501
// ---- ActionServer -----

0 commit comments

Comments
 (0)