rclcpp
rclcpp copied to clipboard
ros2
A timer that can be triggered a finite number of times.
Feature request
Feature description
There can be situations in which you would like to call a function a finite number of times within the (same period of time) and not to be called endlessly. For those kind of situations the proposal is to give the timers the option to select the number of times that the timer is triggered. An example of use can be a "One Shot Timer" (which was implemented in ROS 1).
Implementation considerations
Given that use case stated above, it is possible to add the feature by keeping track of the number of times the timer is being triggered and canceling it after the number of callbacks is completed. An example of this implementation can be found on the following fork:
- https://github.com/Voldivh/rclcpp/tree/voldivh/callback_counter
IMO a feature like this can be easily implement via a wrapper around the Timer class (and this is also how I would recommend to implement it, without modifying the existing classes.
However, I think that as long as something can be easily implemented by a wrapper, we should think twice before including it in rclcpp. The main reason is to avoid bloating rclcpp with any possible extension/variation of the ROS 2 basic entities.
A different story instead is any feature that would require to modify the internals of rclcpp in order to be implemented.
i am with @alsora on this, i think that user can handle this in the user callback internally.
I don't think that this gets anywhere near the definition of bloat. It requires two extra internal unsigned int and conditionally increments them. The constructor gets an additional default value.
rclcpp is designed to be the user friendly native developer interface for ROS in C++. Providing this feature is a core capability of a Timer. If we don't add it here, we'll need to make a parallel class. And I don't think that it's clearer or more discoverable to have a parallel class which would just replace a default argument.
The real implementation of this is actually at the lower level in rcl. And the rclcpp Timer class is just making a nice c++ API on top of that. This does not change the underlying fundamentals of any of the system.
If we do not integrate this into the core Time implementation what would you like to see a parallel class for the wrapper?
We're also likely to soon need to also provide Timers that don't autostart as well (#1980). Which in ROS 1 was also a default parameter. Which if we want to support the various combinations sounds like a potentially very complex situation compared to having two default parameters that the user can set.
It is a bloat because the feature is easily implementable by the user and apparently not a common case. It's not about the size of the implementation.
The ROS ecosystem already suffers from the habit of thin wrappers in the name of user-friendliness. This is not a good design philosophy. Users should learn to code.
It is a bloat because the feature is easily implementable by the user and apparently not a common case. It's not about the size of the implementation.
This does not appear to be a supported argument.
The ROS ecosystem already suffers from the habit of thin wrappers in the name of user-friendliness. This is not a good design philosophy. Users should learn to code.
With the philosophy that "users should learn to code" what should we provide at all? Pretty much all of software is just wrappers and abstractions, we can get rid of those and go back to punch cards down to real hardware? Designing and building good abstractions is the job of an effective software engineer.
Providing user friendly interfaces has been the hallmark of ROS projects. This has been one of the main reasons that ROS has been able to grow and thrive to the size that it has. We provide useful interfaces that make other developers able to write more efficient simpler code. Enabling code reuse across projects and avoiding code duplication. If we can write this once and do it well, it's much better for the ecosystem than hundreds of people writing the same or similar functionality into their software.
As a counter point: I am a user who knows how to code. And I want this feature integrated into our user focused API to make downstream project code efficient and compact.
This does not appear to be a supported argument.
👍 agree, the topic here is NOT whether bloat or not, let's just focus on feature.
Sorry, i totally did misunderstand this feature. i was thinking that feature is to ask the timer to call the user callback for specified times on each cyclic expire...
having one-shot (or specified number) timer and start/stop control in base class sounds reasonable. any thoughts?
These questions must be answered before deciding on the implementation:
- How many times does the user need a timer? (base estimation)
- How many times does the user need a one-shot timer?
- How many times does the user need an arbitrary shot timer?
If these (2&3) are estimated to be the rare cases compared to 1, then the decision to implement this feature forces many users to pay its price unnecessarily. In standard C++ library, this is formulated as zero-overhead principle and such implementations are avoided.
Considering that multiple developers agree on this feature, I'm ok with integrating it. However, I also think that the ros core should pay attention to the zero overhead principle, as described by @doganulus.
We should make sure that the new counter add zero overhead to the "old" one. Moreover, should this feature maybe be implemented in the C layer?
This issue has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/complexity-performance-analyzer-and-consideration/27170/1
This probably isn't very useful, but in case it is: I had need of something like this a while back, so I implemented a packaged wrapper. It's made to roughly approximate the threading.Timer interface, since that is what I needed, but there is a one-shot class.
I see that there is an active PR -- and I admittedly didn't read through this entire thread -- so I apologize if this is just noise.
