Skip to content

VW: allow adjusting set speed when disabled#36758

Draft
RJWadley wants to merge 8 commits intocommaai:masterfrom
RJWadley:set-speed
Draft

VW: allow adjusting set speed when disabled#36758
RJWadley wants to merge 8 commits intocommaai:masterfrom
RJWadley:set-speed

Conversation

@RJWadley
Copy link
Copy Markdown

@RJWadley RJWadley commented Dec 3, 2025

goal: more closely align with stock ACC behavior on VW cars with independent speed buttons

some cars have +/- buttons that are separate. realistically for openpilot this just means VW, since most other affected cars use flexray, but it's not inherently a VW thing. KIA EV9 has independent buttons for example, but it still treats them as SET+ and SET- so they're not affected here.

Since it has independent buttons, VW's stock system allows adjustment of the set point while disabled. In most cars, the +/- buttons are shared with RES/SET and this is inherently not possible.

tweaks:

  • handle cruise +/- presses when disabled
  • skip if we don't have an ACC setpoint
  • tweak speed adjustment handling to ignore any button press that could enable openpilot
VW MQB KIA EV6
Screenshot 2025-12-02 at 10 05 32 PM Screenshot 2025-12-02 at 10 04 50 PM

Sample route showing adjustment while disabled:
a3d353f2254caee4/00000242--cb0f881531/0

@github-actions github-actions Bot added the car vehicle-specific label Dec 3, 2025
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@RJWadley RJWadley marked this pull request as draft December 3, 2025 07:42
@RJWadley RJWadley marked this pull request as ready for review December 4, 2025 07:49
@RJWadley RJWadley marked this pull request as draft December 4, 2025 07:50
@RJWadley RJWadley marked this pull request as ready for review December 4, 2025 08:45
@github-actions
Copy link
Copy Markdown
Contributor

This PR has had no activity for 24 days. It will be automatically closed in 7 days if there is no activity.

@github-actions github-actions Bot added the stale label Feb 12, 2026
@github-actions github-actions Bot removed the stale label Feb 17, 2026
@andiradulescu
Copy link
Copy Markdown
Contributor

andiradulescu commented Apr 10, 2026

One side effect on non-VW non-PCM brands worth considering:

Their base update_button_enable fires on +/- release, so those buttons double as enable triggers. With the if not enabled: return gone, the long-press branch at cruise.py:84-88 now ticks v_cruise_kph every 50 frames during a hold, even while disabled (as long as v_cruise_initialized). On + specifically, the drift survives the enable transition because initialize_v_cruise restores v_cruise_kph_last for accelCruise/resumeCruise. Net: hold + for 2s to re-engage → openpilot enables with setpoint +20 kph. Held - is fine, it gets clobbered by the else branch.

GM is worse, it enables on the rising edge of accelCruise (gm/carstate.py:34-41), so the drift happens live during the hold rather than all at once on release. The new CS.buttonEnable guard doesn't fire mid-hold, so it doesn't protect GM; the old button_change_states[...]["enabled"] guard did.

Since the feature is really "this car has independent set-speed buttons," I'd scope it to VW explicitly and keep the old guard intact for everyone else:

if not enabled and self.CP.brand != 'volkswagen':
  return

# ...

if not self.button_change_states[button_type]["enabled"] and self.CP.brand != 'volkswagen':
  return

Brand checks are already standard in this area (car_specific.py, latcontrol_angle.py, etc.), though cruise.py has none today. If you'd rather keep it brand-agnostic, a CarParams flag (hasIndependentSetSpeedButtons or similar) set by the VW interface would work too, at the cost of an opendbc capnp bump. The new CS.buttonEnable and not enabled_before_pressing guard can then be dropped - the restored old guard is strictly stronger.

On testing: this PR needs tests. I took a stab at coverage for the adjust-while-disabled paths on https://github.com/andiradulescu/openpilot/commits/set-speed/ - feel free to cherry-pick, fold in, or ignore. That branch also carries a latent precedence bug fix in test_rising_edge_enable, which I separately opened as #37802.

@RJWadley RJWadley marked this pull request as draft April 10, 2026 22:15
@github-actions
Copy link
Copy Markdown
Contributor

Process replay diff report

Replays driving segments through this PR and compares the behavior to master.
Please review any changes carefully to ensure they are expected.

✅ 0 changed, 66 passed, 0 errors

@RJWadley
Copy link
Copy Markdown
Author

thanks for the feedback @andiradulescu!

I hadn't considered how openpilot would behave while disabled during a button hold. I cooked up some tests to cover some of the missing cases and this branch does indeed fail in exactly the scenarios you've described! master as-is passes.

In the short term, a branch check is probably the simplest fix. I'd rather keep it generic but it's not possible with openpilot's button handling at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

car vehicle-specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants