-
Notifications
You must be signed in to change notification settings - Fork 79
Change timer period to type of bigint in nanoseconds #1038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
test/test-msg-type-cpp-node.js:61
- The timer period value was changed from 100 (ms) to BigInt(100000) (ns), which implies a conversion factor of 1000. Please confirm that this conversion accurately reflects the intended timer duration now measured in nanoseconds.
var timer = node.createTimer(BigInt(100000), () => {
test/publisher_array_setup.js:42
- [nitpick] Different test files now use varying BigInt literal values (e.g. BigInt(100000) vs BigInt('100000000')). It would be beneficial to standardize the conversion factor from the previous millisecond values to nanoseconds to ensure consistent timer behavior across tests.
var timer = node.createTimer(BigInt('100000000'), () => {
| } else if (arguments.length === 4) { | ||
| clock = arguments[3]; | ||
| } | ||
|
|
Copilot
AI
Feb 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new bigint-based period in nanoseconds, the previous range validation (designed for millisecond values) has been removed. Consider adding a new range check that verifies the period is within acceptable nanosecond limits to avoid unintended timer values.
| const MIN_PERIOD = 1n; // 1 nanosecond | |
| const MAX_PERIOD = 3600000000000n; // 1 hour in nanoseconds |
Per current implementation, the timer period is in milliseconds, because the range limitation of integer that JavaScript can [represents](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER). While, after [`BigInt`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt) introduced, we can change the timer period to type of `bigint` in nanoseconds, which aligns with other ROS2 clients, like `rclpy`. This patch implements: 1. Change the timer period to type of `bigint` in nanoseconds for JavaScript side. 2. Get the period from `v8::BigInt` object as `int64_t` and pass it to `rcl` from C++ side. 3. Update the related unit tests accordingly. Fix: #1037
Per current implementation, the timer period is in milliseconds, because the range limitation of integer that JavaScript can represents. While, after
BigIntintroduced, we can change the timer period to type ofbigintin nanoseconds, which aligns with other ROS2 clients, likerclpy.This patch implements:
bigintin nanoseconds for JavaScript side.v8::BigIntobject asint64_tand pass it torclfrom C++ side.Fix: #1037