refactor: remove period parameter and make rate double #108
Conversation
| this->add_predicate("in_error_state", false); | ||
|
|
||
| this->rate_ = this->get_parameter_value<double>("rate"); | ||
| this->period_ = 1.0 / this->rate_; |
There was a problem hiding this comment.
Do we really need to have a member variable for the period? As in, couldn't we just call the get_period() function which in C++ could be a constexpr 1 / rate_. Clearly this is not a mistake, just trying to understand if it's for ease of use in derived classes or serves another purpose.
Same goes for the python code.
There was a problem hiding this comment.
I wanted to save one operation...No there is no other purpose, I will remove it 👍
There was a problem hiding this comment.
no strong opinion on this honestly. And I do hear the argumentation of saving one operation. The reason I asked is because we rarely use period in our derived classes.
There was a problem hiding this comment.
The reason I asked is because we rarely use period in our derived classes.
We have several motion components that are interested in the component period for sinusoidal motion or to keep track of elapsed time. At the end, it's all about providing the helpers to simplify the work of the users.
There was a problem hiding this comment.
You are right yes. Then let's leave it and resolve this question. Thank you!
eeberhard
left a comment
eeberhard
left a comment
There was a problem hiding this comment.
Nicely done, I really like that with the helper functions 👍
3 participants
Description
See quick discussion in parent issue. We added the
rateparameter previously in a non breaking way which required a few hacks with the parameters overrides. I think it will be nice to commit to a double rate parameter for v5. I added some helpers to get the period directly as few different types.Review guidelines
Estimated Time of Review: 10 minutes
Checklist before merging: