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.

Sign up for GitHub

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

Jump to bottom

Hyundai CAN FD: utilize macros for common address checks #1658

Merged
merged 21 commits into from
Sep 8, 2023
Merged

Hyundai CAN FD: utilize macros for common address checks #1658

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b7f7f62
draft
sshane Sep 7, 2023
4257abf
now clean up old comments
sshane Sep 7, 2023
f4a50ca
no more special broken ice addr checks
sshane Sep 7, 2023
2908e8a
revert
sshane Sep 7, 2023
88039ff
fix
sshane Sep 7, 2023
76d2b0f
add test for ICE HDA1
sshane Sep 7, 2023
97b0235
Revert "add test for ICE HDA1"
sshane Sep 7, 2023
3636033
can be separate
sshane Sep 7, 2023
0ad5b82
macro for addr check struct
sshane Sep 7, 2023
c235983
add checks for alt buttons (fixes race condition)
sshane Sep 7, 2023
a69b4ee
add macro for array len
sshane Sep 7, 2023
42d48df
add comment
sshane Sep 7, 2023
a136d92
misra
sshane Sep 7, 2023
a83c636
comments to help separate
sshane Sep 7, 2023
6f21f85
can't put parenthesis around array item
sshane Sep 7, 2023
b80c9e5
review suggestions
sshane Sep 8, 2023
93955a8
no intermediary macros for making the structs (remove misra violation)
sshane Sep 8, 2023
d895bbd
Update board/safety/safety_hyundai_canfd.h
sshane Sep 8, 2023
b8a6686
Merge remote-tracking branch 'upstream/master' into hkg-canfd-addr-ch…
sshane Sep 8, 2023
3c65a82
single lines
sshane Sep 8, 2023
d4546d4
can avoid double checking hda2 since we have addr checks for it now
sshane Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 75 additions & 67 deletions board/safety/safety_hyundai_canfd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,68 +53,66 @@ const CanMsg HYUNDAI_CANFD_HDA1_TX_MSGS[] = {
{0x1E0, 0, 16}, // LFAHDA_CLUSTER
};

AddrCheckStruct hyundai_canfd_addr_checks[] = {
{.msg = {{0x35, 1, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0x35, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0x105, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}}},
{.msg = {{0x175, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U},
{0x175, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }}},
{.msg = {{0xa0, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0xa0, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }}},
{.msg = {{0xea, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0xea, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }}},
{.msg = {{0x1a0, 1, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U},
{0x1a0, 2, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }}},
{.msg = {{0x1cf, 1, 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U},
{0x1cf, 0, 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U},
{0x1aa, 0, 16, .check_checksum = false, .max_counter = 0xffU, .expected_timestep = 20000U}}},
};
#define HYUNDAI_CANFD_ADDR_CHECK_LEN (sizeof(hyundai_canfd_addr_checks) / sizeof(hyundai_canfd_addr_checks[0]))

// 0x1a0 is on bus 0
AddrCheckStruct hyundai_canfd_radar_scc_addr_checks[] = {
{.msg = {{0x35, 1, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0x35, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0x105, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}}},
{.msg = {{0x175, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U},
{0x175, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }}},
{.msg = {{0xa0, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0xa0, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }}},
{.msg = {{0xea, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0xea, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }}},
{.msg = {{0x1a0, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }, { 0 }}},
{.msg = {{0x1cf, 1, 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U},
{0x1cf, 0, 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U},
{0x1aa, 0, 16, .check_checksum = false, .max_counter = 0xffU, .expected_timestep = 20000U}}},
};
#define HYUNDAI_CANFD_RADAR_SCC_ADDR_CHECK_LEN (sizeof(hyundai_canfd_radar_scc_addr_checks) / sizeof(hyundai_canfd_radar_scc_addr_checks[0]))

AddrCheckStruct hyundai_canfd_long_addr_checks[] = {
{.msg = {{0x35, 1, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0x35, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0x105, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}}},
{.msg = {{0x175, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U},
{0x175, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }}},
{.msg = {{0xa0, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0xa0, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }}},
{.msg = {{0xea, 1, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U},
{0xea, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }}},
{.msg = {{0x1cf, 1, 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U},
{0x1cf, 0, 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U},
{0x1aa, 0, 16, .check_checksum = false, .max_counter = 0xffU, .expected_timestep = 20000U}}},
};
#define HYUNDAI_CANFD_LONG_ADDR_CHECK_LEN (sizeof(hyundai_canfd_long_addr_checks) / sizeof(hyundai_canfd_long_addr_checks[0]))

