ros2 / rclc

ROS Client Library for the C language.
Apache License 2.0
119 stars 42 forks source link

fix: deprecated rcl_timer_init #389

Closed Zard-C closed 1 year ago

Zard-C commented 1 year ago

Hi, I am trying to fix the rcl_timer_init problem.

pablogs9 commented 1 year ago

Don't they need the extra bool argument?

https://github.com/ros2/rcl/blob/b55c41b8a877508f69a39daf8f0f8d9e551c69e8/rcl/include/rcl/timer.h#L172

Zard-C commented 1 year ago

Don't they need the extra bool argument?

https://github.com/ros2/rcl/blob/b55c41b8a877508f69a39daf8f0f8d9e551c69e8/rcl/include/rcl/timer.h#L172

Thanks for your CR, I am trying fix my error by passing bool argument true as the last one, Is this modification appropriate in rclc/rclc/src/rclc/timer.c?

  rcl_ret_t rc = rcl_timer_init2(
    timer,
    &support->clock,
    &support->context,
    timeout_ns,
    callback,
    (*support->allocator),
    true);
  if (rc != RCL_RET_OK) {
    PRINT_RCLC_ERROR(rclc_timer_init_default, rcl_timer_init2);
  } else {
    RCUTILS_LOG_INFO("Created a timer with period %ld ms.\n", timeout_ns / 1000000);
  }
  return rc;
Zard-C commented 1 year ago

Thanks a lot to @pablogs9 's helping, I've read https://github.com/ros2/rcl/pull/1004/commits/22dc947cc23981343293668e2a9da9a7932057e9, https://github.com/ros2/rcl/blob/b55c41b8a877508f69a39daf8f0f8d9e551c69e8/rcl/include/rcl/timer.h#L172, the and modified my commit, hope it works

Zard-C commented 1 year ago

I do not like this naming convention of adding a 2 at the end. But as it is used in ROS 2, LGTM.

As far as I understand as soon as the change definitively the naming in rolling we can get rid of this 2 right?

Exactly, maybe rcl_timer_init_with_start or rcl_timer_init_and_start is more suitable,or just rcl_timer_init_default?