-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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. #31022
Conversation
Should have good test routes ready to go in a couple of days. |
This has some similar changes to #30850, such as the dicts for the two button signals. @sunnyhaibin, let me know if you want attribution. |
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>
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. |
Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:
|
I’m having issue 2 I believe going to post route here in a few days |
Can confirm this fix works with my Acura Integra branch #32056 |
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. |
Hey @csouers it looks like the latest commit causes CAN errors and also the LKAS warning light lights on my dash. Routes:
|
Did you checkout the two unmerged submodule PRs listed in the OP too? |
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. |
selfdrive/car/honda/hondacan.py
Outdated
'ENABLED': 1, | ||
'SET_ME_X20': 0x20, |
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.
Does this maintain both MSB 30 and LDW_OFF
at MSB 27?
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 are still just for LKAS. I will rename ENABLED
.
selfdrive/car/honda/hondacan.py
Outdated
@@ -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, |
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.
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?
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.
Just enabled. available
or ready
might be a good alternative.
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:
Yes, this entire PR only applies to stock ACC.
Will do. |
We've moved the car interfacing code to our PR_NUMBER=33045
curl -L https://github.com/commaai/openpilot/pull/$PR_NUMBER.patch | sed -e 's/selfdrive\/car/opendbc_repo\/opendbc\/car/g' | git apply -v --reject Simply replace the PR number with your own. Once done, add the files, fix any conflicts, and open a new PR. Alternatively, you may start a new PR from scratch if that is easier for you. |
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:
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
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
Another good route that's more recent:
a918d2895b3210a1|2024-02-23--19-10-17
Prereqs:
Todo: