-
Notifications
You must be signed in to change notification settings - Fork 1.6k
VW MQB: work around standstill limit #2764
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: master
Are you sure you want to change the base?
Conversation
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.
Thanks for contributing to opendbc! 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
- include a route or your device' dongle ID if relevant
a9300c3
to
ce25ac1
Compare
opendbc/car/volkswagen/mqbcan.py
Outdated
acc_hold_type = 4 # hold release / startup | ||
elif esp_hold: | ||
acc_hold_type = 3 # hold standby | ||
acc_hold_type = 1 # hold request (at stop) |
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.
these were flipped from how stock ACC uses them. the car will still respect either value and stop, but sometimes the car is late to release ESP_confirmation (ESP_Haltebestaetigung) when transitioning directly from 3 -> 4.
on cars with EPB this is fine, but on cars without this can easily cause faults if the hold confirmation is retained for too long.
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.
I'll take a look when you're out-of-draft and ready for real review, but IIRC I got those field descriptions from an authoritative source back in the day. That's not to say we're for-sure using them correctly.
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.
after further testing, I don't think it matters like I thought it did. I'll probably leave it this way since its more "correct" but I am not entirely sure if the ESP on my car even uses this value
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 needs must, you're welcome to add FtS-specific conditional behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
8000The reason will be displayed to describe this comment to others. Learn more.
after further testing it doesn't seem to matter either way. left it in what I think is the more 'correct' orientation but happy to put this in a separate PR with the rest of the tuning stuff if you rather
acc_control = self.CCS.acc_control_value(CS.out.cruiseState.available, CS.out.accFaulted, longActive) | ||
accel = float(np.clip(actuators.accel, self.CCP.ACCEL_MIN, self.CCP.ACCEL_MAX) if longActive else 0) | ||
stopping = longActive and actuators.longControlState == LongCtrlState.stopping | ||
rollout_in_progress = CS.out.vEgo < self.CP.vEgoStopping and accel > 0 |
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 previous logic would often flip 'starting' to true when slowing to a stop, which is not what the car expects
I'm restricting 'starting' to only positive acceleration here
before merge the openpilot long controller will have to be adapted to work with PQ as well, the current implementation may not function properly with PQ vehicles running stock openpilot |
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.
Nice work finding a way to hold longer! I've never been happy with our (lack of) proper FtS support, and this might be an even better solution.
# reset standstill timer for MQB w/out EPB while braking | ||
# this mimics what would happen if the stock ACC intentionally rolled off after holding for too long | ||
# the key difference is that the stock ACC also enables freewheel (ACC_Freilauf_Info) during this time | ||
# but we leave it disabled to reset the timer without actually rolling off |
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 sure you have a car that actually supports freewheeling? This seems to be rare, potentially dry-clutch DSGs only. IIRC we've only had one show up, and IIRC they might be broken without passing through another flag.
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 sure you have a car that actually supports freewheeling?
I have no idea, really. But sending different values for ACC_Freilauf_Info
does have a measurable effect.
If I send 0 (freewheel allowed) the car will immediately start rolling. If we're facing uphill, the car will start rolling backward. If I send 2 (freewheel not allowed) the car will remain at a stop for a moment before rolling forward (assuming no brake request, ofc). It won't ever roll backwards, even if I send ACC disabled.
I haven't experimented much with 1 (No transition to coasting allowed) and 3 (Coasting requested) but plan to soon. The stock system sends 0 when forcing FTS disengagement.
opendbc/car/volkswagen/mqbcan.py
Outdated
acc_hold_type = 4 # hold release / startup | ||
elif esp_hold: | ||
acc_hold_type = 3 # hold standby | ||
acc_hold_type = 1 # hold request (at stop) |
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.
I'll take a look when you're out-of-draft and ready for real review, but IIRC I got those field descriptions from an authoritative source back in the day. That's not to say we're for-sure using them correctly.
I've been doing some fairly extensive tests on this over the last month or so to try and improve the behavior and reduce faults. The biggest problem right now is hills. If we're on a hill (specifically uphill), we have to send a longer 'start request' before the car will acknowledge. I'm looking for some kind of workaround, but I'm leaning toward just doing the best we can while also asking the user to press brake on steeper hills. (The dashboard has a 'press brake' alert we can use for that) |
You may find this challenging. One of the differences between FtS and SnG cars is bidirectional rear wheel speed sensors, for detecting when the wheel is rolling backwards, to protect against rollback on hill starts. Essentially the same problem you're having. With that info, I suspect the ESP is able to keep the car still until the powertrain develops enough force to overcome the hill. Same basic thing happens with manual trans hill-hold. I don't know how well FtS cars deal with this.
Not exactly Plan A, but we do have longitudinal pitch data available in carcontroller. I wouldn't trust the car's road-grade signal for this. |
b9282ce
to
b59f8bd
Compare
6446a85
to
599a184
Compare
599a184
to
10083c8
Compare
goal
MQB cars without an EPB (often referred to as 'ACC Low') generally will not hold at a standstill for more than a few seconds before faulting. This works around that fault to get full stop-n-go capability without an EPB.
why it works
Stock ACC will only hold at a stop for about 1 second before disengaging ACC and rolling. As stock ACC disengages, it sends a short burst of 'start request' to inform the car that it should start rolling. We copy this request with a few changes to reset the timer without moving.
0.01
. This resets the timer without any noticable effect, but only works on relatively flat ground. On hills, the start request must be longer than a single frame. We know the timer was reset ifesp_hold_confirmation
flips toFalse
.0.0
. The steeper the hill, the longer the request must be to reset the standstill timer. Sending a positive acceleration here would create torque, which isn't what we want.esp_hold_confirmation
but will not reset our timer.other related tweaks and notes
ACC_Anforderung_HMS
did not match the stock ACC when approaching a stop. When approaching a stop, stock ACC transitions 0 -> 3 -> 1. The previous logic would transition 0 -> 4 -> 1 -> 3. (possible states are 0 none, 1 hold, 2 park, 3 hold‑standby, 4 drive‑off, 5 ramp‑release)todo
Longitudinal Maneuvers Route (Outdated)
(has lots of standstill, great for seeing the new behavior)
a3d353f2254caee4
a3d353f2254caee4/00000016--544d9e5e69
Validation (Outdated)
a3d353f2254caee4
a3d353f2254caee4/0000000f--3a1c287356