search

commaai / openpilot

openpilot is an operating system for robotics. Currently, it upgrades the driver assistance system on 275+ supported cars.
https://comma.ai/openpilot
MIT License
49.91k stars 9.1k forks source link

Set missing car minimum enable/steer speeds in CarParams #24121

Closed sshane closed 7 months ago

sshane commented 2 years ago

Some cars don't have any minimum enable or steer speeds defined in their CarParams as we expect the car to handle this for us if we're pcmCruise. We should find the correct speeds and add them to CarParams so we can eventually remove these exceptions for the car docs: https://github.com/commaai/openpilot/blob/e40c36f22ba93200a5fc83211005b28fca3fa728/selfdrive/car/docs_definitions.py#L55

Draft PRs to work off of:

abesisu commented 2 years ago

Hi there @sshane, I'm a student working on a class at Iowa State University where we choose an open-source project to analyze and contribute to. If I were to try and contribute to this issue, how might I find the correct missing minimum enable/steer speeds that need to be put in?

sshane commented 2 years ago

Hey! Since you don't have access to many cars, or the data that they upload to us, I would try to ask around in the Discord for users to upload segments to you and analyze when the stock system stops applying torque, if they have an EPS torque signal.

Some cars may be easier to fix, the Accord for example has the same minimum steering speed for all cars under that platform, but the speed is just not defined in the CarParams: https://github.com/commaai/openpilot/blob/master/selfdrive/car/honda/values.py#L112

For these cases, you can find CarInfo entries where the speeds are all the same, and then just move those speeds into the interface file: https://github.com/commaai/openpilot/blob/master/selfdrive/car/honda/interface.py#L117

Then I would ask these owners or get a segment so you can verify 3 mph is correct in this case

abesisu commented 2 years ago

@sshane is there anything I'm doing wrong in this PR that would lead these checks to fail? I haven't changed CarInfo and I've only added to CarParams. I originally tried to remove the min_steer_speed from values.py, but the test I'm failing said the minSteerSpeed should be in both CarParams and CarInfo.

jyoung8607 commented 2 years ago

@sshane is there anything I'm doing wrong in this PR that would lead these checks to fail? I haven't changed CarInfo and I've only added to CarParams. I originally tried to remove the min_steer_speed from values.py, but the test I'm failing said the minSteerSpeed should be in both CarParams and CarInfo.

The unit test is complaining because both are set; they shouldn't be. The CarInfo setting should only be used if there's some reason they aren't (or can't) be set in CarParams; now that you have set them, they should be removed from CarInfo.

The process replay test is mainly a speed bump designed to make sure openpilot behaves the same before and after any given change, and if it doesn't, that someone examines the difference and confirms that it's expected and desired. In your case, it detects the CarParams change. Assuming the change has no ill effects in real world testing on those cars, a comma employee would review that difference and update the replay baseline to include them as part of merging the PR.

abesisu commented 2 years ago

@jyoung8607 do you know of a way that I can get the unit test to pass? If what I've done is correct? https://github.com/commaai/openpilot/pull/26136

jyoung8607 commented 2 years ago

For the docs unit tests, when you removed the min steer speed from the individual HondaCarInfo instances, the default min steer speed set in the top level HondaCarInfo class replaced it. That HondaCarInfo default will need to go away if you're going to drive these settings via carParams, taking care to implement carParams for all Hondas or setting min speed explicitly at the individual HondaCarInfo instances.

For the process replay tests, for your purposes, don't worry about it failing for now.

jimbrend commented 11 months ago

why has this gone stale?

adeebshihadeh commented 8 months ago

Still relevant @sshane?

MarinkoMagla commented 8 months ago

Hi @sshane and @adeebshihadeh, if this issue is still relevant, i would like to contribute. I just wanted to ask about the information that already exists in CarParams and its discrepancy with the information from the wiki. For example, on the wiki it says that all VAG MQB can steer down to 0mph, while in the code we have different values. Also, for Ford models we have 0mph for TJA and APA and 35mph for LKA, and the code says 0mph for all cases.