AddrCheckStruct hyundai_canfd_ice_addr_checks[] = {
{.msg = {{0x100, 0, 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }, { 0 }}},
{.msg = {{0xa0, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }, { 0 }}},
{.msg = {{0xea, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }, { 0 }}},
{.msg = {{0x175, 0, 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }, { 0 }}},
{.msg = {{0x1aa, 0, 16, .check_checksum = false, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }, { 0 }}},
sshane marked this conversation as resolved.
Show resolved Hide resolved
};
#define HYUNDAI_CANFD_ICE_ADDR_CHECK_LEN (sizeof(hyundai_canfd_ice_addr_checks) / sizeof(hyundai_canfd_ice_addr_checks[0]))

addr_checks hyundai_canfd_rx_checks = {hyundai_canfd_addr_checks, HYUNDAI_CANFD_ADDR_CHECK_LEN};
// *** Addresses checked in rx hook ***
#define HYUNDAI_CANFD_COMMON_ADDR_CHECKS(pt_bus) \
/* EV, ICE, HYBRID: ACCELERATOR, ACCELERATOR_BRAKE_ALT, ACCELERATOR_ALT */ \
sshane marked this conversation as resolved.
Show resolved Hide resolved
{.msg = {{0x35, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, \
{0x100, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, \
{0x105, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}}}, \
{.msg = {{0x175, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }, { 0 }}}, \
{.msg = {{0xa0, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }, { 0 }}}, \
{.msg = {{0xea, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 10000U}, { 0 }, { 0 }}}, \

#define HYUNDAI_CANFD_SCC_ADDR_CHECK(scc_bus) \
/* SCC_CONTROL (from ADAS unit or camera) */ \
{.msg = {{0x1a0, (scc_bus), 32, .check_checksum = true, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }, { 0 }}}, \

#define HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(pt_bus) \
{.msg = {{0x1cf, (pt_bus), 8, .check_checksum = false, .max_counter = 0xfU, .expected_timestep = 20000U}, { 0 }, { 0 }}}, \

#define HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK(pt_bus) \
{.msg = {{0x1aa, (pt_bus), 16, .check_checksum = false, .max_counter = 0xffU, .expected_timestep = 20000U}, { 0 }, { 0 }}}, \


// *** Macros to create addr check structs ***
#define HYUNDAI_CANFD_CREATE_ADDR_CHECK(name, pt_bus, scc_bus, button_msg) \
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 */ \
Copy link
Contributor

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?

Copy link
Contributor Author

@sshane sshane Sep 8, 2023 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look good?

}; \

#define HYUNDAI_CANFD_CREATE_LONG_ADDR_CHECK(name, pt_bus, button_msg) \
AddrCheckStruct (name)[] = { \
/* SCC not checked since its ECU is disabled on HDA2 and we use buttons */ \
HYUNDAI_CANFD_COMMON_ADDR_CHECKS((pt_bus)) \
button_msg((pt_bus)) /* cppcheck-suppress misra-c2012-20.7 */ \
}; \


// *** Non-HDA2 checks ***
// Camera sends SCC messages on HDA1.
// Both button messages exist on some platforms, so we ensure we track the correct one
sshane marked this conversation as resolved.
Show resolved Hide resolved
HYUNDAI_CANFD_CREATE_ADDR_CHECK(hyundai_canfd_addr_checks, 0, 2, HYUNDAI_CANFD_BUTTONS_ADDR_CHECK)
HYUNDAI_CANFD_CREATE_ADDR_CHECK(hyundai_canfd_alt_buttons_addr_checks, 0, 2, HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK)

// Longitudinal checks for HDA1
HYUNDAI_CANFD_CREATE_LONG_ADDR_CHECK(hyundai_canfd_long_addr_checks, 0, HYUNDAI_CANFD_BUTTONS_ADDR_CHECK)
HYUNDAI_CANFD_CREATE_LONG_ADDR_CHECK(hyundai_canfd_long_alt_buttons_addr_checks, 0, HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK)

// Radar sends SCC messages on these cars instead of camera
HYUNDAI_CANFD_CREATE_ADDR_CHECK(hyundai_canfd_radar_scc_addr_checks, 0, 0, HYUNDAI_CANFD_BUTTONS_ADDR_CHECK)
HYUNDAI_CANFD_CREATE_ADDR_CHECK(hyundai_canfd_radar_scc_alt_buttons_addr_checks, 0, 0, HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK)


// *** HDA2 checks ***
// E-CAN is on bus 1, ADAS unit sends SCC messages on HDA2.
// Does not use the alt buttons message
HYUNDAI_CANFD_CREATE_ADDR_CHECK(hyundai_canfd_hda2_addr_checks, 1, 1, HYUNDAI_CANFD_BUTTONS_ADDR_CHECK)
HYUNDAI_CANFD_CREATE_LONG_ADDR_CHECK(hyundai_canfd_hda2_long_addr_checks, 1, HYUNDAI_CANFD_BUTTONS_ADDR_CHECK)

addr_checks hyundai_canfd_rx_checks = {hyundai_canfd_addr_checks, ARRAY_LEN(hyundai_canfd_addr_checks)};


uint16_t hyundai_canfd_crc_lut[256];
Expand Down Expand Up @@ -369,14 +367,24 @@ static const addr_checks* hyundai_canfd_init(uint16_t param) {
}

if (hyundai_longitudinal) {
hyundai_canfd_rx_checks = (addr_checks){hyundai_canfd_long_addr_checks, HYUNDAI_CANFD_LONG_ADDR_CHECK_LEN};
if (hyundai_canfd_hda2) {
hyundai_canfd_rx_checks = (addr_checks){hyundai_canfd_hda2_long_addr_checks, ARRAY_LEN(hyundai_canfd_hda2_long_addr_checks)};
sshane marked this conversation as resolved.
Show resolved Hide resolved
} else {
hyundai_canfd_rx_checks = hyundai_canfd_alt_buttons
? (addr_checks){hyundai_canfd_long_alt_buttons_addr_checks, ARRAY_LEN(hyundai_canfd_long_alt_buttons_addr_checks)}
: (addr_checks){hyundai_canfd_long_addr_checks, ARRAY_LEN(hyundai_canfd_long_addr_checks)};
}
} else {
if (!hyundai_ev_gas_signal && !hyundai_hybrid_gas_signal) {
hyundai_canfd_rx_checks = (addr_checks){hyundai_canfd_ice_addr_checks, HYUNDAI_CANFD_ICE_ADDR_CHECK_LEN};
} else if (!hyundai_camera_scc && !hyundai_canfd_hda2) {
hyundai_canfd_rx_checks = (addr_checks){hyundai_canfd_radar_scc_addr_checks, HYUNDAI_CANFD_RADAR_SCC_ADDR_CHECK_LEN};
if (!hyundai_camera_scc && !hyundai_canfd_hda2) {
hyundai_canfd_rx_checks = hyundai_canfd_alt_buttons
? (addr_checks){hyundai_canfd_radar_scc_alt_buttons_addr_checks, ARRAY_LEN(hyundai_canfd_radar_scc_alt_buttons_addr_checks)}
: (addr_checks){hyundai_canfd_radar_scc_addr_checks, ARRAY_LEN(hyundai_canfd_radar_scc_addr_checks)};
} else if (hyundai_canfd_hda2) {
hyundai_canfd_rx_checks = (addr_checks){hyundai_canfd_hda2_addr_checks, ARRAY_LEN(hyundai_canfd_hda2_addr_checks)};
} else {
hyundai_canfd_rx_checks = (addr_checks){hyundai_canfd_addr_checks, HYUNDAI_CANFD_ADDR_CHECK_LEN};
hyundai_canfd_rx_checks = hyundai_canfd_alt_buttons
? (addr_checks){hyundai_canfd_alt_buttons_addr_checks, ARRAY_LEN(hyundai_canfd_alt_buttons_addr_checks)}
: (addr_checks){hyundai_canfd_addr_checks, ARRAY_LEN(hyundai_canfd_addr_checks)};
}
}

Expand Down
2 changes: 2 additions & 0 deletions board/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
(_a > 0) ? _a : (-_a); \
})

#define ARRAY_LEN(name) (sizeof((name)) / sizeof((name)[0]))

#ifndef NULL
#define NULL ((void*)0)
#endif
Expand Down
Loading