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

Ford: handle metric cruise speed (v2) #31463

Merged
merged 7 commits into from
Mar 21, 2024

Conversation

incognitojam
Copy link
Contributor

@incognitojam incognitojam commented Feb 14, 2024

Description

I found a signal which appears to match the IPC "Show km/h" setting.
Requires commaai/opendbc#1010. Previous attempt: #29526.

Closes #29494.

Verification

  • Test in car and confirm that toggling the "Show km/h" setting does not result in the cruise speed shown in openpilot being incorrect. Started in kph: e886087f430e7fe7/2024-02-14--19-23-28, in mph: e886087f430e7fe7/2024-02-14--19-27-31
  • Test in a non-English (metric) car: 32a35015aa4d5598/2024-03-03--14-58-48

@incognitojam
Copy link
Contributor Author

@magicparty @chiachunli08 Do either of you want to test this branch for me? installer.comma.ai/incognitojam/ford-ipc-metric-v2

@incognitojam
Copy link
Contributor Author

incognitojam commented Feb 28, 2024

The signal is present on the Bronco Sport (from the process replay test route)
image

What does this mean? I'm guessing a change in canValid is not unexpected when adding a new message

***** results for segment regen6D39E54606E|2023-10-30--23-20-54--0 *****
    controlsd
        ref: https://commadataci.blob.core.windows.net/openpilotci/regen6D39E54606E|2023-10-30--23-20-54--0_controlsd_d0cdea7eb15f3cac8a921f7ace3eaa6baebb4fd5.bz2
        new: /tmp/openpilot/openpilot/selfdrive/test/process_replay/fakedata/regen6D39E54606E|2023-10-30--23-20-54--0_controlsd_ec907e09cd9fa3901c5d64fc6340c774100fd591.bz2

        carState.canValid: 19
        valid: 57

@@ -54,7 +54,8 @@ def update(self, cp, cp_cam):
ret.steerFaultTemporary |= cp.vl["Lane_Assist_Data3_FD1"]["LatCtlSte_D_Stat"] not in (1, 2, 3)

# cruise state
ret.cruiseState.speed = cp.vl["EngBrakeData"]["Veh_V_DsplyCcSet"] * CV.MPH_TO_MS
is_metric = cp.vl["INSTRUMENT_PANEL"]["METRIC_UNITS"] == 1 if self.CP.carFingerprint not in CANFD_CAR else False
Copy link
Contributor Author

@incognitojam incognitojam Feb 28, 2024

Choose a reason for hiding this comment

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

Signal does not appear to be present on CAN FD platform (checked 112e4d6e0cad05e1|2023-11-14--08-21-43) so will keep this for now.

@magicparty
Copy link
Contributor

@magicparty @chiachunli08 Do either of you want to test this branch for me? installer.comma.ai/incognitojam/ford-ipc-metric-v2

I will test it this weekend

Copy link
Contributor

github-actions bot commented Feb 29, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@magicparty
Copy link
Contributor

@incognitojam I tested this solution on my car, and it worked.
route: 32a35015aa4d5598/2024-03-03--14-58-48

@incognitojam
Copy link
Contributor Author

@magicparty thank you! can you make the route public?

@incognitojam
Copy link
Contributor Author

There isn't really anything to check if it worked. Ready for review.

**Description**

I found a signal which appears to match the IPC "Show km/h" setting.
Requires commaai/opendbc#1010.

**Verification**

- [ ] Test in car and confirm that toggling the "Show km/h" setting does
  not result in the cruise speed shown in openpilot being incorrect.
- [ ] Test in a non-English (metric) car.
Comment on lines 135 to 138
else:
messages += [
("INSTRUMENT_PANEL", 10),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check how long this message takes to become valid/present?

Copy link
Contributor Author

@incognitojam incognitojam Mar 12, 2024

Choose a reason for hiding this comment

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

Using commaCarSegments
notebook
image

Copy link
Contributor Author

@incognitojam incognitojam Mar 12, 2024

Choose a reason for hiding this comment

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

The ones with no message are F-150 and Mach-E
image

@@ -131,6 +132,10 @@ def get_can_parser(CP):
messages += [
("Lane_Assist_Data3_FD1", 33),
]
else:
messages += [
("INSTRUMENT_PANEL", 10),
Copy link
Contributor

@sshane sshane Mar 20, 2024

Choose a reason for hiding this comment

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

are you sure this is 10Hz? It's 1Hz on our Bronco, and takes 0.48s to show up on the process replay route (VehicleOperatingModes is instant for example), which makes sense given it's 1Hz in that route. When it shows up is when carState now goes valid, also making sense.

What does this mean? I'm guessing a change in canValid is not unexpected when adding a new message

That's because it's just expanding the invalid carState time/messages out to when we see all the CAN messages, it takes 0.79 seconds for the TOYOTA3 route to go valid for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, it was an oversight from me. must be 1Hz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason it's still invalid when freq=1

Copy link
Contributor

Choose a reason for hiding this comment

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

See

That's because it's just expanding the invalid carState time/messages out to when we see all the CAN messages, it takes 0.79 seconds for the TOYOTA3 route to go valid for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I thought it would go valid quicker for 1hz than 10hz

Copy link
Contributor

Choose a reason for hiding this comment

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

It goes valid when it sees the message

@sshane sshane merged commit b59ae50 into commaai:master Mar 21, 2024
27 checks passed
@incognitojam incognitojam deleted the ford-ipc-metric-v2 branch March 21, 2024 23:27
cydia2020 pushed a commit to cydia2020/dodgypilot that referenced this pull request May 14, 2024
* Ford: handle metric cruise speed (v2)

**Description**

I found a signal which appears to match the IPC "Show km/h" setting.
Requires commaai/opendbc#1010.

**Verification**

- [ ] Test in car and confirm that toggling the "Show km/h" setting does
  not result in the cruise speed shown in openpilot being incorrect.
- [ ] Test in a non-English (metric) car.

* not present on Q4

* fix freq

* test

* Revert "test"

This reverts commit 5e3a9f6.

* Update ref_commit

---------

Co-authored-by: Shane Smiskol <shane@smiskol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ford maximum cruise speed incorrect
4 participants