-
Notifications
You must be signed in to change notification settings - Fork 778
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
Hyundai CAN FD: utilize macros for common address checks #1658
Conversation
This reverts commit 76d2b0f.
@adeebshihadeh this fixes the missing |
board/safety/safety_hyundai_canfd.h
Outdated
AddrCheckStruct (name)[] = { \ | ||
HYUNDAI_CANFD_COMMON_ADDR_CHECKS((pt_bus)) \ | ||
HYUNDAI_CANFD_SCC_ADDR_CHECK((scc_bus)) \ | ||
button_msg((pt_bus)) /* cppcheck-suppress misra-c2012-20.7 */ \ |
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.
what's the suppression for?
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.
Misra wants all args enclosed in parenthesis for safety, but the macro is expanded incorrectly if you do (button_msg)((pt_bus))
panda/board/safety/safety_hyundai_canfd.h:135:3: error: called object is not a function or function pointer
135 | (button_msg)((pt_bus)), /* cppcheck-suppress misra-c2012-20.7 */ \
| ^
creating a macro to replace each AddrCheckStruct initialization works but adds another layer of indirection
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.
Removed the HYUNDAI_CANFD_CREATE_ADDR_CHECK
helpers and required each safety config to define all its messages from the available address macros. Removes this misra violation, but is just a bit longer in lines. Should be clearer though.
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.
Look good?
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.
LGTM
@sunnyhaibin adding ICE long support should be easy now, feel free to attempt it if you have spare time (let's get a route!) |
test models passes on internal seglist of HKG enforcing we only update when the address was checked through the address checks! (before there was the ICE SCC_CONTROL issue and button issue) |
Will do! |
Related: #1655
Fixes the missing SCC_CONTROL check discovered in #1656, fixes a race condition with selecting wrong button check, and allows ICE to work with long by simply removing the if statement that blocks it (future PR pending a user test and route)