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

Issues on nissan_rx_checks on file safety_nissan.h #744

Open
thanosat opened this issue Oct 19, 2021 · 1 comment
Open

Issues on nissan_rx_checks on file safety_nissan.h #744

thanosat opened this issue Oct 19, 2021 · 1 comment

Comments

@thanosat
Copy link

thanosat commented Oct 19, 2021

The following analysis applies only for NISSAN LEAF.
nissan.safety.h (https://github.com/commaai/panda/blob/master/board/safety/safety_nissan.h#L22) defines 10Hz for the expected reception of CRUISE_STATE msg, while on OpenPilot that frequency is at 50Hz (https://github.com/commaai/openpilot/blob/master/selfdrive/car/nissan/carstate.py#L202).

It is not fully clear which is the correct, because OP CANParser object "checks" (see carstate.py "checks" list) are checking for 50Hz to create timeouts and if the actual case is 10Hz (which is slower), no timeout from OP side looks eminent because from what I have seen I think we need to miss roughly 10 messages to create a timeout.
From Panda FW side, this frequency is used again to verify for possible timeouts from the car, but no matter 10 or 50Hz being the actual case, the relevant consistency checks from the receiving side of the Panda FW are exaggerated quite a bit to define a max of 1s lagging messages from the car (see safety.h , safety_tick function). This means that it doesn't matter if we receive at 10 or 50 Hz since the verification will happen at 1Hz. It should be though corrected on all cases to the actual frequency in order to avoid potential future problems and weird timeouts, if additional verifications are added.

And then again on nissan.safety.h line 29, BRAKE message (hex: 1cc) is defined as having a DLC of 4 bytes, but the nissan dbc defines the same msg with 8 bytes (https://github.com/commaai/opendbc/blob/d597599ebe9fcccf87f42fd588e3b4a4ec39e00a/nissan_leaf_2018.dbc#L70). The real case should be 4, otherwise I believe OP shouldn’t work at all since we should get "controls_allowed" = 0 from Panda FW side. So most likely just a dbc problem here. The actual dbc is used to create data structures from CANParser/CANPacker objects on OP and potential impact on that side of the SW is not clear. Since CANParser will mostly just verify the frequency (which is correct) and CANPacker will pack only specific messages and not this particular one, there probably isnt any major problem from this issue if indeed the correct case is 4. If the correct case is 8 bytes then I would definitely expect Panda Safety to through some error message.
If someone wants to verify these problems, I would be happy to further explain and support potential corrections.

@sshane
Copy link
Contributor

sshane commented Feb 15, 2024

#1732 and openpilot's fuzzing test should hopefully catch this. Will come back and test

https://github.com/commaai/openpilot/blob/c65dfaac68bbf836900dd612a33667cb00cdf854/selfdrive/car/tests/test_models.py#L331

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

No branches or pull requests

2 participants