Bounty: 2016 Honda CR-V Touring #102

energee · 2017-06-08T06:12:56Z

This port has been worked on as a team by myself and @johnnwvs. This code adheres to the safety rules. What we have found is that the CR-V most closely resembles the ILX's CAN messaging and firmware. The biggest difference in this car from other Hondas is that the steering message, aside from having a different message ID, is 12 bits instead of 16. The same goes for the steering torque sensor signal on 0x18F.

The steering does need more work before this is ready to be upstreamed into the release branch, but realistically, that is probably a 1 or 2 line code change.

Although we felt it might be safer (for securing the port bounty) to get the steering more precise before making this code public, that contradicts the purpose of open source code. The bounty was created to incentivize contribution back to the community, but in some ways has also encouraged secrecy and a lack of collaboration. We think the majority of the comma developer community would agree.

Please note that this code should be used with caution until approved by the Comma team.

geohot · 2017-06-08T23:24:04Z

Congrats on the first submission!

With the bounty claim, as stated on the bounty page, you must have your code do 6 minutes of driving in stop and go traffic and prove it to us. Video proof is best! I'm not sure if the CR-V can do stop and go, in which case a good justification of why it can't and 6 minutes of no disengagement no steering diverse driving is acceptable.

(I added a note about video proof of this claim to the bounty page)

energee · 2017-06-09T01:31:24Z

Thanks george, john will work on the video

energee · 2017-06-09T16:07:21Z

This should be sufficient justification as to why CR-V can't do stop and go. Same limitation as ILX, 25+ ACC See: http://owners.honda.com/utility/download?path=/static/pdfs/2015/CR-V/2015_CR-V_Adaptive_Cruise_Control.pdf

geohot · 2017-06-09T18:09:16Z

selfdrive/car/honda/interface.py

@@ -181,7 +181,8 @@ def update(self):
      errors.append('steerTemporarilyUnavailable')
    if self.CS.brake_error:
      errors.append('brakeUnavailable')
-    if not self.CS.gear_shifter_valid:
+    # crvtodo: fix gearbox read.


Must fix before upstream. I'm sure it's really simple.

I will implement all the code changes.
Thanks for your help.

geohot · 2017-06-09T18:10:41Z

selfdrive/car/honda/carcontroller.py

