Skip to content

Commit 9cb227d

Browse files
minggangwminggangw
committed
Address comments
1 parent b6c46d5 commit 9cb227d

3 files changed

Lines changed: 139 additions & 29 deletions

File tree

‎lib/clock.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,31 @@ class Clock {
5656
* @param {bigint} minBackward - Minimum backward jump to trigger the callback.
5757
*/
5858
addClockCallback(callbackObject, onClockChange, minForward, minBackward) {
59+
if (typeof callbackObject !== 'object' || callbackObject === null) {
60+
throw new TypeValidationError(
61+
'callbackObject',
62+
callbackObject,
63+
'object',
64+
{
65+
entityType: 'clock',
66+
}
67+
);
68+
}
69+
if (typeof onClockChange !== 'boolean') {
70+
throw new TypeValidationError('onClockChange', onClockChange, 'boolean', {
71+
entityType: 'clock',
72+
});
73+
}
74+
if (typeof minForward !== 'bigint') {
75+
throw new TypeValidationError('minForward', minForward, 'bigint', {
76+
entityType: 'clock',
77+
});
78+
}
79+
if (typeof minBackward !== 'bigint') {
80+
throw new TypeValidationError('minBackward', minBackward, 'bigint', {
81+
entityType: 'clock',
82+
});
83+
}
5984
rclnodejs.clockAddJumpCallback(
6085
this._handle,
6186
callbackObject,
@@ -70,6 +95,16 @@ class Clock {
7095
* @param {object} callbackObject - The object containing _pre_callback and _post_callback methods.
7196
*/
7297
removeClockCallback(callbackObject) {
98+
if (typeof callbackObject !== 'object' || callbackObject === null) {
99+
throw new TypeValidationError(
100+
'callbackObject',
101+
callbackObject,
102+
'object',
103+
{
104+
entityType: 'clock',
105+
}
106+
);
107+
}
73108
rclnodejs.clockRemoveJumpCallback(this._handle, callbackObject);
74109
}
75110

‎src/rcl_time_point_bindings.cpp‎

Lines changed: 59 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -176,32 +176,40 @@ Napi::Value ClockGetNow(const Napi::CallbackInfo& info) {
176176
}
177177

178178
struct JumpCallbackData {
179-
Napi::ObjectReference ref;
179+
Napi::ThreadSafeFunction tsfn_pre;
180+
Napi::ThreadSafeFunction tsfn_post;
181+
};
182+
183+
struct JumpCallbackContext {
184+
rcl_time_jump_t time_jump;
185+
bool before_jump;
180186
};
181187

182188
void _rclnodejs_on_time_jump(const rcl_time_jump_t* time_jump, bool before_jump,
183189
void* user_data) {
184190
JumpCallbackData* data = static_cast<JumpCallbackData*>(user_data);
185-
Napi::Env env = data->ref.Env();
186-
Napi::HandleScope scope(env);
187191

188-
Napi::Object callback_obj = data->ref.Value();
192+
auto context = new JumpCallbackContext{*time_jump, before_jump};
189193

190194
if (before_jump) {
191-
Napi::Value pre_callback = callback_obj.Get("_pre_callback");
192-
if (pre_callback.IsFunction()) {
193-
pre_callback.As<Napi::Function>().Call(callback_obj, {});
194-
}
195+
auto callback = [](Napi::Env env, Napi::Function js_callback,
196+
JumpCallbackContext* context) {
197+
js_callback.Call({});
198+
delete context;
199+
};
200+
data->tsfn_pre.NonBlockingCall(context, callback);
195201
} else {
196-
Napi::Value post_callback = callback_obj.Get("_post_callback");
197-
if (post_callback.IsFunction()) {
202+
auto callback = [](Napi::Env env, Napi::Function js_callback,
203+
JumpCallbackContext* context) {
198204
Napi::Object jump_info = Napi::Object::New(env);
199205
jump_info.Set("clock_change",
200-
static_cast<int32_t>(time_jump->clock_change));
201-
jump_info.Set("delta",
202-
Napi::BigInt::New(env, time_jump->delta.nanoseconds));
203-
post_callback.As<Napi::Function>().Call(callback_obj, {jump_info});
204-
}
206+
static_cast<int32_t>(context->time_jump.clock_change));
207+
jump_info.Set("delta", Napi::BigInt::New(
208+
env, context->time_jump.delta.nanoseconds));
209+
js_callback.Call({jump_info});
210+
delete context;
211+
};
212+
data->tsfn_post.NonBlockingCall(context, callback);
205213
}
206214
}
207215

@@ -212,31 +220,56 @@ Napi::Value ClockAddJumpCallback(const Napi::CallbackInfo& info) {
212220
rcl_clock_t* clock = reinterpret_cast<rcl_clock_t*>(clock_handle->ptr());
213221

214222
Napi::Object callback_obj = info[1].As<Napi::Object>();
223+
Napi::Function pre_callback =
224+
callback_obj.Get("_pre_callback").As<Napi::Function>();
225+
Napi::Function post_callback =
226+
callback_obj.Get("_post_callback").As<Napi::Function>();
227+
215228
bool on_clock_change = info[2].As<Napi::Boolean>();
216229

217230
bool lossless;
218231
int64_t min_forward = info[3].As<Napi::BigInt>().Int64Value(&lossless);
232+
if (!lossless) {
233+
Napi::TypeError::New(
234+
env, "min_forward BigInt value cannot be represented as int64_t")
235+
.ThrowAsJavaScriptException();
236+
return env.Undefined();
237+
}
219238
int64_t min_backward = info[4].As<Napi::BigInt>().Int64Value(&lossless);
239+
if (!lossless) {
240+
Napi::TypeError::New(
241+
env, "min_backward BigInt value cannot be represented as int64_t")
242+
.ThrowAsJavaScriptException();
243+
return env.Undefined();
244+
}
220245

221246
rcl_jump_threshold_t threshold;
222247
threshold.on_clock_change = on_clock_change;
223248
threshold.min_forward.nanoseconds = min_forward;
224249
threshold.min_backward.nanoseconds = min_backward;
225250

226-
JumpCallbackData* data = new JumpCallbackData{Napi::Persistent(callback_obj)};
227-
callback_obj.Set("_cpp_handle",
228-
Napi::External<JumpCallbackData>::New(env, data));
251+
JumpCallbackData* data = new JumpCallbackData();
252+
data->tsfn_pre = Napi::ThreadSafeFunction::New(
253+
env, pre_callback, "ClockJumpPreCallback", 10, 1, [](Napi::Env) {});
254+
data->tsfn_post =
255+
Napi::ThreadSafeFunction::New(env, post_callback, "ClockJumpPostCallback",
256+
10, 1, [data](Napi::Env) { delete data; });
257+
258+
Napi::Object handle_obj = Napi::Object::New(env);
259+
handle_obj.Set("_cpp_handle",
260+
Napi::External<JumpCallbackData>::New(env, data));
229261

230262
rcl_ret_t ret = rcl_clock_add_jump_callback(clock, threshold,
231263
_rclnodejs_on_time_jump, data);
232264

233265
if (ret != RCL_RET_OK) {
234-
data->ref.Reset();
235-
delete data;
236-
callback_obj.Set("_cpp_handle", env.Undefined());
266+
data->tsfn_pre.Release();
267+
data->tsfn_post.Release();
237268
THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str);
238269
}
239270

271+
callback_obj.Set("_cpp_handle", handle_obj.Get("_cpp_handle"));
272+
240273
return env.Undefined();
241274
}
242275

@@ -246,8 +279,8 @@ Napi::Value ClockRemoveJumpCallback(const Napi::CallbackInfo& info) {
246279
RclHandle* clock_handle = RclHandle::Unwrap(info[0].As<Napi::Object>());
247280
rcl_clock_t* clock = reinterpret_cast<rcl_clock_t*>(clock_handle->ptr());
248281

249-
Napi::Object callback_obj = info[1].As<Napi::Object>();
250-
Napi::Value cpp_handle = callback_obj.Get("_cpp_handle");
282+
Napi::Object handle_obj = info[1].As<Napi::Object>();
283+
Napi::Value cpp_handle = handle_obj.Get("_cpp_handle");
251284

252285
if (cpp_handle.IsUndefined() || !cpp_handle.IsExternal()) {
253286
Napi::Error::New(env,
@@ -263,9 +296,9 @@ Napi::Value ClockRemoveJumpCallback(const Napi::CallbackInfo& info) {
263296
rcl_clock_remove_jump_callback(clock, _rclnodejs_on_time_jump, data);
264297

265298
if (ret == RCL_RET_OK) {
266-
data->ref.Reset();
267-
delete data;
268-
callback_obj.Set("_cpp_handle", env.Undefined());
299+
data->tsfn_pre.Release();
300+
data->tsfn_post.Release();
301+
handle_obj.Set("_cpp_handle", env.Undefined());
269302
} else {
270303
THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str);
271304
}

‎test/test-clock-callback.js‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,27 @@ describe('rclnodejs Clock Callback testing', function () {
1717
rclnodejs.shutdown();
1818
});
1919

20-
it('Test add and remove clock callback', function () {
20+
let testClock = null;
21+
let testCallbacks = [];
22+
23+
afterEach(function () {
24+
// Clean up all registered callbacks
25+
if (testClock && testCallbacks.length > 0) {
26+
testCallbacks.forEach((callback) => {
27+
try {
28+
testClock.removeClockCallback(callback);
29+
} catch (e) {
30+
// Callback may already be removed
31+
}
32+
});
33+
}
34+
testClock = null;
35+
testCallbacks = [];
36+
});
37+
38+
it('Test add and remove clock callback', async function () {
2139
const clock = new Clock(ClockType.ROS_TIME);
40+
testClock = clock;
2241

2342
let preCallbackCalled = false;
2443
let postCallbackCalled = false;
@@ -36,6 +55,7 @@ describe('rclnodejs Clock Callback testing', function () {
3655

3756
// Add callback
3857
clock.addClockCallback(callbackObj, true, 0n, 0n);
58+
testCallbacks.push(callbackObj);
3959

4060
// Enable ROS time override
4161
rclnodejsNative.setRosTimeOverrideIsEnabled(clock.handle, true);
@@ -46,6 +66,9 @@ describe('rclnodejs Clock Callback testing', function () {
4666
// Set override (should trigger jump)
4767
rclnodejsNative.setRosTimeOverride(clock.handle, timePoint._handle);
4868

69+
// Wait for callbacks to be processed
70+
await new Promise((resolve) => setImmediate(resolve));
71+
4972
assert.strictEqual(
5073
preCallbackCalled,
5174
true,
@@ -72,6 +95,9 @@ describe('rclnodejs Clock Callback testing', function () {
7295
const timePoint2 = new Time(200n, 0n, ClockType.ROS_TIME);
7396
rclnodejsNative.setRosTimeOverride(clock.handle, timePoint2._handle);
7497

98+
// Wait for any potential callbacks
99+
await new Promise((resolve) => setImmediate(resolve));
100+
75101
assert.strictEqual(
76102
preCallbackCalled,
77103
false,
@@ -84,8 +110,9 @@ describe('rclnodejs Clock Callback testing', function () {
84110
);
85111
});
86112

87-
it('Test clock callback thresholds', function () {
113+
it('Test clock callback thresholds', async function () {
88114
const clock = new Clock(ClockType.ROS_TIME);
115+
testClock = clock;
89116
let callbackCalled = false;
90117

91118
const callbackObj = {
@@ -99,12 +126,17 @@ describe('rclnodejs Clock Callback testing', function () {
99126
// Add callback with threshold of 1 second (10^9 nanoseconds)
100127
// Set onClockChange to false to ensure we are testing time jump threshold
101128
clock.addClockCallback(callbackObj, false, 1000000000n, 0n);
129+
testCallbacks.push(callbackObj);
102130

103131
rclnodejsNative.setRosTimeOverrideIsEnabled(clock.handle, true);
104132

105133
// Jump forward by 0.5 seconds (should NOT trigger)
106134
let timePoint = new Time(0n, 500000000n, ClockType.ROS_TIME);
107135
rclnodejsNative.setRosTimeOverride(clock.handle, timePoint._handle);
136+
137+
// Wait for callbacks to be processed
138+
await new Promise((resolve) => setImmediate(resolve));
139+
108140
assert.strictEqual(
109141
callbackCalled,
110142
false,
@@ -115,15 +147,20 @@ describe('rclnodejs Clock Callback testing', function () {
115147
// Current time is 0.5s. New time is 2.0s. Delta is 1.5s > 1.0s.
116148
timePoint = new Time(2n, 0n, ClockType.ROS_TIME);
117149
rclnodejsNative.setRosTimeOverride(clock.handle, timePoint._handle);
150+
151+
// Wait for callbacks to be processed
152+
await new Promise((resolve) => setImmediate(resolve));
153+
118154
assert.strictEqual(
119155
callbackCalled,
120156
true,
121157
'Callback should be called for large jump'
122158
);
123159
});
124160

125-
it('Test multiple clock callbacks', function () {
161+
it('Test multiple clock callbacks', async function () {
126162
const clock = new Clock(ClockType.ROS_TIME);
163+
testClock = clock;
127164
let callback1Called = false;
128165
let callback2Called = false;
129166

@@ -142,12 +179,17 @@ describe('rclnodejs Clock Callback testing', function () {
142179
};
143180

144181
clock.addClockCallback(callbackObj1, true, 0n, 0n);
182+
testCallbacks.push(callbackObj1);
145183
clock.addClockCallback(callbackObj2, true, 0n, 0n);
184+
testCallbacks.push(callbackObj2);
146185

147186
rclnodejsNative.setRosTimeOverrideIsEnabled(clock.handle, true);
148187
const timePoint = new Time(1n, 0n, ClockType.ROS_TIME);
149188
rclnodejsNative.setRosTimeOverride(clock.handle, timePoint._handle);
150189

190+
// Wait for callbacks to be processed
191+
await new Promise((resolve) => setImmediate(resolve));
192+
151193
assert.strictEqual(callback1Called, true);
152194
assert.strictEqual(callback2Called, true);
153195
});

0 commit comments

Comments
 (0)