Skip to content
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

Honda Radarless: Create&send buttons to camera when engaged. Keep stock LKAS off. #31022

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

csouers
Copy link
Contributor

@csouers csouers commented Jan 16, 2024

Description: Fix two issues with Honda Bosch Radarless + stock ACC


Issue # 1: Intermittent failure to cancel/auto-resume.

Cause: The stock camera firmware in radarless models locks out button commands when button messages are received at the wrong time/with a bad counter. A number of consistently valid incoming frames must be received before the lockout is released. I didn't find any kind of feedback bits from the camera indicating a lockout.

Attempted fixes that were unreliable/ineffective:

  • Spam quickly in between the car's frames with valid counters.
  • Spam just after or before the car's frame is sent/forwarded with the proper counter as well as any other data bytes in the frame that OP doesn't normally send.
  • Waiting a while and trying again after unsuccessful spamming (no counter matching on this). I didn't really like this anyway, so I quickly abandoned it.

Fix: Intercept the buttons frame at the panda when engaged then create & send to the camera at the normal rate - 25hz. Panda firmware forwards when disengaged. The cancel button is still spammed as before.


Issue # 2: Sudden ACC disengagement (Cruise is Off) after not touching the wheel for period of time.

Cause: When stock LKAS is enabled in the background, the stock wheel touch timer/nag expiring can disengage ACC. The alerts aren't forwarded by openpilot so the user is unaware of the issue. Users have also reported this issue saying that they never pushed the LKAS button, so there is a possibility that the car is pressing the LKAS button on its own for some reason. It doesn't happen consistently, so it's unclear what is really happening there.

Fixes: Send the LKAS button to turn off LKAS if LKAS_HUD from the camera shows that it is enabled. Also block pass-through of the LKAS button if pressed by the car/user when openpilot is enabled/intercepting to prevent the stock system from being turned on.


Broken route. Release3 : a918d2895b3210a1|2022-08-15--12-43-11

  • 89.3: user engages ACC
  • 90.2: car sends LKAS button. stock LKAS turns on
  • 102.2: user has been hands off too long. stock LKAS turns on the visual nag (not forwarded, so not seen)
  • 112.2: longer still. stock LKAS now turns on the auditory nag (not forwarded, so not heard)
  • 118.1: after ~30 seconds since the user has last touched wheel (~86 seconds) and has also "ignored" the nag, ACC finally disengages

Verification: Tested by two users with 2022+ Honda Civic Sedan and also a Hatchback. One user has been driving on this fix for at least 9 months with no issues or recurrences of these two issues.

Good route from this PR : a918d2895b3210a1|2024-01-27--10-03-34

  • 28.2 : user presses LKAS button. Panda is forwarding, so stock LKAS turns on.
  • 30.0: user engages ACC.
  • 30.4: openpilot engages and presses the LKAS button, which successfully turns off LKAS.
  • 36.9: user presses LKAS button again as a test, which is successfully blocked by openpilot.

Another good route that's more recent: a918d2895b3210a1|2024-02-23--19-10-17

Prereqs:

Todo:

  • test on new HR-V? Should be fine anyway as the sent button frames are otherwise typical.
  • test route
  • broken route example
  • add updated civic2022 route to routes.py

@csouers
Copy link
Contributor Author

csouers commented Jan 16, 2024

Should have good test routes ready to go in a couple of days.

@csouers
Copy link
Contributor Author

csouers commented Jan 16, 2024

This has some similar changes to #30850, such as the dicts for the two button signals. @sunnyhaibin, let me know if you want attribution.

@csouers csouers changed the title Honda Radarless: Send buttons to camera when engaged vs forwarding/spamming. Honda Radarless: Create&send buttons to the stock camera when engaged. Jan 16, 2024
@csouers csouers changed the title Honda Radarless: Create&send buttons to the stock camera when engaged. Honda Radarless: Create&send buttons to the stock camera when engaged. Keep stock LKAS disabled. Jan 17, 2024
Copy link
Contributor

It looks like you didn't use on of the Pull Request templates. Please check the contributing docs. Also make sure that you didn't modify any of the checkboxes or headings within the template.

… spamming.

Co-authored-by: Jason Wen <47793918+sunnyhaibin@users.noreply.github.com>
Copy link
Contributor

