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

SP fixes for sync-20250309 #72

Merged
merged 4 commits into from
Mar 9, 2025
Merged

Conversation

devtekve
Copy link

@devtekve devtekve commented Mar 9, 2025

Summary by Sourcery

This pull request includes several fixes and improvements. It addresses an argument mismatch in a function call, adds a new test case for MADS heartbeat engagement, updates CAN message creation, increases timeout values for mutation tests, and updates the safety helpers.

Tests:

  • Adds a new test case for MADS heartbeat engaged check behavior, ensuring that the system correctly handles the engagement and disengagement of the heartbeat based on boolean values.
  • Adds a test to toggle MADS with the MADS button.
  • Adds tests for longitudinal control for ICE, hybrid, and EV cars with LFA steering, covering various scenarios and ensuring proper functionality across different vehicle types.

Updated the function call to match the required arguments by removing the redundant 'model' parameter. This ensures compatibility with the function definition and prevents potential errors.
Copy link

sourcery-ai bot commented Mar 9, 2025

Reviewer's Guide by Sourcery

This pull request includes several fixes and improvements. It adds a test case for the MADS heartbeat check, modifies the _accel_msg and _tx_acc_state_msg functions to use the PT_BUS constant, increases the timeout for safety mutation tests, adds an abstract method _tx_acc_state_msg, adds a function prototype for mads_heartbeat_engaged_check, and updates the get_params_for_docs function call.

Sequence diagram for get_params_for_docs function call

sequenceDiagram
  participant platform
  participant get_params_for_docs

  platform->>+get_params_for_docs: get_params_for_docs(platform)
  get_params_for_docs-->>-platform: CP
Loading

File-Level Changes

Change Details Files
Added a test case to verify the behavior of the MADS (Manually Activated Driver Steering) heartbeat engaged check.
  • Added a new test function test_heartbeat_engaged_mads_check.
  • The test function checks if the controls_allowed_lat state remains engaged or disengages based on the heartbeat state.
  • The test function calls mads_heartbeat_engaged_check multiple times to ensure the engagement state is correctly updated.
opendbc/safety/tests/mads_common.py
Modified the _accel_msg and _tx_acc_state_msg functions to use the PT_BUS constant instead of hardcoded bus numbers.
  • Replaced the literal 1 with self.PT_BUS in the _accel_msg function.
  • Replaced the literal 0 with self.PT_BUS in the _tx_acc_state_msg function.
opendbc/safety/tests/test_hyundai_canfd.py
opendbc/safety/tests/test_hyundai_can.py
Increased the timeout for safety mutation tests in the GitHub Actions workflow.
  • Increased the timeout-minutes for the mutation job from 20 to 60.
  • Increased the timeout-minutes for the Run mutation tests step from 10 to 60.
.github/workflows/tests.yml
Added an abstract method _tx_acc_state_msg to the hyundai_common.py file.
  • Added _tx_acc_state_msg to the HyundaiCommon class.
opendbc/safety/tests/hyundai_common.py
Added a function prototype for mads_heartbeat_engaged_check to the PandaSafety protocol.
  • Added void mads_heartbeat_engaged_check(void); to the setup_safety_helpers function.
  • Added def mads_heartbeat_engaged_check(self) -> None: ... to the PandaSafety protocol.
opendbc/safety/tests/libsafety/safety_helpers.py
Updated the get_params_for_docs function call by removing the redundant 'model' parameter.
  • Removed the model parameter from the get_params_for_docs function call.
opendbc/sunnypilot/car/platform_list.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

devtekve added 3 commits March 9, 2025 09:44
Introduce the _tx_acc_state_msg method in Hyundai safety tests to handle ACC state messaging. Replace hardcoded bus values with the self.PT_BUS variable for consistency and maintainability across SCC_CONTROL messages.
Introduce a new test function `test_heartbeat_engaged_mads_check` to verify the behavior of MADS heartbeat engagement logic under varying conditions. This ensures correct state transitions and validates robustness against mismatched heartbeat signals.
Extended the timeout for the entire safety mutation tests workflow and its individual steps from 20 and 10 minutes respectively to 60 minutes. This change ensures that longer-running tests complete successfully without premature termination.
@devtekve
Copy link
Author

