Add Volkswagen carcontroller and values modules (MQB/PQ platform support) - #9
Open
Terges wants to merge 2 commits into
Open
Add Volkswagen carcontroller and values modules (MQB/PQ platform support)#9Terges wants to merge 2 commits into
Terges wants to merge 2 commits into
Conversation
…M, STEER_LOW_TORQUE, STEER_TIME_LOW_TORQUE, STEER_TIME_RESET)
…CA-disabled frame' workaround to PQ and MLB racks; update CarController to select CCS by flags and send dual steering outputs where needed
Reviewer's GuideIntroduces Volkswagen MQB/PQ platform support by adding a VW-specific CarController that handles steering, longitudinal ACC, HUD, and stock ACC button emulation, plus a values module that defines platform configs, car specs, CAN layout, firmware query behavior, and fuzzy VIN/FW matching for a wide range of VW-group models. Sequence diagram for Volkswagen CarController update cyclesequenceDiagram
participant CarController
participant SubMaster_longPlan as SubMaster_longitudinalPlanSP
participant CCS as Volkswagen_CAN_Signals
participant CANPacker_pt as CANPacker_pt
participant VW_ECU as Volkswagen_PT_CAN
CarController->>SubMaster_longPlan: update(0)
alt openpilotLongitudinalDisabled
SubMaster_longPlan-->>CarController: longitudinalPlanSP (v_tsc_state, slc_state, m_tsc_state, speed_limit)
end
loop every_STEER_STEP
CarController->>CarController: apply_driver_steer_torque_limits()
CarController->>CarController: EPS_timer_mitigation_logic
CarController->>CCS: create_steering_control(packer_pt, CANBUS_pt, apply_steer, hca_enabled)
CCS-->>CarController: steer_msg
CarController->>VW_ECU: send steer_msg
opt STOCK_HCA_PRESENT
CarController->>CCS: create_eps_update(packer_pt, CANBUS_cam, eps_stock_values, ea_simulated_torque)
CCS-->>CarController: eps_update_msg
CarController->>VW_ECU: send eps_update_msg
end
end
loop every_ACC_CONTROL_STEP when openpilotLongitudinalControl
CarController->>CCS: acc_control_value(cruise_available, accFaulted, longActive)
CCS-->>CarController: acc_control
CarController->>CCS: create_acc_accel_control(packer_pt, CANBUS_pt, acc_type, longActive, accel, acc_control, stopping, starting, esp_hold_confirmation)
CCS-->>CarController: acc_accel_msg
CarController->>VW_ECU: send acc_accel_msg
end
loop LDW_STEP and ACC_HUD_STEP
CarController->>CCS: create_lka_hud_control(...)
CCS-->>CarController: lka_hud_msg
CarController->>VW_ECU: send lka_hud_msg
CarController->>CCS: acc_hud_status_value(...)
CCS-->>CarController: acc_hud_status
CarController->>CCS: create_acc_hud_control(...)
CCS-->>CarController: acc_hud_msg
CarController->>VW_ECU: send acc_hud_msg
end
alt stock_pcc_buttons
CarController->>CCS: create_acc_buttons_control(... cancel/resume ...)
CCS-->>CarController: acc_button_msg
CarController->>VW_ECU: send acc_button_msg
else openpilot_button_emulation
CarController->>CarController: get_cruise_buttons(CS, vCruise)
CarController->>CCS: create_acc_buttons_control(... buttons, custom_stock_long=True)
CCS-->>CarController: acc_button_msg_emu
CarController->>VW_ECU: send acc_button_msg_emu
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- CarController.init assigns self.CCS twice and references VolkswagenFlags.MLB which doesn’t exist in VolkswagenFlags, so the MLB branch is currently dead/buggy and the initial assignment is redundant—clean this logic up and ensure flags align with the values module.
- In update(), new_actuators.steer is set twice in succession (first from apply_steer_last, then immediately from output_steer), which effectively discards the first value; clarify which value should be exposed and remove the redundant assignment.
- The calls to create_acc_buttons_control use different parameter patterns (one with self.ext_bus and one without a bus argument), which is likely inconsistent with the helper’s expected signature; standardize these calls to avoid sending ACC button commands on the wrong bus or with incorrect parameters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- CarController.__init__ assigns self.CCS twice and references VolkswagenFlags.MLB which doesn’t exist in VolkswagenFlags, so the MLB branch is currently dead/buggy and the initial assignment is redundant—clean this logic up and ensure flags align with the values module.
- In update(), new_actuators.steer is set twice in succession (first from apply_steer_last, then immediately from output_steer), which effectively discards the first value; clarify which value should be exposed and remove the redundant assignment.
- The calls to create_acc_buttons_control use different parameter patterns (one with self.ext_bus and one without a bus argument), which is likely inconsistent with the helper’s expected signature; standardize these calls to avoid sending ACC button commands on the wrong bus or with incorrect parameters.
## Individual Comments
### Comment 1
<location path="selfdrive/car/volkswagen/carcontroller.py" line_range="157-160" />
<code_context>
+ self.hca_frame_timer_running = 0
+ apply_steer = 0
+
+ self.eps_timer_soft_disable_alert = self.hca_frame_timer_running > self.CCP.STEER_TIME_ALERT / DT_CTRL
+ self.apply_steer_last = apply_steer
+ can_sends.append(self.CCS.create_steering_control(self.packer_pt, CANBUS.pt, apply_steer, hca_enabled))
+ can_sends.append(self.CCS.create_steering_control(self.packer_pt, CANBUS.pt, output_steer, hca_enabled))
+
+ if self.CP.flags & VolkswagenFlags.STOCK_HCA_PRESENT:
</code_context>
<issue_to_address>
**issue (bug_risk):** Steering command is sent twice per cycle with different torque values on the same bus, which is likely unintended.
Within a single `STEER_STEP`, `create_steering_control` is called twice on `CANBUS.pt`: once with `apply_steer` and once with `output_steer`. Because `output_steer` is usually equal to `apply_steer`, this sends duplicate frames; when they differ (due to mitigation logic), the second call overrides the first. To avoid confusion and unintended behavior, consider emitting a single steering control frame per cycle using the final torque value after the enable/timer logic has selected it.
</issue_to_address>
### Comment 2
<location path="selfdrive/car/volkswagen/carcontroller.py" line_range="228-231" />
<code_context>
+ self.send_count = 0
+ self.last_cruise_button = self.cruise_button
+
+ new_actuators = actuators.as_builder()
+ new_actuators.steer = self.apply_steer_last / self.CCP.STEER_MAX
+ new_actuators.steer = output_steer / self.CCP.STEER_MAX
+ new_actuators.steerOutputCan = self.apply_steer_last
+
+ self.gra_acc_counter_last = CS.gra_stock_values["COUNTER"]
</code_context>
<issue_to_address>
**issue (bug_risk):** new_actuators.steer is assigned twice, with the first assignment immediately overwritten.
`new_actuators.steer` is first set from `self.apply_steer_last` then immediately overwritten by `output_steer`, so the initial value is never used. Please decide which value should populate `steer` and remove the redundant assignment, or introduce a separate field if you need to expose both the commanded and mitigated values.
</issue_to_address>
### Comment 3
<location path="selfdrive/car/volkswagen/carcontroller.py" line_range="100" />
<code_context>
+ self.param_s.put_bool_nonblocking("LastSpeedLimitSignTap", False)
+ self.last_speed_limit_sign_tap_prev = CS.params_list.last_speed_limit_sign_tap
+
+ sl_force_active = CS.params_list.speed_limit_control_enabled and (self.frame < (self.sl_force_active_timer * DT_CTRL + 2.0))
+ sl_inactive = not sl_force_active and (not CS.params_list.speed_limit_control_enabled or (True if self.slc_state == 0 else False))
+ sl_temp_inactive = not sl_force_active and (CS.params_list.speed_limit_control_enabled and (True if self.slc_state == 1 else False))
</code_context>
<issue_to_address>
**issue (bug_risk):** Speed-limit force-active timeout mixes frame counts and seconds, likely causing an incorrect duration.
`self.sl_force_active_timer` is a frame index, but the condition compares `self.frame` (frames) to `self.sl_force_active_timer * DT_CTRL + 2.0` (seconds). This mixes units, so the force-active duration is scaled incorrectly by 1/DT_CTRL. If the goal is a 2-second window after the tap, the comparison should use consistent time units, e.g. `(self.frame * DT_CTRL) < (self.sl_force_active_timer * DT_CTRL + 2.0)` or `self.frame < self.sl_force_active_timer + 2.0 / DT_CTRL`.
</issue_to_address>
### Comment 4
<location path="selfdrive/car/volkswagen/carcontroller.py" line_range="220-221" />
<code_context>
+ self.cruise_button = 1 if self.cruise_button == 1 else 2 # accel, decel
+ elif self.acc_type == 1:
+ self.cruise_button = 3 if self.cruise_button == 1 else 4 # resume, set
+ if self.frame % self.CCP.BTN_STEP == 0:
+ can_sends.append(self.CCS.create_acc_buttons_control(self.packer_pt, CS.gra_stock_values, frame=(self.frame // self.CCP.BTN_STEP),
+ buttons=self.cruise_button, custom_stock_long=True))
+ self.send_count += 1
</code_context>
<issue_to_address>
**issue (bug_risk):** create_acc_buttons_control is called with a different signature than in the earlier branch, which may not match the helper's API.
In the cancel/resume path above, `create_acc_buttons_control` is called as `create_acc_buttons_control(self.packer_pt, self.ext_bus, CS.gra_stock_values, cancel=..., resume=...)`, but here it is `create_acc_buttons_control(self.packer_pt, CS.gra_stock_values, frame=..., buttons=..., custom_stock_long=True)` with no bus argument and shifted positionals. Unless the helper is overloaded for both forms, this will either misassign the bus value or raise a runtime error. Please make the bus argument explicit and align positional/keyword usage with the other call site.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+157
to
+160
| self.eps_timer_soft_disable_alert = self.hca_frame_timer_running > self.CCP.STEER_TIME_ALERT / DT_CTRL | ||
| self.apply_steer_last = apply_steer | ||
| can_sends.append(self.CCS.create_steering_control(self.packer_pt, CANBUS.pt, apply_steer, hca_enabled)) | ||
| can_sends.append(self.CCS.create_steering_control(self.packer_pt, CANBUS.pt, output_steer, hca_enabled)) |
There was a problem hiding this comment.
issue (bug_risk): Steering command is sent twice per cycle with different torque values on the same bus, which is likely unintended.
Within a single STEER_STEP, create_steering_control is called twice on CANBUS.pt: once with apply_steer and once with output_steer. Because output_steer is usually equal to apply_steer, this sends duplicate frames; when they differ (due to mitigation logic), the second call overrides the first. To avoid confusion and unintended behavior, consider emitting a single steering control frame per cycle using the final torque value after the enable/timer logic has selected it.
Comment on lines
+228
to
+231
| new_actuators = actuators.as_builder() | ||
| new_actuators.steer = self.apply_steer_last / self.CCP.STEER_MAX | ||
| new_actuators.steer = output_steer / self.CCP.STEER_MAX | ||
| new_actuators.steerOutputCan = self.apply_steer_last |
There was a problem hiding this comment.
issue (bug_risk): new_actuators.steer is assigned twice, with the first assignment immediately overwritten.
new_actuators.steer is first set from self.apply_steer_last then immediately overwritten by output_steer, so the initial value is never used. Please decide which value should populate steer and remove the redundant assignment, or introduce a separate field if you need to expose both the commanded and mitigated values.
| self.param_s.put_bool_nonblocking("LastSpeedLimitSignTap", False) | ||
| self.last_speed_limit_sign_tap_prev = CS.params_list.last_speed_limit_sign_tap | ||
|
|
||
| sl_force_active = CS.params_list.speed_limit_control_enabled and (self.frame < (self.sl_force_active_timer * DT_CTRL + 2.0)) |
There was a problem hiding this comment.
issue (bug_risk): Speed-limit force-active timeout mixes frame counts and seconds, likely causing an incorrect duration.
self.sl_force_active_timer is a frame index, but the condition compares self.frame (frames) to self.sl_force_active_timer * DT_CTRL + 2.0 (seconds). This mixes units, so the force-active duration is scaled incorrectly by 1/DT_CTRL. If the goal is a 2-second window after the tap, the comparison should use consistent time units, e.g. (self.frame * DT_CTRL) < (self.sl_force_active_timer * DT_CTRL + 2.0) or self.frame < self.sl_force_active_timer + 2.0 / DT_CTRL.
Comment on lines
+220
to
+221
| if self.frame % self.CCP.BTN_STEP == 0: | ||
| can_sends.append(self.CCS.create_acc_buttons_control(self.packer_pt, CS.gra_stock_values, frame=(self.frame // self.CCP.BTN_STEP), |
There was a problem hiding this comment.
issue (bug_risk): create_acc_buttons_control is called with a different signature than in the earlier branch, which may not match the helper's API.
In the cancel/resume path above, create_acc_buttons_control is called as create_acc_buttons_control(self.packer_pt, self.ext_bus, CS.gra_stock_values, cancel=..., resume=...), but here it is create_acc_buttons_control(self.packer_pt, CS.gra_stock_values, frame=..., buttons=..., custom_stock_long=True) with no bus argument and shifted positionals. Unless the helper is overloaded for both forms, this will either misassign the bus value or raise a runtime error. Please make the bus argument explicit and align positional/keyword usage with the other call site.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a Volkswagen-specific CarController implementation and a values module to provide CAN definitions, platform/car specs, and firmware-query configuration for MQB and PQ Volkswagen group vehicles.
Implements lateral (HCA) steering control, longitudinal (ACC) control and HUD messaging, stock ACC button emulation, cruise-button automation logic, and fuzzy FW-to-car matching for VIN/platform detection.
What changed
new file: selfdrive/car/volkswagen/carcontroller.py
Implements CarController class for Volkswagen vehicles:
Steering logic with EPS timer mitigation, torque limits, and HCA enable/disable handling.
Acceleration/ACC control message generation for openpilot longitudinal control.
HUD (LKA/ACC) message creation and lead display handling.
Stock ACC button control and automated button-press logic (multi-mode handling for different ACC behaviors).
Helper methods for curve/turn speed, speed-limit integration, and button-press state machine.
Integrates different CAN layouts for MQB / PQ / MLB using flags.
new file: selfdrive/car/volkswagen/values.py
Defines CarControllerParams (steer/ACC timing, limits, message steps) and CANBUS constants.
Adds Volkswagen platform/config classes (MQB/PQ), VolkswagenCarSpecs, CAR enum entries for many VW/Skoda/Seat/Audi models, and documentation/footnotes.
Provides FW query configuration and a fuzzy match algorithm to map firmware responses & VIN to supported cars.
Supplies button mappings, LDW/ACC HUD codes, DBC handling, and FW-query constants.
Why
Adds comprehensive Volkswagen platform support including both MQB and PQ families, encapsulating vehicle-specific CAN message generation and safety mitigations.
Provides infrastructure for identifying VW group cars by firmware/VIN and for safely controlling steering, ACC and HUD on these platforms.
Summary by Sourcery
Add Volkswagen-specific car controller and platform definitions to support steering, longitudinal control, HUD, and firmware-based identification for MQB and PQ vehicles.
New Features:
Enhancements: