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

safety: set true by default for both counter and checksum #1907

Closed
wants to merge 8 commits into from

Conversation

BBBmau
Copy link
Contributor

@BBBmau BBBmau commented Mar 3, 2025

fixes #1839

@github-actions github-actions bot added the car safety vehicle-specific safety code label Mar 3, 2025
@sshane
Copy link
Contributor

sshane commented Mar 3, 2025

How does this pass? You should need to flip all of the definitions for all the safety modes from false to true, and vice versa

We want the checksum and counter checks to fail by default, i.e. if check_checksum isn't set to false, but there's no checksum function added

I suggest renaming check_checksum to ignore_checksum and maybe a new flag for ignoring counter if needed

See: #1840 (but fix the segfault and validity setting gracefully)

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 4, 2025

We want the checksum and counter checks to fail by default, i.e. if check_checksum isn't set to false, but there's no checksum function added

This helped and I went with using ignore_checksum though I see that we get 6 fail tests from this change. (this may be expected since it's doing the check_sum yet no checksum function is set correct?)

FAILED test_hyundai.py::TestHyundaiSafetyFCEV::test_allow_engage_with_gas_pressed - AssertionError: False is not true
FAILED test_hyundai.py::TestHyundaiSafetyFCEV::test_alternative_experience_no_disengage_on_gas - AssertionError: False is not true
FAILED test_hyundai.py::TestHyundaiSafetyFCEV::test_prev_gas - AssertionError: True != False
FAILED test_hyundai.py::TestHyundaiLegacySafetyHEV::test_alternative_experience_no_disengage_on_gas - AssertionError: False is not true
FAILED test_hyundai.py::TestHyundaiLegacySafetyHEV::test_prev_gas - AssertionError: True != False
FAILED test_hyundai.py::TestHyundaiLegacySafetyHEV::test_allow_engage_with_gas_pressed - AssertionError: False is not true

I'll look into it more in-depth tomorrow but I wouldn't expect that the 6 hyundai tests to fail.

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 4, 2025

applied ignore_checksum on the following messages

This makes sense since the whole idea of defaulting that it's true is to ensure which tests are missing checksum functions or not, the 6 hyundai tests weren't using it and thus forced me to apply the ignore_checksum on both .msg. Moving onto counter now.

@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 4, 2025

I didn't run into the case involving the segfault and wasn't able to reproduce it. I also didn't run into any issues with setting the skip_counter as I did with checksum. Moving to review although I expect there are some things that I'm just completely missing.

@BBBmau BBBmau marked this pull request as ready for review March 4, 2025 23:03
@sshane
Copy link
Contributor

sshane commented Mar 4, 2025

Yep, we just want to make it more obvious whenever you're skipping a counter or checksum check

@BBBmau BBBmau requested a review from sshane March 6, 2025 18:12
@sshane sshane added this to the openpilot 0.9.9 milestone Mar 6, 2025
@sshane
Copy link
Contributor

sshane commented Mar 7, 2025

This still doesn't do what we want. Rivian is untouched, but we aren't checking its checksum or counter

{.msg = {{0x260, 0, 8, .max_counter = 3U, .frequency = 100U}, \
{0x371, 0, 8, .ignore_checksum = true, .frequency = 100U}, \
{0x91, 0, 8, .ignore_checksum = true, .frequency = 100U}}}, \
{.msg = {{0x386, 0, 8, .ignore_checksum = !(legacy), .max_counter = (legacy) ? 0U : 15U, .frequency = 100U}, { 0 }, { 0 }}}, \
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
{.msg = {{0x386, 0, 8, .ignore_checksum = !(legacy), .max_counter = (legacy) ? 0U : 15U, .frequency = 100U}, { 0 }, { 0 }}}, \
{.msg = {{0x386, 0, 8, .ignore_checksum = (legacy), .max_counter = (legacy) ? 0U : 15U, .frequency = 100U}, { 0 }, { 0 }}}, \

note this inconsistency

{.msg = {{0x260, 0, 8, .quality_flag = (lta), .frequency = 50U}, { 0 }, { 0 }}}, \
{.msg = {{0x1D2, 0, 8, .frequency = 33U}, \
{0x176, 0, 8, .frequency = 32U}, { 0 }}}, \
{.msg = {{0x101, 0, 8, .ignore_checksum = true, .frequency = 50U}, \
Copy link
Contributor

Choose a reason for hiding this comment

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

missing ignore_counter

@sshane
Copy link
Contributor

sshane commented Mar 7, 2025

Merging in #1939

@sshane sshane closed this Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car safety vehicle-specific safety code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[$100 bounty] safety: counter and checksum tests should be true by default
2 participants