-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
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 See: #1840 (but fix the segfault and validity setting gracefully) |
This helped and I went with using 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. |
applied 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 |
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. |
Yep, we just want to make it more obvious whenever you're skipping a counter or checksum check |
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 }}}, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{.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}, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ignore_counter
Merging in #1939 |
fixes #1839