-
Notifications
You must be signed in to change notification settings - Fork 837
Ford: add safety code for LCA vehicles #966
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
Conversation
Ready for a review? I'm cool merging this behind the |
Sure! |
Your review comments disappeared but I've made those changes anyway |
4d90f41
to
b1f3606
Compare
b1f3606
to
59ec4b6
Compare
59ec4b6
to
22e9f9f
Compare
board/safety/safety_ford.h
Outdated
{.msg = {{MSG_ENG_BRAKE_DATA, 0, 8, .expected_timestep = 100000U}, { 0 }, { 0 }}}, | ||
{.msg = {{MSG_ENG_VEHICLE_SP_THROTTLE2, 0, 8, .expected_timestep = 20000U}, { 0 }, { 0 }}}, | ||
{.msg = {{MSG_ENG_VEHICLE_SP_THROTTLE, 0, 8, .expected_timestep = 10000U}, { 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.
these don't have any counters or checksums?
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.
Oh I missed this, ApedPos_Pc_ActlArb
has a checksum and counter in MSG_ENG_VEHICLE_SP_THROTTLE
...
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.
I'm cool merging this as-is and adding counter/checksum handling in a follow up PR if you want
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.
sure!
board/safety/safety_ford.h
Outdated
bool violation = 0U != (GET_BYTE(to_send, 0) | | ||
(GET_BYTE(to_send, 1) & 0xFEU) | | ||
GET_BYTE(to_send, 2) | | ||
GET_BYTE(to_send, 3) | | ||
GET_BYTE(to_send, 4) | | ||
GET_BYTE(to_send, 5)); |
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.
this can be cleaned up with GET_BYTES_04
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.
I tried switching it to GET_BYTES_04(to_send) & 0xFEU
but then we would be allowing some other button presses to get through
Co-authored-by: Adeeb Shihadeh <[email protected]>
Co-authored-by: Adeeb Shihadeh <[email protected]>
Co-authored-by: Adeeb Shihadeh <[email protected]>
Co-authored-by: Adeeb Shihadeh <[email protected]>
tests/safety/test_ford.py
Outdated
def _user_brake_msg(self, brake: bool): | ||
# brake pedal and cruise state share same message, so we have to send | ||
# the other signal too | ||
enable: bool = self.safety.get_controls_allowed() |
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.
this type annoation shouldn't be necessary. can we annotate the return type of safety.get_controls_allowed()
instead?
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.
I've added a whole set of type hints which I think is a good start
I couldn't find any way to get CFFI to generate them? not really sure how that would work
Once those comments are address and the tests pass, this should be good to merge. |
* Ford: add CAN messages and signal definitions * Ford: implement safety hooks * Ford: add safety unit tests * Use standstill signal * Rename bus Co-authored-by: Adeeb Shihadeh <[email protected]> * Simplify brake pressed Co-authored-by: Adeeb Shihadeh <[email protected]> * Ford LKAS msg helper * Simplify button violation check Co-authored-by: Adeeb Shihadeh <[email protected]> * Simplify LKAS action check Co-authored-by: Adeeb Shihadeh <[email protected]> * Tidy up tests * Use standstill signal for tests * Misra fix * Unused definition Co-authored-by: Adeeb Shihadeh <[email protected]> * Simplify signal bit shifting * Remove type hints * Add panda safety type hints * Fix vehicle moving signal * Fix button checks * Fix steer allowed tests * Fix standstill threshold in tests * Maybe this works? * More button tests * Revert "Simplify button violation check" This reverts commit c649626. Co-authored-by: Adeeb Shihadeh <[email protected]>
* Ford: add CAN messages and signal definitions * Ford: implement safety hooks * Ford: add safety unit tests * Use standstill signal * Rename bus Co-authored-by: Adeeb Shihadeh <[email protected]> * Simplify brake pressed Co-authored-by: Adeeb Shihadeh <[email protected]> * Ford LKAS msg helper * Simplify button violation check Co-authored-by: Adeeb Shihadeh <[email protected]> * Simplify LKAS action check Co-authored-by: Adeeb Shihadeh <[email protected]> * Tidy up tests * Use standstill signal for tests * Misra fix * Unused definition Co-authored-by: Adeeb Shihadeh <[email protected]> * Simplify signal bit shifting * Remove type hints * Add panda safety type hints * Fix vehicle moving signal * Fix button checks * Fix steer allowed tests * Fix standstill threshold in tests * Maybe this works? * More button tests * Revert "Simplify button violation check" This reverts commit c649626. Co-authored-by: Adeeb Shihadeh <[email protected]>
Safety code for the LCA vehicles (support added in commaai/openpilot#23331)
Currently this does not include any rate limit checks.