added dt to controller interface and controller manager #438

[dignakov]

I took a stab at adding #438. This is by no means complete, but if no one has started work on this, I'd like to get some feedback and I can keep working on it.

[destogl]

This looks good. The only question I have is how do we want to proceed. Should we provide dt or absolute current time? Anyway, IMO, we should provide read measured time since the last update call and not theoretical one. What do you think?

[dignakov]

Should we provide dt or absolute current time

I didn't see the discussion when I started, so I went with dt, but I can see why time would make sense, especially if there's a plan to have different controllers execute at different rates.

What I'm not sure about is what would be the source of the absolute time, would it come from ros, or something like steady_clock, or something else? (system_clock is used in the ros2_control_node, so just reuse that?)

Either way, I'm happy to try and implement the change if you'd like to proceed with absolute current time.

Anyway, IMO, we should provide read measured time since the last update call and not theoretical one.

I agree. But I wasn't sure about the right source of time to use, so I just used the theoretical update rate.

Also, what would we provide on the very first iteration?

I think also that the following is possible: (it would make more sense and less "0")

Yup, agreed. I'll change that too depending on what you'd like to do about point 1.

I went ahead and implemented the above two for now.

Please let me know if it's better to go with absolute current time instead.

[dignakov]

update functions in controller_interface and controller_manager now accept both time and period:
update(const rclcpp::Time& time, const rclcpp::Duration& period)

[destogl]

Generally I agree, just few points to discuss.

Also, do we need a test for this functionality? A simple one testing parameter propagation?

[destogl]

Maybe we should provide only time in this method and period is calculated just before controller-update is called. What do you think?

[dignakov]

Would that still work with node->now() from your comment below to get a good dt or do you think it's ok to add a couple of variables there to track dt using steady_clock?

[bmagyar]

@destogl the purpose of this API is to provide both as both should be available from the CM time without having to re-compute the same stuff all the time.

[bmagyar]

@destogl in terms of a test for this... I don't think we need one at this point. The main point of this PR is to extend the API which was achieved. Testing that is testing that C++ can propagate arguments to a function :D I don't think we need that

[destogl]

Thanks for the work and follow-up to getting this PR straight! 🤖