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

Subaru: Crosstrek Hybrid dashcam mode #25378

Merged
merged 20 commits into from
Aug 24, 2023
Merged

Subaru: Crosstrek Hybrid dashcam mode #25378

merged 20 commits into from
Aug 24, 2023

Conversation

adeebshihadeh
Copy link
Contributor

No description provided.

@adeebshihadeh
Copy link
Contributor Author

@martinl got a test route for me?

Comment on lines 58 to 63
<previouslyLoaded_Datafiles>
<fileInfo filename="../../../../threepilot/tools/plotjuggler/tmp03i8v25j.rlog" prefix="">
<selected_datasources value=""/>
<plugin ID="DataLoad Rlog"/>
</fileInfo>
</previouslyLoaded_Datafiles>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<previouslyLoaded_Datafiles>
<fileInfo filename="../../../../threepilot/tools/plotjuggler/tmp03i8v25j.rlog" prefix="">
<selected_datasources value=""/>
<plugin ID="DataLoad Rlog"/>
</fileInfo>
</previouslyLoaded_Datafiles>

@sshane
Copy link
Contributor

sshane commented Aug 26, 2022

It appears there are no similar messages on bus 0 to the cruise active status. Most common bits are quite a bit off:

hex(msg)='0x361', bit_mismatches=981 of 3720 (26.37%), byt_idx=1, bit_idx=2
hex(msg)='0x1f9', bit_mismatches=1476 of 6009 (24.56%), byt_idx=5, bit_idx=7
hex(msg)='0x1f9', bit_mismatches=1002 of 6009 (16.67%), byt_idx=5, bit_idx=6
hex(msg)='0x244', bit_mismatches=1589 of 10190 (15.59%), byt_idx=4, bit_idx=3
Searched bus: 0

On bus 2, it gets a bit interesting. There's of course the bit on 0x321 which is what I'm comparing (ES_DashStatus), but also 0x220 which is (ES_Brake):

ex(msg)='0x321', bit_mismatches=864 of 2423 (35.66%), byt_idx=5, bit_idx=2
hex(msg)='0x220', bit_mismatches=56 of 4800 (1.17%), byt_idx=0, bit_idx=0
hex(msg)='0x220', bit_mismatches=56 of 4800 (1.17%), byt_idx=4, bit_idx=0
hex(msg)='0x321', bit_mismatches=9 of 2423 (0.37%), byt_idx=4, bit_idx=3
Searched bus: 2

However I checked both signals on older ICE cars, and they proved to be inaccurate, specifically:

There's no other similar signals to ES_DashStatus->Cruise_Activated, so we'll have to verify that this signal on the hybrids don't have these same problems.

@sshane
Copy link
Contributor

sshane commented Aug 30, 2022

This is also missing the hybrid signals for gas and brake: https://github.com/ClockeNessMnstr/openpilot/blob/crosstrek-pid/selfdrive/car/subaru/carstate.py#L23

ES_DashStatus->Cruise_Activated has the same gas pressed issue, but ES_Brake does not seem to have the same issue where you press the brake at a standstill and it doesn't fall.

Screenshot from 2022-08-30 11-39-23

Showing gas pressed behavior:
Screenshot from 2022-08-30 11-39-31

@sshane sshane mentioned this pull request Dec 5, 2022
4 tasks
@martinl
Copy link
Contributor

martinl commented Dec 5, 2022

There is also ES_Status (0x222) Cruise_Activated, I'll check if it behaves differently

@martinl
Copy link
Contributor

martinl commented Dec 10, 2022

On Crosstrek ICE, ES_Status Cruise_Activated is identical to CruiseControl Cruise_Activated signal, I tested with 3 cases above - stopping behind lead car / hold (standstill), disengage using brake while in standstill, gas press while engaged
8de015561e1ea4a0|2022-12-10--14-08-26

@martinl
Copy link
Contributor

martinl commented Dec 10, 2022

I checked Crosstrek Hybrid dbc and it does not have ES_Status message. I'll find the signal once I find someone who can provide route for Crosstrek Hybrid

@jnewb1
Copy link
Contributor

jnewb1 commented Aug 17, 2023

crosstrek hybrid route: 31d10447d8c7dc19|2023-08-17--07-30-31--0

@jnewb1
Copy link
Contributor

jnewb1 commented Aug 23, 2023

I'll just merge the hybrid cars behind dashcam, we still need a good cruise_activated bit. Would be helpful to get a forester hybrid route so I can see if they are the same

@jnewb1 jnewb1 changed the title Subaru: Crosstrek Hybrid support Subaru: Crosstrek Hybrid dashcam mode Aug 23, 2023
@jnewb1 jnewb1 added the subaru label Aug 23, 2023
@sshane
Copy link
Contributor

sshane commented Aug 23, 2023

Can you merge #24770 as dashcam after this? Make sure to leave a comment in the code at least of what is required to un-dashcam

@jnewb1 jnewb1 marked this pull request as ready for review August 24, 2023 01:12
@jnewb1 jnewb1 merged commit 973d90b into master Aug 24, 2023
@jnewb1 jnewb1 deleted the subaru-hybrid branch August 24, 2023 01:50
@@ -3,6 +3,7 @@
from openpilot.selfdrive.car import apply_driver_steer_torque_limits
from openpilot.selfdrive.car.subaru import subarucan
from openpilot.selfdrive.car.subaru.values import DBC, GLOBAL_GEN2, PREGLOBAL_CARS, CanBus, CarControllerParams, SubaruFlags
from selfdrive.car.subaru.values import HYBRID_CARS
Copy link
Contributor

Choose a reason for hiding this comment

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

we already import from values above

if self.car_fingerprint not in HYBRID_CARS: # Hybrid cars don't have es_distance, need a replacement
# 8 is known AEB, there are a few other values related to AEB we ignore
ret.stockAeb = (cp_es_distance.vl["ES_Brake"]["AEB_Status"] == 8) and \
(cp_es_distance.vl["ES_Brake"]["Brake_Pressure"] != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing

Comment on lines +108 to +114
self.es_distance_msg = copy.copy(cp_es_distance.vl["ES_Distance"])

self.es_status_msg = copy.copy(cp_es_status.vl["ES_Status"])
self.cruise_control_msg = copy.copy(cp_cruise.vl["CruiseControl"])

if self.car_fingerprint not in HYBRID_CARS:
self.es_distance_msg = copy.copy(cp_es_distance.vl["ES_Distance"])
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we can simplify this so we don't need to copy twice?

@sshane
Copy link
Contributor

sshane commented Aug 24, 2023

Make sure to request a final review before merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants