-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[mppi] Don't reset zone-based speed limits #5768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Adi Vardi <[email protected]>
Signed-off-by: Adi Vardi <[email protected]>
Signed-off-by: Adi Vardi <[email protected]>
f38e6c0 to
dc5fa33
Compare
SteveMacenski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other 2 lines I agree with
| // Don't reset zone-based speed limits after params changes | ||
| parameters_handler_->addPostCallback([this]() {reset(false);}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the parameters were updated, that can mean that there are new maximum/minimum limits that need to be respected. If for instance the speed limits are set as 50% of 1m/s but the new max controller speed is now 0.3m/s, that would be invalid. That seems like a potential problem on this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
I can see different approaches, each could make sense:
- Run the speed limit conversion again (so in your example, new max speed will be 50% * 0.3 = 0.15m/s)
- Take the lowest between the old speed with speed limit and the new value from reconfigure without speed limit
- Ignore the speed limit and only use the new value from reconfigure
I think (1) is the most strict and consistent, but maybe a bit less intuitive (user sets a new speed, but it's then modified).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you having a situation where you're messing with the parameters in the middle of a mission via dynamic parameters? That feels like it might not be a wise choice if also using speed limit callbacks.
I also just looked at setSpeedLimit and I guess nothing about this technically prevents the ratio or set velocity from being larger than the limits in the controller (i.e. 110% or 100m/s). So... I suppose you're right that we could keep it as false to not mess with the current limits. I don't think I see an issue with keeping that at the end of a control sequence, even if the new limits are not reset here. that's not intuitive to me because then the new limits really wouldn't be looked at until a no speed limit set enum comes into the setSpeedLimit, but I suppose (?) that's the workflow as intended.
Messing around with both velocities in dynamic parameters and speed limits is super duper inadvisable which is what makes this subtle. Maybe the better action is to set boundaries that you can only set one or the other - so that this workflow is simpler to think about and programming logic? If a dynamic parameter comes in but s.constraints != s.base_constraints, reject the update. Then the rest of what you have now would be fine and we wouldn't have to be concerned about this in the future.
Basic Info
Description of contribution in a few bullet points
Fix 2 bugs:
reset()flag correctly to avoid resetting speed limits when other parameters are changed and after a fallback.setSpeedLimit()so that the new speed limit is updated in the motion model settings. This way thepredict()function is also aware of the new speed limit.Description of documentation updates required from your changes
None
Description of how this change was tested
Tested in simulation:
Bug 1:
Bug 2:
Print the value of one of the constraints params inside
predict(). Without this PR, it is never updated. With it, is it updated when entering/leaving a speed limit zone.Future work that may be required in bullet points
None
For Maintainers:
backport-*.