devtekve commented Mar 9, 2025

Note: The safety mutation failure is expected
commaai#1852 (comment)
image

@devtekve devtekve marked this pull request as ready for review March 9, 2025 10:21
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine.

@devtekve devtekve merged commit 353a2d3 into sync-20250309 Mar 9, 2025
6 of 7 checks passed
@devtekve devtekve deleted the sync-20250309-sp-fixes branch March 9, 2025 10:23
devtekve added a commit that referenced this pull request Mar 9, 2025
* Hyundai CAN-FD Safety: Update wheel speed signal parsing (commaai#1850)

* reimplement

* use average wheel speed

* fix factor with eEgoRaw

* Revert "fix factor with eEgoRaw"

This reverts commit 35f3624.

* Revert "use average wheel speed"

This reverts commit ec4e9c1.

* revert to previous

* mutation again ugh

* mutation you got to be kidding me

* nice, this passes test models now

---------

Co-authored-by: Shane Smiskol <[email protected]>

* Angle safety: test above max angle (commaai#1879)

* test test_angle_cmd_when_enabled above max angle and set them

* nissan is too high

* clean up

* more

* fingerprint: 2023 Tesla Model Y (HW3) (commaai#1882)

* Hyundai CAN FD: ADAS steering API updates (commaai#1873)

* Hyundai CAN FD: Steering API updates

* comments

* comments

* keep it for now

* not used

* should use DBC spec

* consistent

---------

Co-authored-by: Shane Smiskol <[email protected]>

* Remove test_generator from commit hooks (commaai#1883)

* angle limits: use a struct (commaai#1885)

* struct to prevent forgetting something and make more organized

* use it

* move all over

* clean up

* rm

* Revert "Hyundai CAN FD: ADAS steering API updates" (commaai#1888)

Revert "Hyundai CAN FD: ADAS steering API updates (commaai#1873)"

This reverts commit 2a75806.

* Hyundai CAN FD: add angle control signals to the DBC (commaai#1887)

* restart

* fix

* missing

* remove stuff we don't use

* huh

* fix

* fix fix fix

* we haven't seen LKAS with angle yet

* this is missing from the new message

* ...

* more explicit

* flip

* import

* Hyundai CAN FD angle control: add fault signal

* Run format_fingerprints.py

* Fix Ford CAN FD safety replay (commaai#1889)

* safety replay: fix messages on segment boundaries not sorted (commaai#1890)

* Update replay_drive.py

* warn

* no true

* tqdm too

* master-ci is dead

* Replace gcov & lcov with gcovr (commaai#1867)

Use gcovr instead of gcov to support exclusion markers.

This also replaces lcov, since gcov has far fewer dependencies,
can be installed via uv (and looks better if anything).

* Ford Q4: limit max curvature from vehicle speed (commaai#1852)

* curvature safety: limit to ISO max

* .

* cmt

* implement in carcontroller

* MISRAAAAAA

* use ~average road roll

* stash

* clean up

* compiles

* fix safety replay for ford can fd

* always init

* stash

* fix replay

* fix that

* safety doesn't consider rate limits when clipping max curvature

consider: at the max curvature limit and user gas overrides heavily, max curvature limit might drop quicker than the safet rate limit will allow for

* don't limit for CAN

* do tests (need to clean up)

* stash

* rm

* clean up tests

* fix that

* smaller

* fix capnp error

* because we allow tolerance on the error limiting, we need the same type of tolerance on the max accel curvature to not block

* fix a test

* fix another

* consistent naming

* debugging

* debug

* remove

* stash

* clean up

* clean up

* minor

* and this

* space

* misra

* doesn't matter here

* safety mutation test fix

* misra false positive?

* rename

* ah good catch, these are equivalent

* comment

* clean up

* rename like safety

* fix

* simpler

* not needed

* Ford CAN FD: enable safety in release (commaai#1896)

CAN FD out of ALLOW_DEBUG

* Hyundai CAN FD: test ACC cancel (commaai#1898)

* debug

* remove debugging

* huh

* Coverage fixups (commaai#1900)

* fixup

* test

* bye copilot

* rev

* fix

* fix

* test again

* revert

* need

* Only run tx hooks on msgs in safety_config.tx_msgs (commaai#1851)

* Only run tx hooks on msgs in safety_config.tx_msgs.

* 'fix' tests

* retrigger checks bc timeouts

* not hit

* formatting

* add back

---------

Co-authored-by: Shane Smiskol <[email protected]>

* Add Bosch C harness

* Ford CAN FD: upstream car docs (commaai#1904)

* in upstream

* fix tests

* Car docs: small cleanup

these are the same

* Toyota: low speed lockout is a fault only if openpilot longitudinal (commaai#1906)

only if long

* Ford CAN FD: SecOC is dashcam (commaai#1908)

* no tron (2010)

* switch to actual ACC message

* switch to camera

* add logging

* Ford CAN FD: fix docs

* add CI test artifacts to gitignore (commaai#1916)

* Tesla: add Model Y 2022 (HW3) (commaai#1915)

add fingerprint

* website: use new highlight class (commaai#1920)

* highlight

* not here

* `test_models_trigger.yaml`: add checkout check to prevent opendbc bump fail (commaai#1919)

add checkout check to prevent opendbc bump fail

* Rivian: add VDM fault signal (commaai#1918)

* add VDM fault signal

* fix

* Rivian: accFaulted comments

* Fulfillment parts (commaai#1921)

* highlight

* not here

* changes for fulfilment parts

* not used

* Tesla: setSpeed is vEgo while disabled (commaai#1923)

set set speed to vEgo while inactive

* carcontroller cleanup (commaai#1924)

* cc cleanup

* ??????

* docs: start extra cars clean up (commaai#1925)

* support support-type

* fix

* fix

* fix

* Rivian safety: minor clean up

* Rivian safety: test accel actuation message w/ stock long (commaai#1927)

stock longitudinal when stock long too

* Rivian: cancel command (commaai#1910)

* add candidate cancel command and test

* fixes

* this was correct

* try spacing out the messages

* try adas status to acm to cancel

* try a bunch of stuff

* send the entire range

* try faulted

* try two faults

* try faster

* 50hz

* Revert "50hz"

This reverts commit babcbdf.

* no fault on dash

* 50hz

* see if "unavailable" also works

* try sending just 1

* 100 Hz

* Revert "100 Hz"

This reverts commit 29ae625.

* Revert "try sending just 1"

This reverts commit 64908eb.

* revert other stuff - forgot counter!

* Revert "revert other stuff - forgot counter!"

This reverts commit 5ce09b7.

* try counter

* final thing

* 100 hz

* Revert "100 hz" (faults)

This reverts commit bea3977.

* nvm we can't do this since the state management to know when openpilot stops sending cancel is too complex

* Revert "nvm we can't do this since the state management to know when openpilot stops sending cancel is too complex"

This reverts commit 19fb385.

* forward through openpilot

* try these two

* let's also try this

* are we doing this right?

* temp fault?

* ?

* try oscillating?!

* that works?!

* maybe ACM needs to see VDM status rise before VDM can request cancel with a falling edge

* try to cancel immediately (1 frame of available)

* no counter

* cmts

* 3 frames

* comments

* clean up safety

* clean up

* cc clean up

* typo

* comments

* clean this up too

* and this too

* Rivian: fix fuzzy test_models failures (commaai#1929)

* fix possible gear exception

* fix cruise state mismatch

* clean up

* Rivian: don't show set speed in UI (commaai#1842)

* Rivian: don't show set speed in UI

* carparam

* just use speed

* remove a dynamic import

fix

* Replace @parameterized.expand with @pytest.mark.parametrize to improve test speed (commaai#1930)

* Replace @parameterized.expand with @pytest.mark.parametrize for faster test collection

This change reduces the opendbc/car/tests/ collection time to ~0.4s.

* clean up

---------

Co-authored-by: Shane Smiskol <[email protected]>

* Tesla: support 0 cruise state speed (commaai#1928)

so far only tesla has been seen being able to set to 0

* VW MEB: Reserve safety identifier (commaai#1931)

* VW PQ: Add FW for 2012 Jetta Sportwagen TDI (commaai#1712)

* Update interface.py

* Update fingerprints.py

* Revert "Update interface.py"

This reverts commit a2abbc0.

* HKG: Add FW for 2020 Hyundai Ioniq HEV (commaai#1914)

* Update fingerprints.py

* 2020 confirmed!

* should be fine

---------

Co-authored-by: Jason Young <[email protected]>

* vin: add parse function (commaai#1935)

* parse vin

* Update opendbc/car/vin.py

* Rivian: values cleanup

* Vin helper class (commaai#1937)

* TODO: cant pass down due to circular imports

* revert

* screw it, we can split vin.py and vin_query.py later. this is nicer

* Revert "screw it, we can split vin.py and vin_query.py later. this is nicer"

This reverts commit 9ddab33.

* Rivian: VIN fuzzy fingerprinting (commaai#1821)

* add match_fw_to_car_fuzzy

* remove comment

* fix dbc

* format

* don't remove...

* parse_vin function

* test caught bug

* safe string accessing

---------

Co-authored-by: Shane Smiskol <[email protected]>

* safety: checksum and counter are explicitly opt-out (commaai#1939)

* this is what we want

* rm old

* and now counter

* need this

* toyota

* subaru's good

* Tesla and Chrysler

* Ford

* do some find and replace

* do the rest

* clean up

* remove this

* rm

* not broken but bad

* this was wrong!

* safety replay: fix torque init

* safety replay: start to support Hyundai CAN FD

* safety tests: common attrs (commaai#1948)

* set FWD_BUS_LOOKUP to most common

* this is always assumed to be a number, test models tests mismatches

* clean up

* and this

* Rivian safety: check missing rx addr (commaai#1949)

Fix Rivian

* Rivian: remove a VDM ACC fault signal (commaai#1951)

only what we've triggered

* SP fixes for sync-20250309 (#72)

* Fix get_params_for_docs call to use correct parameters

Updated the function call to match the required arguments by removing the redundant 'model' parameter. This ensures compatibility with the function definition and prevents potential errors.

* Add _tx_acc_state_msg method and standardize PT_BUS usage

Introduce the _tx_acc_state_msg method in Hyundai safety tests to handle ACC state messaging. Replace hardcoded bus values with the self.PT_BUS variable for consistency and maintainability across SCC_CONTROL messages.

* Add MADS heartbeat engaged state validation test

Introduce a new test function `test_heartbeat_engaged_mads_check` to verify the behavior of MADS heartbeat engagement logic under varying conditions. This ensures correct state transitions and validates robustness against mismatched heartbeat signals.

* Increase timeout for safety mutation tests to 60 minutes

Extended the timeout for the entire safety mutation tests workflow and its individual steps from 20 and 10 minutes respectively to 60 minutes. This change ensures that longer-running tests complete successfully without premature termination.

* Bring replay_drive from SP into opendbc instead of panda

---------

Co-authored-by: Jason Wen <[email protected]>
Co-authored-by: Shane Smiskol <[email protected]>
Co-authored-by: Trey Moen <[email protected]>
Co-authored-by: Eric Brown <[email protected]>
Co-authored-by: Adeeb Shihadeh <[email protected]>
Co-authored-by: chris dunder <[email protected]>
Co-authored-by: Jason Young <[email protected]>
Co-authored-by: Lukas <[email protected]>
Co-authored-by: Mauricio Alvarez Leon <[email protected]>
Co-authored-by: Aniurm <[email protected]>
Co-authored-by: insertwittyusername <[email protected]>
Co-authored-by: royjr <[email protected]>
Co-authored-by: Jason Young <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant