[vcu] cloran controls#223
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive traction control system and an enhanced torque mapping component with power limiting. Key changes include the addition of a new TractionControl component, a major update to TorqueMap to include efficiency-aware power limiting and OCV estimation, and the integration of various CAN inputs (wheel speeds, battery status, motor speed) into the VCU model. Additionally, new CAN messages for debugging traction control were added, and unit tests were implemented for the new logic. Feedback focuses on the potential inaccuracy of using unsprung acceleration sensors for vehicle dynamics due to steering rotation, the stability of safety cuts by using filtered power values, and the replacement of magic numbers with constants.
| in->longitudinal_accel_mps2 = | ||
| 0.25f * (unsprung_accel_fl_mailbox.x + unsprung_accel_fr_mailbox.x + | ||
| unsprung_accel_rl_mailbox.x + unsprung_accel_rr_mailbox.x); | ||
| in->lateral_accel_mps2 = | ||
| 0.25f * (unsprung_accel_fl_mailbox.y + unsprung_accel_fr_mailbox.y + | ||
| unsprung_accel_rl_mailbox.y + unsprung_accel_rr_mailbox.y); |
There was a problem hiding this comment.
The longitudinal and lateral acceleration are being calculated by averaging the 'x' and 'y' axes of the unsprung acceleration sensors. However, unsprung sensors (mounted on the wheel hubs/uprights) rotate with the steering assembly. When the steering angle is non-zero, the 'x' and 'y' axes of the front sensors no longer align with the vehicle's longitudinal and lateral axes. This will introduce significant errors in the acceleration estimates used for traction control and power limiting during cornering. It is highly recommended to use sprung mass (chassis-mounted) acceleration sensors for vehicle dynamics control.
| if (state->filtered_current_a > hard_current_cut_a || | ||
| measured_power_w > hard_power_cut_w) { |
There was a problem hiding this comment.
The safety cut for power uses the raw measured_power_w, which is susceptible to high-frequency noise and spikes. This could lead to nuisance trips and 'chattering' of the torque command at the power limit. It is safer and more stable to use the filtered power value state->filtered_power_w for this safety check, as is done for the current safety cut.
| if (state->filtered_current_a > hard_current_cut_a || | |
| measured_power_w > hard_power_cut_w) { | |
| if (state->filtered_current_a > hard_current_cut_a || | |
| state->filtered_power_w > hard_power_cut_w) { |
| float motor_mps = 0.0f; | ||
| bool motor_valid = in->inverter_speed_valid && final_drive_ratio > 0.0f; | ||
| if (motor_valid) { | ||
| float motor_rad_s = fabsf(in->motor_speed_rpm) * 6.28318530718f / 60.0f; |
There was a problem hiding this comment.
Avoid using magic numbers for mathematical constants like 2*pi. Use a shared constant or define it at the top of the file for better maintainability.
| float motor_rad_s = fabsf(in->motor_speed_rpm) * 6.28318530718f / 60.0f; | |
| float motor_rad_s = fabsf(in->motor_speed_rpm) * 2.0f * M_PI / 60.0f; |
…ather than silently defaulting
… Inverter voltage and current for PL calcs.
github-actions
Bot
added
the
documentation
1 participant
No description provided.