Add limit setters to SlewRateLimiter
#7793
Conversation
|
/format |
|
/format has been temporarily disabled. You'll have to run wpiformat yourself. |
This comment has been minimized.
This comment has been minimized.
katzuv
force-pushed
the
slew-rate-limiter-rate-limits
branch
from
84f2278 to
173ae8f
Compare
There was a problem hiding this comment.
Not sure if it should be a setter or
withLimit()so you can chain it (limiter.withLimit(newLimit).calculate(...)).
Personally I think either having a setter or having optional parameters on calculate() would be good. A setter is good for when the calculation of what the limits should be happens in a different spot from where the limits are applied, while optional parameters on calculate() is good for when the limits are calculated where the limits are applied.
Using withLimit() also covers the second case, but my personal subjective feeling is that it's weird to use with[a-z]*() on reused objects that do more than carry configuration information. (That said, this is all subjective feeling and I'm fine with it going either way)
Not sure about the C++ implementation.
Aside from the comment that also applies to Java, it looks good to this local not-an-actual-WPILib-maintainer!
| */ | ||
| public void setLimit(double rateLimit) { | ||
| m_positiveRateLimit = rateLimit; | ||
| m_negativeRateLimit = rateLimit; |
There was a problem hiding this comment.
| m_negativeRateLimit = rateLimit; | |
| m_negativeRateLimit = -rateLimit; |
You could also make the first sentence of the doc comment match the first sentence of the SlewRateLimit(double) doc comment, but that's up to you.
| */ | ||
| void SetLimit(Rate_t rateLimit) { | ||
| m_positiveRateLimit = rateLimit; | ||
| m_negativeRateLimit = rateLimit; |
There was a problem hiding this comment.
| m_negativeRateLimit = rateLimit; | |
| m_negativeRateLimit = -rateLimit; |
Same as Java.
This helps when the rate limit needs to be dynamically changed, such as when the drivetrain's acceleration is limited based on an elevator's height.
Not sure if it should be a setter or
withLimit()so you can chain it (limiter.withLimit(newLimit).calculate(...)). Not sure about the C++ implementation. Feedback appreciated.