[HKG] Add ALT_BUTTONS, ALT_STEERING, & refactor safety.

ccdunder · ccdunder · commit d7a1032525b2 · 2025-02-20T23:12:05.000Z
As a newish contributor to this codebase, I should make clear up front that I
haven't taken up this refactoring lightly or without good reason. I would love to avoid it but it seems necessary and less bad than all the other options.  If there is another way to accomplish these goals I'm all ears.

Root problem: HKG flags combine to produce combinatoric configurations.
Supporting this requires tradeoffs between maintainability, convention,
and quality.

Alternatives considered:
1. Continue making RX &amp; TX allowlists overly permissive.
   This seems antithetical to the whole point of safety flags.
2. Continue Copy-pasting nested branches with 1-line differences.
   It seems like this has already been done to the limit of
   reasonable maintainability (3-4 conditions deep).
   Also, to support the new conditions here would mean duplicating the code 8x.

Callout: MISRA
I am not a MISRA expert but AFAIU this conforms. Happy to discuss and iterate if this is incorrect. There are other options that trade readability for reduced dynamism. But at a certain point making everything static for the sake of MISRA seems to miss the forest for the trees.

LMK what you think. I'd love to hear if there's a better way to solve this.
diff --git a/opendbc/safety/safety/safety_hyundai_canfd.h b/opendbc/safety/safety/safety_hyundai_canfd.h
@@ -3,25 +3,26 @@
 #include "safety_declarations.h"
 #include "safety_hyundai_common.h"
 
