Add ClockEvent support #1354
Open
Add ClockEvent support #1354
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
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.
Comment on lines
+7
to
+45
| 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); | ||
| }); |
There was a problem hiding this comment.
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.
test/test-clock-event.js
Outdated
|
|
||
| const assert = require('assert'); | ||
| const rclnodejs = require('../index.js'); | ||
| const { Clock, ClockType, Time, Duration } = rclnodejs; |
There was a problem hiding this comment.
Unused variable Time.
Suggested change
| const { Clock, ClockType, Time, Duration } = rclnodejs; | |
| const { Clock, ClockType, Duration } = rclnodejs; |
Co-authored-by: Copilot <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Fix: #1330