martinl / openpilot

Open source driving agent - Subaru fork
https://github.com/martinl/openpilot/wiki/
MIT License
70 stars 72 forks source link

3071 limits for early 2018 Crosstrek EPS b'z\xc0\x0 {0, 4, 8} \x00' #71

Closed ClockeNessMnstr closed 2 years ago

ClockeNessMnstr commented 2 years ago

Max steer is 3071 on b'z\xc0\x04\x00'

I believe this was the max value accepted by the EPS during testing a while back.

confirmed with Budney that this is working with the same EPS. He had the same concerns about drifting behavior. His EPS was a match to mine and accepted the higher values as well.

with the change to 3071 the torque is still less capable than the 2019 forester which does not accept higher values than 2047

ClockeNessMnstr commented 2 years ago

not sure what to do about overlap between 2018 Crosstrek and other imp/cross for esp, fwdCamera, engine, transmission. Tried restructuring to not explicitly duplicate. Copies Impreza but replaces EPS values with known 3071 fw

ClockeNessMnstr commented 2 years ago

Prlifestyle's eps is b'z\xc0\x00\x00', Looks like it could be the same level of firmware functionally but from the Impreza instead of the Crosstrek.

It sounds like he's having a similar torque issue but I sent him a branch with this PR and his fw and he got a lkas error. Need to get out and test this PR on my car to make sure I didn't miss anything from this change my main branch and retest with prlifestyle

ClockeNessMnstr commented 2 years ago

OP is fingerprinting correctly but panda isn't happy for some reason. Going to check logs and compare to my working code. I might have broke it merging with the "2020 limits" taken out of panda.

ClockeNessMnstr commented 2 years ago

Moody/mood killer has b'z\xc0\x08\x00 eps

Going to test that as well as prlifestyle's again

ClockeNessMnstr commented 2 years ago

Duplicated the lower safety check outside of the conditional.

Should be all set now. Prlifestyle and Moody are going to test on their eps and I'll check this branch on mine.

ClockeNessMnstr commented 2 years ago

added Moody's and prlifestyles eps. both are working.

These are the eps so far

  b'z\xc0\x00\x00',
  b'z\xc0\x04\x00',
  b'z\xc0\x08\x00',
ClockeNessMnstr commented 2 years ago

Went back and tested 4095 as a sanity check. Gave me lkas fault. 3071 should be the EPS limit on these.

michaelhonan commented 2 years ago

Not sure if this helps, but I have z\xc0\x08\x00 as well, and the 4095 3071 value works great.

Subaru Impreza 2018

ClockeNessMnstr commented 2 years ago

Not sure if this helps, but I have z\xc0\x08\x00 as well, and the 4095 value works great.

Subaru Impreza 2018

I can only find 3071 on your fork. 4095 gave me a lane keep assist error right after it went above around 3100-3200 or so. Do you have a route you can make public with the higher value?

michaelhonan commented 2 years ago

Apologies, I mistook 4095 for the new 3071 value you found. Corrected my comment.

ClockeNessMnstr commented 2 years ago

Had me jealous and double (quintuple) checking for a minute haha. I'm glad it's not more complicated though lol

ClockeNessMnstr commented 2 years ago

Screenshot from 2022-01-11 19-58-07

here is the failure. 3 messages after 3071 is exeeded the eps shuts out LKAS

martinl commented 2 years ago

I would recommend testing if the new tuning values also work without increasing the max steering torque. Increasing the max steering torque would require revalidating panda safety tests (using joystick to inject max steer value).

ClockeNessMnstr commented 2 years ago

I would recommend testing if the new tuning values also work without increasing the max steering torque. Increasing the max steering torque would require revalidating panda safety tests (using joystick to inject max steer value).

We could re-scale the torque with a different tune x1.5 but that doesn't fix the max torque / turning capability being considerably low. It just can't take some turns that it should be able to.

I have a log from last night with a couple max steering exceeded on turns the Forester can make at those speeds. From comparing the two the max torque is safer than the forester as it produces lower lateral acceleration at max.
The max slew rate is the same as it was so that reaction time to counter torque/sec remains the same.

I can set up my joystick again though if we still want anything specific logged.

ClockeNessMnstr commented 2 years ago

It also hits max torque on highways without the fix which was a major disappointment to me in OP before testing the new values.

ClockeNessMnstr commented 2 years ago

Need to enforce the EPS match for Impreza versus the new FP.

Unit test showing multiple fingerprints if the EPS is missing. (Or if fuzzy just grabs both anyway)

martinl commented 2 years ago

I reviewed one drive from prlifestyle93 with max_steer 3071 (eps-test branch) and it increased the steering torque output beyond to what stock openpilot currently allows:

Screenshot 2022-01-14 at 08 22 35

Here is one stock drive with max_steer 2047 (subaru-community branch):

Screenshot 2022-01-14 at 09 00 52

Allowing higher steer_max also increases eps motor torque output and max steering angle at given speed, so there is currently no evidence that some Impreza/Crosstrek eps firmwares have lower torque request scaling factor and higher max_steer limit does not increase output steering torque.

I would recommend opening panda safety max torque request increase PR against upstream panda and work with comma to do the appropriate torque request injection tests. I can merge it with new tuning values once it is upstreamed.

In general, I think the higher torque request limit would be useful for all models to take a few more sharp turns, if it passes the safety tests.

You can also take a look how toyota does setting eps firmware specific tuning values in interface, this looks a bit cleaner than current implementation: https://github.com/commaai/openpilot/blob/master/selfdrive/car/toyota/interface.py#L117

ClockeNessMnstr commented 2 years ago

Sounds good! I'll set that up. Thanks Martin!

ClockeNessMnstr commented 2 years ago

Edit: nvm, saw your comment on discord.

It looks like you may have nudged the wheel slightly at 95sec However, it does look like 2047 was achieving 22-25deg right before that possibly?

What was the desired output angles?

ClockeNessMnstr commented 2 years ago

moved to: https://github.com/commaai/openpilot/pull/23530

martinl commented 2 years ago

Yes, around 95s it needed some driver assistance to make the turn. As you can see, eps torque output is reducing and steering angle is increasing at the same time.

At the measured point, desired steering angle was -24.1

Screenshot 2022-01-14 at 17 06 28
martinl commented 2 years ago

I asked prlifestyle93 to do stock subaru-community drive on the same route as eps-test above to make things more comparable

ClockeNessMnstr commented 2 years ago

Sounds good, if that turn holds max 3071 for a sec it should tapout pretty early with 2047.

On Fri, Jan 14, 2022, 10:19 AM martinl @.***> wrote:

I asked prlifestyle93 to do stock subaru-community drive on the same route as eps-test above to make things more comparable

— Reply to this email directly, view it on GitHub https://github.com/martinl/openpilot/pull/71#issuecomment-1013217036, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSYPJEZ5F74BZABSKJTCTDUWA5IJANCNFSM5J74BEKA . You are receiving this because you authored the thread.Message ID: @.***>

martinl commented 2 years ago

prlifestyle93 impreza eps with 2047 limit can do about 20 degrees at 30mph, same as my crosstrek:

Screenshot 2022-01-15 at 07 28 33

I do not see any significant difference in steering torque and angle performance with that eps fw that would require increasing the torque request limit for highway driving.