@@ -180,7 +184,8 @@ def update(self, sendcan, enabled, CS, frame, final_gas, final_brake, final_stee
      print "STEER ERROR"
      self.controls_allowed = False

-    if CS.brake_error:
+    # crvtodo, fix brake error, might be issue with dbc.


Must fix before upstream. Having brake error working is safety critical, since we never want the system to be engaged with the car incapable of hitting the brakes.

In order to avoid brake_error on the Civic, the NEO has to send messages right on car startup. Could this be causing the issue?

This can be reverted, george's assumption appears to be correct

geohot · 2017-06-09T18:11:18Z

selfdrive/car/honda/carcontroller.py

@@ -140,12 +140,16 @@ def update(self, sendcan, enabled, CS, frame, final_gas, final_brake, final_stee
    tt = sec_since_boot()
    GAS_MAX = 1004
    BRAKE_MAX = 1024/4
-    STEER_MAX = 0xF00
+    if CS.crv:
+      STEER_MAX = 0xFFF/2


What is the actual max value? What happens if you exceed it? STEER_ERROR? I don't like this integer division.

If you are using this to have it steer less hard, you probably want instead to change the Kp and Ki for steering.

I was changing a lot of variables at once during initial testing, but 0x7FF seemed to be too high. The system is working well at 0x320 now, but I will start to increase it and do more testing over time to determine the real max value.

The issue was that the system would just give up on steering for a while and let me drift right off the road. I printed out some debug information and while it was drifting, I was getting apply_steer values of either 0, -2047 or +2048. Once the values showed something other than those three, it would steer again. Lowering down STEER_MAX down to 0x320 seemed to clear this up, but it is possible that other changes could account for it as well. More testing with fewer changes would help me clear this up.

Oh, yea, you should look in your CAN logs at the max value the stock system sent and not exceed that. It probably stopped steering and threw an error when you exceed MAX.

The max we had in the oem log was 0xFFF0, only 12 bits. John should have a log.

geohot · 2017-06-09T18:12:24Z

Okay, we accept no stop and go on 2015/2016. Looks like 2017 can do it, but not the older ones.

What's the minimum steering speed? Is there a minimum braking speed?

Get a good safe no intervention 6 minute video, address code review comments, we will rent the car and test, then we should be good!

geohot · 2017-06-09T18:42:31Z

A note also, for safety, add the 0x194 message to https://github.com/commaai/panda/blob/master/board/honda_safety.h

You want the board to not allow torque when the system is not engaged.

johnnwvs · 2017-06-09T21:34:45Z

What's the minimum steering speed? Is there a minimum braking speed?

I believe the minimum speed to start everything is 25MPH, but it seems to allow the car to go down to 20MPH if someone in front of me slows down below 25MPH. Once they speed back up, the car will continue back to it's set speed. If the car slows down below 20MPH, self drive will be cut off and I have to manually set the cruise again. I am not sure if it continues steering down to 20MPH when this happens.

The car will brake all the way down to a stop and hold the brake until the lead car moves again. At that point it will release the brakes but it will only idle forward, so it must be brought up to speed and set manually again.

geohot · 2017-06-09T22:08:25Z

Yea, it's nice the brakes work all the way to zero. Some cars just totally disengage at a set cut off (like the Ford Fusion). Wonder how many rear endings that caused.

Steering limitation is fine, matches ILX.

… ki/kp changes

johnnwvs · 2017-06-12T01:36:32Z

I am posting a new video with an over a 6-minute drive:
https://youtu.be/HZBZ0On9hK8

And I committed the changes to energee:devel-crv

Thanks for the help.

John

geohot · 2017-06-12T01:49:33Z

Given the video, I'll consider this bounty locked for a week while we upstream. Reflected on website, http://comma.ai/bounties.html

This changes the sat_count_limit globally. This shouldn't be raised, rather the max torque should be raised if possible.

You also still have to address both TODOs and the board safety.

johnnwvs · 2017-06-12T02:06:30Z

I can try it.
Do you raise max torque with higher kp/ki values?

geohot · 2017-06-12T05:08:52Z

No, max torque is STEER_MAX, raise that, then you proportionally lower Kp and Ki to match the behavior.

johnnwvs · 2017-06-12T16:15:28Z

George, I will work on this. I have a question as well about a feature that might really not be needed anyway, it might just feel that it is needed because my CR-V is still being tuned. I am interested in adding this feature, so not asking for other’s to add it. Would it be possible to add some code to adjust where the car is in the lane by taking the driver’s feedback into account? Let’s say the car is going down the road (not during a turn, so the final_steer is very slight) and the driver is giving “helpful tugs” on the wheel every so often. Is there a way we can take these “helpful tugs” on a straight road into account, and assume that the driver would like to see the car maintain a line that is a little more either right or left in the lane? Perhaps move it over some small amount after each tug on the wheel when the car is going straight? I find myself doing this every so often when, for instance, I am too close to the curb, or too close to the double yellow lines, or possibly just running down a very long line of pot-holes, or trying to avoid a turn-off lane that just opened up and I just want the car to be a little more to one side or the other of the lane. Or would this potentially mess something else up?

energee · 2017-06-13T13:59:06Z

commaai/panda#7 Safety code

pinglianghan · 2017-06-16T04:38:29Z

selfdrive/car/honda/carstate.py

@@ -181,6 +232,8 @@ def __init__(self, CP, logcan):
    elif CP.carFingerprint == "HONDA ACCORD 2016 TOURING":
      # fake civic
      self.accord = True
+    elif CP.carFingerprint == "HONDA CR-V 2016 TOURING":
+      self.crv = True


do you need to init it with False ahead, like line #175 - line #176

Yes, good call. @energee @johnnwvs Please fix.

pinglianghan · 2017-06-16T04:41:16Z

common/fingerprints.py

@@ -12,6 +12,11 @@
  },
  "HONDA ACCORD 2016 TOURING": {
    1024L: 5, 929L: 8, 1027L: 5, 773L: 7, 1601L: 8, 777L: 8, 1036L: 8, 398L: 3, 1039L: 8, 401L: 8, 145L: 8, 1424L: 5, 660L: 8, 661L: 4, 918L: 7, 985L: 3, 923L: 2, 542L: 7, 927L: 8, 800L: 8, 545L: 4, 420L: 8, 422L: 8, 808L: 8, 426L: 8, 1029L: 8, 432L: 7, 57L: 3, 316L: 8, 829L: 5, 1600L: 5, 1089L: 8, 1057L: 5, 780L: 8, 1088L: 8, 464L: 8, 1108L: 8, 597L: 8, 342L: 6, 983L: 8, 344L: 8, 804L: 8, 476L: 4, 1296L: 3, 891L: 8, 1125L: 8, 487L: 4, 892L: 8, 490L: 8, 871L: 8, 1064L: 7, 882L: 2, 884L: 8, 506L: 8, 507L: 1, 380L: 8, 1365L: 5
+  },
+  "HONDA CR-V 2016 TOURING": {


@geohot , is it better to replace the space with other delimiters(like '_' or "-")? It would be prone to typo error to use space here.

or use enum to replace string.

pinglianghan · 2017-06-16T04:43:18Z

dbcs/honda_crv_touring_2016_can.dbc

@@ -0,0 +1,312 @@
+VERSION ""


is there any doc to explain the format of this file? I would like to learn it. Thanks!

There's surprisingly little public information about it, but there's a library in openpilot to read it and it is the industry standard.

I've been meaning add some docs to the comma wiki about it. Will try to get to it.

https://wenku.baidu.com/view/41c36d25ed630b1c59eeb534.html

I believe it used to be an official/internal document that leaked out, but it's dated from 2007. This document has one an error thought regarding the endianness encoding, the rest seem ok.

pinglianghan · 2017-06-16T05:08:10Z

selfdrive/car/honda/hondacan.py

@@ -94,14 +100,15 @@ def create_ui_commands(pcm_speed, hud, civic, accord, idx):
    commands.append(make_can_msg(0x39f, msg_0x39f, idx, 0))
  return commands

-def create_radar_commands(v_ego, civic, accord, idx):
+def create_radar_commands(v_ego, civic, accord, crv, idx):


the signature will swell when more and more cars are supported. It's better to define a struct/class for the different type of cars.

Ugh yea, the honda interface code is the lowest quality. Would love a refactor pull request.

pinglianghan · 2017-06-16T05:10:59Z

selfdrive/car/honda/hondacan.py

  msg_0x300 = ("\xf9" + speed + "\x8a\xd0" +\
-              ("\x20" if idx == 0 or idx == 3 else "\x00") +\
-                "\x00\x00")
+                  ("\x20" if idx == 0 or idx == 3 else "\x00") +\


It's better to define the variables to replace such magic numbers(\x20). It can help to improve the readability and encourage more people to contribute to this revolutionary project.

geohot · 2017-06-19T22:01:43Z

Still waiting on brake error and gearbox fixes before test.

johnwaynehacker · 2017-06-20T03:30:29Z

for the gear error, do you need to know is P R N D or gear 0 1 2 3 4 5 6 for cvt? Actual or target gear?

regarding brake error : is the brake error command from neos or the actual vsa/abs brake pressure error value in reading

regarding car_gas, do you need the info with cruise control command?

geohot · 2017-06-20T04:06:23Z

gear error is just to confirm it's in D

brake error is from the brake controller

car_gas is needed in CarState but not CarController.

energee · 2017-06-20T06:55:02Z

All changes / fixes complete, @johnnwvs Run a test to confirm?

btw, apologies for the wait on these updates, was vacationing abroad without a computer for the last week or so, had to write the panda code through Github UI from my cell 😆

johnnwvs · 2017-06-20T15:38:43Z

I was on vacation as well, but I did have access to a computer and VERY poor hotel wifi. :)

I tested the latest code this morning and the safety changes were working great on my short commute. I will take a longer test drive after work and write back with the results.

johnnwvs · 2017-06-21T03:01:35Z

After the latest code changes all the safety features are back in and the Honda CR-V is working well with these latest changes. I uploaded a video of the latest test:
https://youtu.be/6IW7Nejsr3A

With these latest changes, we believe the port is complete.

johnwaynehacker · 2017-06-21T23:32:34Z

curious, when you port it on the neos, do you need to create .pyc file? python -m compileall

geohot · 2017-06-22T18:52:34Z

Ok, this is looking good to merge. We still haven't tested it on the car, but due to the similarities with the ILX and a look over the CAN data you've sent back, we are calling it complete! Congrats on claiming the first comma.ai bounty!

My understanding is you'd like to split it? E-mail viv@comma.ai with how you'd like to be paid, we can do wire transfer, check, bitcoin, or ethereum.

2253dd3 fix volt ign detect 3b299d7 add ignition and refactor af9af6d Merge pull request #110 from Jamezz/volt 13e850e more correct f295063 add new define to tests fec9758 gate that with debug 5516ebf one more ifdef cac7b31 only panda has float 938d474 fpu enable ffbf0c7 cleaner de30f27 Revert "need f to not be double" 4142acf need f to not be double 3eb15c8 refactor to share code a4c8b64 change to O2 to fix make recover 711fd11 Enable compiler optimizations, fix things it breaks 2e6f774 block IPAS in main toyota safety mode e7a2b3a add ipas tests 894572c fix tests 367c9ad add safety toyota ipas 95919b9 Bounty: panda high quality CAN autobaud (#96) 6557cd2 Toyota Safety: allow controls only on rising edge of cruise_engaged 02c1ddf Revert "added steer override check when IPAS is in control (#106)" 9f925ba Fix the merge mess 23d3833 Merge from comma upstream a0cc51a Undo safety mode override ea1c1dc make wlan interface name generic 6dbd8c9 Implement WebUSB and upgrade WinUSB to 2.0 (#107) 4fc83a5 Add safety hook for ignition and have GM use gear selector to determine ignition 52b2ac0 switch from travis to circleci 48e2374 build panda esp image 065572a circleci build stm image 7a1f319 add panda python package test and fix safety test 021dde7 move saftey test helper files into safety folder ce0545f add ci files 6a3307c no LIN over ELM 7d21acb added steer override check when IPAS is in control (#106) 1c88caf Safety code testing (#104) f4efd1f Merge pull request #101 from adhintz/master c02618b Merge pull request #102 from quillford/master 1ba5f8a added link to wiki for user scripts de2b19e add support for multiple buses to can_unique and can_bittransition output data in sorted order. git-subtree-dir: panda git-subtree-split: 2253dd3

Refactor time gap calculation

added link to wiki for user scripts

Bounty: 2016 Honda CR-V Touring

Alternative Map Styles

Adds support for 2016 Honda CR-V Touring

7b5ee81

energee mentioned this pull request Jun 8, 2017

2015-2016 CR-V Touring #98

Closed

Syntax typo

0bb75c5

energee changed the base branch from master to devel June 8, 2017 07:52

Reverting steering to original value

615db3f

geohot reviewed Jun 9, 2017

View reviewed changes

johnnwvs added 2 commits June 11, 2017 13:20

adding steering change from video and removing integer div and adding…

942655c

… ki/kp changes

missed one update in latcontrol

6e824a2

pinglianghan reviewed Jun 16, 2017

View reviewed changes

energee added 3 commits June 20, 2017 02:26

Init crv variable

dbf71a2

Adds correct value of 8 to gearshifter check array

65134be

Fixes global saturation change

97eb55c

Addresses brake error review comment

e40c161

geohot merged commit 6fee0bd into commaai:devel Jun 22, 2017

mespiritu pushed a commit to mespiritu/openpilot that referenced this pull request Mar 14, 2019

Merge pull request commaai#102 from rhinodavid/arne_refactor

942aca6

Refactor time gap calculation

avolmensky pushed a commit to avolmensky/openpilot that referenced this pull request Jun 30, 2019

Merge pull request commaai#102 from quillford/master

c02618b

added link to wiki for user scripts

dragonpilot referenced this pull request in dragonpilot-community/dragonpilot Apr 7, 2020

Merge pull request #102 from energee/devel-crv

3e4854e

Bounty: 2016 Honda CR-V Touring

weimou pushed a commit to SFtoLA/openpilot that referenced this pull request May 6, 2021

Add HW types (commaai#102)

2594b4d

MoreTore pushed a commit to MoreTore/openpilot that referenced this pull request Mar 2, 2024

Merge pull request commaai#102 from mike86437/map-styles

0ee5293

Alternative Map Styles

carleeno pushed a commit to carleeno/openpilot that referenced this pull request Aug 2, 2024

ui: move Error Troubleshoot to Device menu (commaai#102)

5d3caec

Kirito3481 added a commit to Kirito3481/openpilot that referenced this pull request Dec 6, 2024

Hyundai: Added Ioniq 5 N car port (commaai#102)

7ffee72

Bounty: 2016 Honda CR-V Touring #102

Bounty: 2016 Honda CR-V Touring #102

Conversation

energee commented Jun 8, 2017 • edited Loading

geohot commented Jun 8, 2017 • edited Loading

energee commented Jun 9, 2017

energee commented Jun 9, 2017

geohot Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geohot Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geohot Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geohot commented Jun 9, 2017 • edited Loading

geohot commented Jun 9, 2017 • edited Loading

johnnwvs commented Jun 9, 2017

geohot commented Jun 9, 2017

johnnwvs commented Jun 12, 2017

geohot commented Jun 12, 2017 • edited Loading

johnnwvs commented Jun 12, 2017

geohot commented Jun 12, 2017

johnnwvs commented Jun 12, 2017 via email • edited Loading

energee commented Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geohot commented Jun 19, 2017

johnwaynehacker commented Jun 20, 2017

geohot commented Jun 20, 2017

energee commented Jun 20, 2017 • edited Loading

johnnwvs commented Jun 20, 2017

johnnwvs commented Jun 21, 2017

johnwaynehacker commented Jun 21, 2017

geohot commented Jun 22, 2017 • edited Loading

energee commented Jun 8, 2017 •

edited

Loading

geohot commented Jun 8, 2017 •

edited

Loading

geohot Jun 9, 2017 •

edited

Loading

geohot Jun 9, 2017 •

edited

Loading

geohot Jun 9, 2017 •

edited

Loading

geohot commented Jun 9, 2017 •

edited

Loading

geohot commented Jun 9, 2017 •

edited

Loading

geohot commented Jun 12, 2017 •

edited

Loading

johnnwvs commented Jun 12, 2017 via email •

edited

Loading

energee commented Jun 13, 2017 •

edited

Loading

energee commented Jun 20, 2017 •

edited

Loading

geohot commented Jun 22, 2017 •

edited

Loading