-
Notifications
You must be signed in to change notification settings - Fork 10.1k
Torque controller: refactor calculations to be in accel space #35790
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
Conversation
@@ -26,6 +27,7 @@ | |||
class LatControlTorque(LatControl): | |||
def __init__(self, CP, CI): | |||
super().__init__(CP, CI) | |||
self.steer_max = ISO_LATERAL_ACCEL |
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.
This may limit max torque in ways we don't want
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.
Isn't it the same effect? We were limiting from -1 to 1, which was after gravity_adjusted_lateral_accel
was converted to torque. Now we limit to the torque that can achieve 3 m/s^2?
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.
Assuming a perfectly linear torque car, I see that it could be a saturation problem if the car's lat accel factor is less than 3 m/s^2. Output could be 3 m/s^2 and torque_from_lateral_accel
would convert it to 1.5x of the max torque which would be clipped to 1, and integral would windup outside the range. Is that what you mean?
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.
We need an inverse of all torque_from_lateral_accel
functions I think, but that's impossible for the neural function without binary search which is ugly. It's simple for linear
Maybe this is why the PID controller was in torque space
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.
Actually we can revert #33970 to fix this.
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.
Pull Request Overview
This PR refactors the torque controller to perform all calculations in lateral acceleration space rather than converting to torque space early in the process. This approach allows for more accurate handling of non-linear torque responses, particularly for vehicles like the Bolt that have non-linear torque curves.
Key changes:
- Moves PID calculations to lateral acceleration space and converts to torque only at the final output step
- Simplifies error calculation by working directly with lateral acceleration values
- Updates saturation checking to use acceleration limits instead of torque limits
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
selfdrive/controls/lib/latcontrol_torque.py | Refactors torque controller calculations to work in acceleration space, simplifying PID error computation and deferring torque conversion |
opendbc_repo | Updates subproject commit to support new ISO_LATERAL_ACCEL constant |
Comments suppressed due to low confidence (2)
selfdrive/controls/lib/latcontrol_torque.py:30
- The variable name
steer_max
is misleading since it now represents a lateral acceleration limit rather than a steering/torque limit. Consider renaming tolateral_accel_max
oraccel_limit
to reflect the actual units and purpose.
self.steer_max = ISO_LATERAL_ACCEL
selfdrive/controls/lib/latcontrol_torque.py:69
- [nitpick] The variable name
output_accel
is more descriptive than the originaloutput_torque
, but consider usingpid_output_accel
orlateral_accel_output
to be more explicit about what this represents in the control flow.
output_accel = self.pid.update(pid_log.error,
This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity. |
5db20f5
to
1d192f3
Compare
9ec519d
to
6d3219b
Compare
…commaai#35790)" This reverts commit ab44c9a.
…commaai#35790)" This reverts commit ab44c9a.
…commaai#35790)" This reverts commit ab44c9a.
…commaai#35790)" This reverts commit ab44c9a.
Idea thanks to Harald. Moves everything to be in accel space until the very end. Instead of: #35780
This works because error is added onto feedforward, which is then converted at the right place on the non-linear curve for Bolt at the end.
Changes behavior slightly for Bolt, checking for linear cars...
opendbc: commaai/opendbc#2528