Time#get secondsAndNanoseconds bug

[wayneparrott]

I've isolated an issue in Time where _nanoseconds is being modified when calling the getter 'secondsAndNanoseconds' in version 0.10.2 on MacOS Catalina.

This example program prints out the _nanoseconds before and after calling secondsAndNanoseconds. I can see _nanoseconds change after calling secondsAndNanoseconds. If I comment out the call to secondsAndNanoseconds, _nanoseconds remains unchanged. Notice in the example output how time2 values of _nanosecond are a subset of the original time1 value.

example output
time1 1572996022175992874
time2 572996022
time1 1572996022925996911
time2 572996022
time1 1572996023677128755
time2 572996023
time1 1572996024427889376
time2 572996024
time1 1572996025178983458
time2 572996025
time1 1572996025932756150
time2 572996025

const rcl = require('rclnodejs');

rcl.init().then(() => {
  const node = rcl.createNode('timer_ghost_node');
  const clock = new rcl.Clock();

  setInterval(() => {
    const now = clock.now();
    console.log('time1', now._nanoseconds);
    
    // the following getter is goofing up the _nanoseconds value of variable "now"
    // comment out this next line and _nanoseconds remains constant
    now.secondsAndNanoseconds;
    
    console.log('time2', now._nanoseconds);
  
  }, 750);

  rcl.spin(node);
});

[wayneparrott]

I'm not exactly clear on how Timer _nanoseconds is getting goofed up. In my branch I created a new int64 from _nanoseconds as shown below and this works for me.

get secondsAndNanoseconds() {
  
  const nanos = int64.from(this._nanoseconds);
  const seconds = nanos.divide(1e9).toNumber();
  const nanoseconds = nanos.mod(1e9).toNumber();
  
  return {seconds, nanoseconds};
}

Thoughts? I can submit a PR with this modification.

[minggangw]

Oops, there should be using a temporary var to store the result as the value will be changed after calling the function (secondsAndNanoseconds as property), so you got the wrong number when you called now._nanoseconds the second time.

As JavaScript cannot represent 64 bits integers, we leverage int64-napi to handle it which caused this mess. I think we should write something like below and it's better if you could add a test into test-time.js with your PR, thanks a lot!

get secondsAndNanoseconds() {
  const seconds = int64.from(this._nanoseconds).divide(1e9).toNumber();
  const nanoseconds = int64.from(this._nanoseconds).mod(1e9).toNumber();
  
  return {seconds, nanoseconds};
}

[wayneparrott]

Checking in to see if there are actions I need to take to get the fix committed? I have observed the work with CI config to get builds working correctly.

[minggangw]

I have resolve the problem on macOS/Linux platforms, see #536. But on Windows10, I meet the error of importing rclpy module constantly and I haven't found the root cause of this. Do you meet the same error on Windows?

P.S. instruction about the error of importing Python module: https://index.ros.org/doc/ros2/Troubleshooting/#import-failing-even-with-library-present-on-the-system

[wayneparrott]

re: your Q about my experience on Windows, can't say as I don't have a windows system atm.

[minggangw]

Hi @wayneparrott, finally I get all CIs to be green 😄 , please check the latest SHA d2724ff. So would you please rebase your PR (#533) on the develop branch and push it again (hope it can pass this time), thanks!

[minggangw]

Hi @wayneparrott, I have rebased your patch and landed it on the develop branch and it works well. Based on your latest commit, I published a new release of rclnodejs - 0.10.3. You are welcome to try it, thanks again!