It looks like you didn't use one of the Pull Request templates. Please check the contributing docs. Also make sure that you didn't modify any of the checkboxes or headings within the template.

@csouers csouers changed the title Honda Radarless: Create&send buttons to the stock camera when engaged. Keep stock LKAS disabled. Honda Radarless: Create&send buttons to camera when engaged. Keep stock LKAS off. Feb 22, 2024
@github-actions github-actions bot added the car vehicle-specific label Feb 22, 2024
Copy link
Contributor

github-actions bot commented Mar 21, 2024

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

@Saturntime
Copy link

I’m having issue 2 I believe going to post route here in a few days

@vanillagorillaa
Copy link
Contributor

Can confirm this fix works with my Acura Integra branch #32056

@vanillagorillaa vanillagorillaa mentioned this pull request Apr 27, 2024
9 tasks
@MarcoTheDingo
Copy link
Contributor

Hey @csouers ! I occasionally have Issue 1. Do you still need a route, and if so, are you looking for something specific to test? I have a 2022 Civic Hatchback, Sport Touring Trim.

@MarcoTheDingo
Copy link
Contributor

Hey @csouers it looks like the latest commit causes CAN errors and also the LKAS warning light lights on my dash. Routes:

  • 5130484aa8069bad/00000093--d9777caa5a
  • 5130484aa8069bad/00000094--0b89a3ca92

@csouers
Copy link
Contributor Author

csouers commented May 1, 2024

Hey @csouers it looks like the latest commit causes CAN errors and also the LKAS warning light lights on my dash. Routes:

  • 5130484aa8069bad/00000093--d9777caa5a

  • 5130484aa8069bad/00000094--0b89a3ca92

Did you checkout the two unmerged submodule PRs listed in the OP too?

@sshane
Copy link
Contributor

sshane commented May 22, 2024

I see CI failures. I would rather merge a change without a safety change first. Are you sure we can't improve the receptiveness of our button message without forwarding? I see we don't already set the counter to the one from the car, that could be a problem currently.

Issue 2 is only an issue for stock ACC, correct? We should be able to stay engaged as we manage that with alpha openpilot long.

We should split these two out into separate PRs, that should make things easier to review.

Comment on lines +170 to +171
'ENABLED': 1,
'SET_ME_X20': 0x20,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this maintain both MSB 30 and LDW_OFF at MSB 27?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are still just for LKAS. I will rename ENABLED.

@@ -167,7 +167,8 @@ def create_ui_commands(packer, CAN, CP, enabled, pcm_speed, hud, is_metric, acc_
commands.append(packer.make_can_msg("ACC_HUD", CAN.pt, acc_hud_values))

lkas_hud_values = {
'SET_ME_X41': 0x41,
'ENABLED': 1,
Copy link
Contributor

@sshane sshane May 22, 2024

Choose a reason for hiding this comment

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

Can you rename enabled to something more clear? We have LDW_ON/OFF in the same message. Is ENABLED meant to be LKAS active or just enabled?

Copy link
Contributor Author

@csouers csouers May 23, 2024

Choose a reason for hiding this comment

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

Just enabled. available or ready might be a good alternative.

@csouers
Copy link
Contributor Author

csouers commented May 23, 2024

I see CI failures. I would rather merge a change without a safety change first. Are you sure we can't improve the receptiveness of our button message without forwarding? I see we don't already set the counter to the one from the car, that could be a problem currently.

The camera fw seems to enforce a brief window for incoming frames. If the camera detects that something isn't being received as/when expected, it locks out button commands until the "issue" clears up for a brief period.

As for what I've tried to make spamming better, my counter setting trials didn't improve spamming at all. Of the 4 or so methods I attempted, the two I can recall right now are:

  1. Synchronizing to the car's button counter each time it was received by OP along with incrementing the counter.
  2. Along with synchronizing the counter, I tested sending the spammed frames just ahead of when the car was expected to transmit its next frame. I hoped that the camera would accept our message instead of the car's, but it didn't. Intercepting when OP is engaged is the only thing that has been rock solid.

Issue 2 is only an issue for stock ACC, correct? We should be able to stay engaged as we manage that with alpha openpilot long.

Yes, this entire PR only applies to stock ACC.

We should split these two out into separate PRs, that should make things easier to review.

Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car vehicle-specific in-bot-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants