Add ClockEvent support #1354

minggangw · 2025-12-16T07:51:25Z

This PR adds ClockEvent support to rclnodejs, providing a synchronization mechanism for waiting until specific clock times are reached. This is useful for time-based coordination in ROS applications.

Implements ClockEvent class with wait methods for steady, system, and ROS clocks
Adds TypeScript type definitions for the new ClockEvent API
Includes JavaScript and C++ implementation with async worker support

Fix: #1330

Copilot

Pull request overview

This PR adds ClockEvent support to rclnodejs, providing a synchronization mechanism for waiting until specific clock times are reached. This is useful for time-based coordination in ROS applications.

Implements ClockEvent class with wait methods for steady, system, and ROS clocks
Adds TypeScript type definitions for the new ClockEvent API
Includes JavaScript and C++ implementation with async worker support

Reviewed changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
types/clock_event.d.ts	Adds TypeScript definitions for the ClockEvent class with wait methods and set/clear operations
types/index.d.ts	Adds reference to the new clock_event type definitions
src/clock_event.hpp	Defines the C++ ClockEvent class interface with template-based wait methods
src/clock_event.cpp	Implements ClockEvent functionality with async workers and native bindings
src/addon.cpp	Registers the ClockEvent bindings in the native module
lib/clock_event.js	JavaScript wrapper class that provides the public API for ClockEvent
index.js	Exports the ClockEvent class in the main module
binding.gyp	Adds clock_event.cpp to the build configuration
test/test-clock-event.js	Adds test cases for ClockEvent functionality
test/types/index.test-d.ts	Adds TypeScript type tests for ClockEvent

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/clock_event.hpp

Copilot · 2025-12-16T07:54:43Z

test/test-clock-event.js

+describe('ClockEvent', function () {
+  let node;
+  this.timeout(10000);
+
+  before(async function () {
+    await rclnodejs.init();
+    node = rclnodejs.createNode('test_clock_event_node');
+    rclnodejs.spin(node);
+  });
+
+  after(function () {
+    rclnodejs.shutdown();
+  });
+
+  it('should wait until steady time', async function () {
+    const clock = new Clock(ClockType.STEADY_TIME);
+    const event = new rclnodejs.ClockEvent();
+    const now = clock.now();
+    const until = now.add(new Duration(1n, 0n)); // 1 second later
+
+    const start = Date.now();
+    await event.waitUntilSteady(clock, until.nanoseconds);
+    const end = Date.now();
+
+    assert(end - start >= 1000);
+  });
+
+  it('should wait until system time', async function () {
+    const clock = new Clock(ClockType.SYSTEM_TIME);
+    const event = new rclnodejs.ClockEvent();
+    const now = clock.now();
+    const until = now.add(new Duration(1n, 0n)); // 1 second later
+
+    const start = Date.now();
+    await event.waitUntilSystem(clock, until.nanoseconds);
+    const end = Date.now();
+
+    assert(end - start >= 1000);
+  });


The waitUntilRos method lacks test coverage. While waitUntilSteady and waitUntilSystem are tested, there is no test case for waitUntilRos. Consider adding a test that validates the ROS clock wait behavior.

Copilot · 2025-12-16T07:54:43Z

test/test-clock-event.js

+
+const assert = require('assert');
+const rclnodejs = require('../index.js');
+const { Clock, ClockType, Time, Duration } = rclnodejs;


Unused variable Time.

Suggested change

const { Clock, ClockType, Time, Duration } = rclnodejs;

const { Clock, ClockType, Duration } = rclnodejs;

coveralls · 2025-12-16T08:26:08Z

coverage: 80.404% (-0.05%) from 80.454%
when pulling 8e07620 on minggangw:fix-1330-11
into afeeae4 on RobotWebTools:develop.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 14 out of 18 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-23T06:25:00Z

src/clock_event.cpp

+  bool lossless;
+  int64_t until = info[2].As<Napi::BigInt>().Int64Value(&lossless);


The 'lossless' parameter from Int64Value is not being checked. When a BigInt value cannot be represented as int64_t without loss of precision, this should be detected and an appropriate error should be thrown to prevent silent data corruption. Other binding functions in the codebase (e.g., ClockAddJumpCallback in rcl_time_point_bindings.cpp) check the lossless flag and throw a TypeError when the conversion loses precision.

Copilot · 2025-12-23T06:25:00Z

src/clock_event.cpp

+  bool lossless;
+  int64_t until = info[2].As<Napi::BigInt>().Int64Value(&lossless);


The 'lossless' parameter from Int64Value is not being checked. When a BigInt value cannot be represented as int64_t without loss of precision, this should be detected and an appropriate error should be thrown to prevent silent data corruption. Other binding functions in the codebase (e.g., ClockAddJumpCallback in rcl_time_point_bindings.cpp) check the lossless flag and throw a TypeError when the conversion loses precision.

Copilot · 2025-12-23T06:25:00Z

src/clock_event.cpp

+  bool lossless;
+  int64_t until = info[2].As<Napi::BigInt>().Int64Value(&lossless);


The 'lossless' parameter from Int64Value is not being checked. When a BigInt value cannot be represented as int64_t without loss of precision, this should be detected and an appropriate error should be thrown to prevent silent data corruption. Other binding functions in the codebase (e.g., ClockAddJumpCallback in rcl_time_point_bindings.cpp) check the lossless flag and throw a TypeError when the conversion loses precision.

Copilot · 2025-12-23T06:25:01Z

lib/clock.js

+    if (until.clockType !== this._clockType) {
+      throw new TypeError(
+        "until's clock type does not match this clock's type"
+      );
+    }


Missing input validation for the 'until' parameter. The function accesses 'until.clockType' and 'until.nanoseconds' without first verifying that 'until' is an instance of Time. This could lead to unclear errors if an invalid argument is passed. Consider adding a validation check similar to the one in ROSClock.set rosTimeOverride (line 276) before accessing the properties.

Copilot · 2025-12-23T06:25:01Z

lib/clock.js

+    const until = this.now().add(duration);
+    return this.sleepUntil(until, context);


Missing input validation for the 'duration' parameter. The function calls 'this.now().add(duration)' without first verifying that 'duration' is an instance of Duration. This could lead to unclear errors if an invalid argument is passed. Consider adding a validation check to ensure the parameter is of the correct type before attempting to use it.

Suggested change

const until = this.now().add(duration);

return this.sleepUntil(until, context);

if (

!duration ||

typeof duration !== 'object' ||

!duration.constructor ||

duration.constructor.name !== 'Duration'

) {

throw new TypeValidationError('duration', duration, 'Duration', {

entityType: 'clock',

});

}

const until = this.now().add(duration);

return this.sleepUntil(until, context);

Copilot · 2025-12-23T06:25:01Z

lib/clock.js

+   * @param {Context} [context=null] - Context which when shut down will cause this sleep to wake early.
+   *   If context is null, then the default context is used.
+   * @return {Promise<boolean>} Promise that resolves to true if 'until' was reached,
+   *   or false if it woke for another reason (time jump, context shutdown).


The documentation states that if the context is shut down during sleep, it will "cause this sleep to wake early." However, the implementation only checks context validity before (line 149) and after (line 203) the sleep operation. There's no active monitoring or callback mechanism to wake the ClockEvent when the context is shut down during the sleep. This could lead to situations where the sleep continues even after context shutdown, contrary to what the documentation promises. Consider either updating the documentation to reflect the actual behavior, or implementing a mechanism to wake the sleep when context is shut down.

Suggested change

* @param {Context} [context=null] - Context which when shut down will cause this sleep to wake early.

* If context is null, then the default context is used.

* @return {Promise<boolean>} Promise that resolves to true if 'until' was reached,

* or false if it woke for another reason (time jump, context shutdown).

* @param {Context} [context=null] - Context whose validity will be checked while sleeping.

* If context is null, then the default context is used. If the context is found to be

* shut down before or by the time the wait completes, the returned promise resolves to false.

* @return {Promise<boolean>} Promise that resolves to true if 'until' was reached without

* detecting a time jump or a shut down context, or false otherwise (for example, if a time

* jump occurred or the context is no longer valid when the wait completes).

Copilot · 2025-12-23T06:25:02Z

test/test-clock-change-integration.js

+
+const assert = require('assert');
+const rclnodejs = require('../index.js');
+const { Clock, ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;


Unused variable Clock.

Suggested change

const { Clock, ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;

const { ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;

Copilot · 2025-12-23T06:25:02Z

test/test-clock-change-integration.js

+
+const assert = require('assert');
+const rclnodejs = require('../index.js');
+const { Clock, ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;


Unused variable Duration.

Suggested change

const { Clock, ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;

const { Clock, ROSClock, ClockType, ClockChange, Time } = rclnodejs;

Copilot · 2025-12-23T06:25:02Z

test/types/index.test-d.ts

+);
+
+// ClockChange in callback
+const clockCallback: rclnodejs.ClockCallbackObject = {


Unused variable clockCallback.

Copilot AI review requested due to automatic review settings December 16, 2025 07:51

Copilot started reviewing on behalf of minggangw December 16, 2025 07:51 View session

minggangw mentioned this pull request Dec 16, 2025

Missing rcl functions in rclnodejs #1330

Open

Copilot AI reviewed Dec 16, 2025

View reviewed changes

minggangw and others added 5 commits December 22, 2025 14:13

Add ClockEvent support

ffa4734

Update src/clock_event.hpp

adf69d6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Address comments

f845382

Fix lint error

ccc1d0f

Add sleepUntil/sleepFor for Clock

70b1f7b

minggangw force-pushed the fix-1330-11 branch from cc951c6 to 70b1f7b Compare December 23, 2025 06:14

minggangw requested a review from Copilot December 23, 2025 06:16

Copilot started reviewing on behalf of minggangw December 23, 2025 06:16 View session

Copilot AI reviewed Dec 23, 2025

View reviewed changes

minggangw added 4 commits December 23, 2025 16:21

Address comments

659c636

Merge clock change tests into one js file

8e07620

Add license

4ae6e04

Add test for active ros time

e88f1bc

	const { Clock, ClockType, Time, Duration } = rclnodejs;
	const { Clock, ClockType, Duration } = rclnodejs;

		bool lossless;
		int64_t until = info[2].As<Napi::BigInt>().Int64Value(&lossless);

		const until = this.now().add(duration);
		return this.sleepUntil(until, context);

-    const until = this.now().add(duration);
-    return this.sleepUntil(until, context);
+   if (
+     !duration ||
+     typeof duration !== 'object' ||
+     !duration.constructor ||
+     duration.constructor.name !== 'Duration'
+   ) {
+     throw new TypeValidationError('duration', duration, 'Duration', {
+       entityType: 'clock',
+     });
+   }
+   const until = this.now().add(duration);
+   return this.sleepUntil(until, context);

-   * @param {Context} [context=null] - Context which when shut down will cause this sleep to wake early.
-   *   If context is null, then the default context is used.
-   * @return {Promise<boolean>} Promise that resolves to true if 'until' was reached,
-   *   or false if it woke for another reason (time jump, context shutdown).
+   * @param {Context} [context=null] - Context whose validity will be checked while sleeping.
+   *   If context is null, then the default context is used. If the context is found to be
+   *   shut down before or by the time the wait completes, the returned promise resolves to false.
+   * @return {Promise<boolean>} Promise that resolves to true if 'until' was reached without
+   *   detecting a time jump or a shut down context, or false otherwise (for example, if a time
+   *   jump occurred or the context is no longer valid when the wait completes).

	const { Clock, ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;
	const { ROSClock, ClockType, ClockChange, Duration, Time } = rclnodejs;

Add ClockEvent support #1354

Are you sure you want to change the base?

Add ClockEvent support #1354

Uh oh!

Conversation

minggangw commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minggangw commented Dec 16, 2025 •

edited

Loading

coveralls commented Dec 16, 2025 •

edited

Loading