-// *** Addresses checked in rx hook ***
-// EV, ICE, HYBRID: ACCELERATOR (0x35), ACCELERATOR_BRAKE_ALT (0x100), ACCELERATOR_ALT (0x105)
-#define HYUNDAI_CANFD_COMMON_RX_CHECKS(pt_bus)                                                                              \
-  {.msg = {{0x35, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U},                   \
-           {0x100, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U},                  \
-           {0x105, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}}},                \
-  {.msg = {{0x175, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .frequency = 50U}, { 0 }, { 0 }}},  \
-  {.msg = {{0xa0, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}},   \
-  {.msg = {{0xea, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}},   \
-
-#define HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(pt_bus)                                                                            \
-  {.msg = {{0x1cf, (pt_bus), 8, .check_checksum = false, .max_counter = 0xfU, .frequency = 50U}, { 0 }, { 0 }}}, \
-
-#define HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK(pt_bus)                                                                            \
-  {.msg = {{0x1aa, (pt_bus), 16, .check_checksum = false, .max_counter = 0xffU, .frequency = 50U}, { 0 }, { 0 }}},   \
-
-// SCC_CONTROL (from ADAS unit or camera)
-#define HYUNDAI_CANFD_SCC_ADDR_CHECK(scc_bus)                                                                                 \
-  {.msg = {{0x1a0, (scc_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 50U}, { 0 }, { 0 }}}, \
+#define MAX_RX_CHECKS 16
+
+static safety_config hyundai_canfd_init_safety_config(void) {
+  static RxCheck hyundai_canfd_rx_checks[MAX_RX_CHECKS] = {0};
+  safety_config ret = {
+    .rx_checks = hyundai_canfd_rx_checks,
+    .rx_checks_len = 0,
+    .tx_msgs = NULL,
+    .tx_msgs_len = 0
+  };
+  return ret;
+}
+
+static void add_rx_check(safety_config *safetyConfig, RxCheck config) {
+  const uint8_t index = safetyConfig->rx_checks_len;
+  safetyConfig->rx_checks_len++;
+  if (index < (uint8_t)MAX_RX_CHECKS) {
+      (void)memcpy(&safetyConfig->rx_checks[index], &config, sizeof(RxCheck));
+  }
+}
 
 static bool hyundai_canfd_alt_buttons = false;
 static bool hyundai_canfd_hda2_alt_steering = false;
@@ -154,10 +155,16 @@ static bool hyundai_canfd_tx_hook(const CANPacket_t *to_send) {
   }
 
   // cruise buttons check
-  if (addr == 0x1cf) {
-    int button = GET_BYTE(to_send, 2) & 0x7U;
-    bool is_cancel = (button == HYUNDAI_BTN_CANCEL);
-    bool is_resume = (button == HYUNDAI_BTN_RESUME);
+  const int button_addr = hyundai_canfd_alt_buttons ? 0x1aa : 0x1cf;
+  if (addr == button_addr) {
+    int cruise_button = 0;
+    if (addr == 0x1cf) {
+      cruise_button = GET_BYTE(to_send, 2) & 0x7U;
+    } else {
+      cruise_button = (GET_BYTE(to_send, 4) >> 4) & 0x7U;
+    }
+    bool is_cancel = (cruise_button == HYUNDAI_BTN_CANCEL);
+    bool is_resume = (cruise_button == HYUNDAI_BTN_RESUME);
 
     bool allowed = (is_cancel && cruise_engaged_prev) || (is_resume && controls_allowed);
     if (!allowed) {
@@ -211,11 +218,12 @@ static int hyundai_canfd_fwd_hook(int bus_num, int addr) {
 
     // HUD icons
     bool is_lfahda_msg = ((addr == 0x1e0) && !hyundai_canfd_hda2);
+    bool is_ccnc_msg = (((addr == 0x161) || (addr == 0x162)) && !hyundai_canfd_hda2);
 
     // CRUISE_INFO for non-HDA2, we send our own longitudinal commands
     bool is_scc_msg = ((addr == 0x1a0) && hyundai_longitudinal && !hyundai_canfd_hda2);
 
-    bool block_msg = is_lkas_msg || is_lfa_msg || is_lfahda_msg || is_scc_msg;
+    bool block_msg = is_lkas_msg || is_lfa_msg || is_lfahda_msg || is_ccnc_msg || is_scc_msg;
     if (!block_msg) {
       bus_fwd = 0;
     }
@@ -228,22 +236,32 @@ static safety_config hyundai_canfd_init(uint16_t param) {
   const int HYUNDAI_PARAM_CANFD_HDA2_ALT_STEERING = 128;
   const int HYUNDAI_PARAM_CANFD_ALT_BUTTONS = 32;
 
+  // TODO: Build these more precisely like RX_CHECKS.
   static const CanMsg HYUNDAI_CANFD_HDA2_TX_MSGS[] = {
     {0x50, 0, 16},  // LKAS
     {0x1CF, 1, 8},  // CRUISE_BUTTON
+    // TODO: Include this iff CANFD_ALT_BUTTONS
+    {0x1AA, 1, 16}, // CRUISE_BUTTONS_ALT
     {0x2A4, 0, 24}, // CAM_0x2A4
   };
 
   static const CanMsg HYUNDAI_CANFD_HDA2_ALT_STEERING_TX_MSGS[] = {
     {0x110, 0, 32}, // LKAS_ALT
     {0x1CF, 1, 8},  // CRUISE_BUTTON
+    // TODO: Include this iff CANFD_ALT_BUTTONS
+    {0x1AA, 1, 16}, // CRUISE_BUTTONS_ALT
+    // Needed for cruise control in case of ALT_BUTTONS.
+    {0x1A0, 1, 32}, // CRUISE_INFO
     {0x362, 0, 32}, // CAM_0x362
   };
 
   static const CanMsg HYUNDAI_CANFD_HDA2_LONG_TX_MSGS[] = {
     {0x50, 0, 16},  // LKAS
     {0x1CF, 1, 8},  // CRUISE_BUTTON
+    // TODO: Include this iff CANFD_ALT_BUTTONS
+    {0x1AA, 1, 16}, // CRUISE_BUTTONS_ALT
     {0x2A4, 0, 24}, // CAM_0x2A4
+    // LONG start
     {0x51, 0, 32},  // ADRV_0x51
     {0x730, 1, 8},  // tester present for ADAS ECU disable
     {0x12A, 1, 16}, // LFA
@@ -254,13 +272,41 @@ static safety_config hyundai_canfd_init(uint16_t param) {
     {0x200, 1, 8},  // ADRV_0x200
     {0x345, 1, 8},  // ADRV_0x345
     {0x1DA, 1, 32}, // ADRV_0x1da
+    {0x38C, 1, 32}, // ADRV_0x38c
+    {0x161, 1, 32}, // MSG_161
+    {0x162, 1, 32}, // MSG_162
+  };
+
+  static const CanMsg HYUNDAI_CANFD_HDA2_ALT_STEERING_LONG_TX_MSGS[] = {
+    {0x110, 0, 32}, // LKAS_ALT
+    {0x1CF, 1, 8},  // CRUISE_BUTTON
+    // TODO: Include this iff CANFD_ALT_BUTTONS
+    {0x1AA, 1, 16}, // CRUISE_BUTTONS_ALT
+    {0x1A0, 1, 32}, // CRUISE_INFO
+    {0x362, 0, 32}, // CAM_0x362
+    // LONG start
+    {0x51, 0, 32},  // ADRV_0x51
+    {0x730, 1, 8},  // tester present for ADAS ECU disable
+    {0x12A, 1, 16}, // LFA
+    {0x160, 1, 16}, // ADRV_0x160
+    {0x1E0, 1, 16}, // LFAHDA_CLUSTER
+    {0x1EA, 1, 32}, // ADRV_0x1ea
+    {0x200, 1, 8},  // ADRV_0x200
+    {0x345, 1, 8},  // ADRV_0x345
+    {0x1DA, 1, 32}, // ADRV_0x1da
+    {0x38C, 1, 32}, // ADRV_0x38c
+    {0x161, 1, 32}, // MSG_161
+    {0x162, 1, 32}, // MSG_162
   };
 
   static const CanMsg HYUNDAI_CANFD_HDA1_TX_MSGS[] = {
     {0x12A, 0, 16}, // LFA
     {0x1A0, 0, 32}, // CRUISE_INFO
     {0x1CF, 2, 8},  // CRUISE_BUTTON
+    {0x1AA, 2, 16}, // CRUISE_BUTTONS_ALT
     {0x1E0, 0, 16}, // LFAHDA_CLUSTER
+    {0x161, 0, 32}, // MSG_161
+    {0x162, 0, 32}, // MSG_162
   };
 
 
@@ -275,77 +321,57 @@ static safety_config hyundai_canfd_init(uint16_t param) {
     hyundai_longitudinal = false;
   }
 
-  safety_config ret;
+  safety_config ret = hyundai_canfd_init_safety_config();
+
+  // RX Common checks.
+  const int pt_bus = hyundai_canfd_hda2 ? 1 : 0;
+  add_rx_check(&ret, (RxCheck){.msg = {{0x175, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .frequency = 50U}, { 0 }, { 0 }}});
+  add_rx_check(&ret, (RxCheck){.msg = {{0xa0, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}});
+  add_rx_check(&ret, (RxCheck){.msg = {{0xea, (pt_bus), 24, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}});
+
+  if (hyundai_ev_gas_signal) {
+    add_rx_check(&ret, (RxCheck){.msg = {{0x35, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}});
+  } else if (hyundai_hybrid_gas_signal) {
+    add_rx_check(&ret, (RxCheck){.msg = {{0x105, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}});
+  } else {
+    add_rx_check(&ret, (RxCheck){.msg = {{0x100, (pt_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 100U}, { 0 }, { 0 }}});
+  }
+
+  // RX Cruise buttons.
+  if (hyundai_canfd_alt_buttons) {
+    add_rx_check(&ret, (RxCheck){.msg = {{0x1aa, (pt_bus), 16, .check_checksum = false, .max_counter = 0xffU, .frequency = 50U}, { 0 }, { 0 }}});
+  } else {
+    add_rx_check(&ret, (RxCheck){.msg = {{0x1cf, (pt_bus), 8, .check_checksum = false, .max_counter = 0xfU, .frequency = 50U}, { 0 }, { 0 }}});
+  }
+
   if (hyundai_longitudinal) {
-    if (hyundai_canfd_hda2) {
-      static RxCheck hyundai_canfd_hda2_long_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(1)
-        HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(1)
-      };
+    // TX SCC_CONTROL.
+  } else {
+    // RX SCC_CONTROL.
+    const int scc_bus = hyundai_canfd_hda2 ? 1 : hyundai_camera_scc ? 2 : 0;
+    add_rx_check(&ret, (RxCheck){.msg = {{0x1a0, (scc_bus), 32, .check_checksum = true, .max_counter = 0xffU, .frequency = 50U}, { 0 }, { 0 }}});
+  }
 
-      ret = BUILD_SAFETY_CFG(hyundai_canfd_hda2_long_rx_checks, HYUNDAI_CANFD_HDA2_LONG_TX_MSGS);
+  // TX checks.
+  if (hyundai_longitudinal) {
+    if (hyundai_canfd_hda2) {
+      if (hyundai_canfd_hda2_alt_steering) {
+        SET_TX_MSGS(HYUNDAI_CANFD_HDA2_ALT_STEERING_LONG_TX_MSGS, ret);
+      } else {
+        SET_TX_MSGS(HYUNDAI_CANFD_HDA2_LONG_TX_MSGS, ret);
+      }
     } else {
-      static RxCheck hyundai_canfd_long_alt_buttons_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(0)
-        HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK(0)
-      };
-
-      // Longitudinal checks for HDA1
-      static RxCheck hyundai_canfd_long_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(0)
-        HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(0)
-      };
-
-      ret = hyundai_canfd_alt_buttons ? BUILD_SAFETY_CFG(hyundai_canfd_long_alt_buttons_rx_checks, HYUNDAI_CANFD_HDA1_TX_MSGS) : \
-                                        BUILD_SAFETY_CFG(hyundai_canfd_long_rx_checks, HYUNDAI_CANFD_HDA1_TX_MSGS);
+      SET_TX_MSGS(HYUNDAI_CANFD_HDA1_TX_MSGS, ret);
     }
   } else {
     if (hyundai_canfd_hda2) {
-      // *** HDA2 checks ***
-      // E-CAN is on bus 1, ADAS unit sends SCC messages on HDA2.
-      // Does not use the alt buttons message
-      static RxCheck hyundai_canfd_hda2_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(1)
-        HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(1)
-        HYUNDAI_CANFD_SCC_ADDR_CHECK(1)
-      };
-
-      ret = hyundai_canfd_hda2_alt_steering ? BUILD_SAFETY_CFG(hyundai_canfd_hda2_rx_checks, HYUNDAI_CANFD_HDA2_ALT_STEERING_TX_MSGS) : \
-                                              BUILD_SAFETY_CFG(hyundai_canfd_hda2_rx_checks, HYUNDAI_CANFD_HDA2_TX_MSGS);
-    } else if (!hyundai_camera_scc) {
-      static RxCheck hyundai_canfd_radar_scc_alt_buttons_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(0)
-        HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK(0)
-        HYUNDAI_CANFD_SCC_ADDR_CHECK(0)
-      };
-
-      // Radar sends SCC messages on these cars instead of camera
-      static RxCheck hyundai_canfd_radar_scc_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(0)
-        HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(0)
-        HYUNDAI_CANFD_SCC_ADDR_CHECK(0)
-      };
-
-      ret = hyundai_canfd_alt_buttons ? BUILD_SAFETY_CFG(hyundai_canfd_radar_scc_alt_buttons_rx_checks, HYUNDAI_CANFD_HDA1_TX_MSGS) : \
-                                        BUILD_SAFETY_CFG(hyundai_canfd_radar_scc_rx_checks, HYUNDAI_CANFD_HDA1_TX_MSGS);
+      if (hyundai_canfd_hda2_alt_steering) {
+        SET_TX_MSGS(HYUNDAI_CANFD_HDA2_ALT_STEERING_TX_MSGS, ret);
+      } else {
+        SET_TX_MSGS(HYUNDAI_CANFD_HDA2_TX_MSGS, ret);
+      }
     } else {
-      // *** Non-HDA2 checks ***
-      static RxCheck hyundai_canfd_alt_buttons_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(0)
-        HYUNDAI_CANFD_ALT_BUTTONS_ADDR_CHECK(0)
-        HYUNDAI_CANFD_SCC_ADDR_CHECK(2)
-      };
-
-      // Camera sends SCC messages on HDA1.
-      // Both button messages exist on some platforms, so we ensure we track the correct one using flag
-      static RxCheck hyundai_canfd_rx_checks[] = {
-        HYUNDAI_CANFD_COMMON_RX_CHECKS(0)
-        HYUNDAI_CANFD_BUTTONS_ADDR_CHECK(0)
-        HYUNDAI_CANFD_SCC_ADDR_CHECK(2)
-      };
-
-      ret = hyundai_canfd_alt_buttons ? BUILD_SAFETY_CFG(hyundai_canfd_alt_buttons_rx_checks, HYUNDAI_CANFD_HDA1_TX_MSGS) : \
-                                        BUILD_SAFETY_CFG(hyundai_canfd_rx_checks, HYUNDAI_CANFD_HDA1_TX_MSGS);
+      SET_TX_MSGS(HYUNDAI_CANFD_HDA1_TX_MSGS, ret);
     }
   }
 
diff --git a/opendbc/safety/tests/libsafety/safety_helpers.h b/opendbc/safety/tests/libsafety/safety_helpers.h
@@ -12,7 +12,16 @@ bool safety_config_valid() {
     const RxCheck addr = current_safety_config.rx_checks[i];
     bool valid = addr.status.msg_seen && !addr.status.lagging && addr.status.valid_checksum && (addr.status.wrong_counters < MAX_WRONG_COUNTERS) && addr.status.valid_quality_flag;
     if (!valid) {
-      // printf("i %d seen %d lagging %d valid checksum %d wrong counters %d valid quality flag %d\n", i, addr.status.msg_seen, addr.status.lagging, addr.status.valid_checksum, addr.status.wrong_counters, addr.status.valid_quality_flag);
+      printf("i %d seen %d lagging %d valid checksum %d wrong counters %d valid quality flag %d\n",
+      i, addr.status.msg_seen, addr.status.lagging, addr.status.valid_checksum, addr.status.wrong_counters, addr.status.valid_quality_flag);
+      for (int j = 0; j < MAX_ADDR_CHECK_MSGS; j++) {
+        const CanMsgCheck msg = addr.msg[j];
+        if (msg.addr == 0) {
+          break;
+        }
+        printf("i %d j %d bus %d addr 0x%x len %d\n",
+        i, j, msg.bus, msg.addr, msg.len);
+      }
       return false;
     }
   }
diff --git a/opendbc/safety/tests/test_hyundai_canfd.py b/opendbc/safety/tests/test_hyundai_canfd.py
