Honda: fix alt brake address check race condition (#1723)

* common honda addr checks * oohhhh * rename RxCheck from merge * more rename * checked data and found 0x1A6 does not exist on any Bosch, so combining with 0x296 is safe * formatting * need this since refactor * clean up * fixup * misra * coverage caught another bug! * another duplicate addr from multi tests * proper base class * formatting * misra * flat * Revert "flat" This reverts commit d5ea2adf26.
2026-06-11 22:55:01 +08:00 · 2023-11-20 19:27:18 -08:00
parent c345bc3cae
commit 42a62936fb
4 changed files with 90 additions and 32 deletions
--- a/board/safety/safety_honda.h
+++ b/board/safety/safety_honda.h
@@ -33,31 +33,45 @@ const LongitudinalLimits HONDA_NIDEC_LONG_LIMITS = {
  .inactive_speed = 0,
 };

+// All common address checks except SCM_BUTTONS which isn't on one Nidec safety configuration
+#define HONDA_COMMON_NO_SCM_FEEDBACK_RX_CHECKS(pt_bus)                                                                                           \
+  {.msg = {{0x1A6, (pt_bus), 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U},                  /* SCM_BUTTONS */      \
+           {0x296, (pt_bus), 4, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U}, { 0 }}},                                \
+  {.msg = {{0x158, (pt_bus), 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U}, { 0 }, { 0 }}},  /* ENGINE_DATA */      \
+  {.msg = {{0x17C, (pt_bus), 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U}, { 0 }, { 0 }}},  /* POWERTRAIN_DATA */  \
+
+#define HONDA_COMMON_RX_CHECKS(pt_bus)                                                                                                  \
+  HONDA_COMMON_NO_SCM_FEEDBACK_RX_CHECKS(pt_bus)                                                                                        \
+  {.msg = {{0x326, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 100000U}, { 0 }, { 0 }}},  /* SCM_FEEDBACK */  \
+
+// Alternate brake message is used on some Honda Bosch, and Honda Bosch radarless (where PT bus is 0)
+#define HONDA_ALT_BRAKE_ADDR_CHECK(pt_bus)                                                                                                    \
+  {.msg = {{0x1BE, (pt_bus), 3, .check_checksum = true, .max_counter = 3U, .expected_timestep = 20000U}, { 0 }, { 0 }}},  /* BRAKE_MODULE */  \
+
+
 // Nidec and bosch radarless has the powertrain bus on bus 0
 RxCheck honda_common_rx_checks[] = {
-  {.msg = {{0x1A6, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U},                   // SCM_BUTTONS
-           {0x296, 0, 4, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U}, { 0 }}},
-  {.msg = {{0x158, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U}, { 0 }, { 0 }}},   // ENGINE_DATA
-  {.msg = {{0x17C, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U},                   // POWERTRAIN_DATA
-           {0x1BE, 0, 3, .check_checksum = true, .max_counter = 3U, .expected_timestep = 20000U}, { 0 }}},          // BRAKE_MODULE (for bosch radarless)
-  {.msg = {{0x326, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 100000U}, { 0 }, { 0 }}},  // SCM_FEEDBACK
+  HONDA_COMMON_RX_CHECKS(0)
 };

-// For Nidecs with main on signal on an alternate msg
+RxCheck honda_common_alt_brake_rx_checks[] = {
+  HONDA_COMMON_RX_CHECKS(0)
+  HONDA_ALT_BRAKE_ADDR_CHECK(0)
+};
+
+// For Nidecs with main on signal on an alternate msg (missing 0x326)
 RxCheck honda_nidec_alt_rx_checks[] = {
-  {.msg = {{0x1A6, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U},
-           {0x296, 0, 4, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U}, { 0 }}},
-  {.msg = {{0x158, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U}, { 0 }, { 0 }}},
-  {.msg = {{0x17C, 0, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U}, { 0 }, { 0 }}},
+  HONDA_COMMON_NO_SCM_FEEDBACK_RX_CHECKS(0)
 };

-// Bosch has pt on bus 1
+// Bosch has pt on bus 1, verified 0x1A6 does not exist
 RxCheck honda_bosch_rx_checks[] = {
-  {.msg = {{0x296, 1, 4, .check_checksum = true, .max_counter = 3U, .expected_timestep = 40000U}, { 0 }, { 0 }}},
-  {.msg = {{0x158, 1, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U}, { 0 }, { 0 }}},
-  {.msg = {{0x17C, 1, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 10000U},
-           {0x1BE, 1, 3, .check_checksum = true, .max_counter = 3U, .expected_timestep = 20000U}, { 0 }}},
-  {.msg = {{0x326, 1, 8, .check_checksum = true, .max_counter = 3U, .expected_timestep = 100000U}, { 0 }, { 0 }}},
+  HONDA_COMMON_RX_CHECKS(1)
+};
+
+RxCheck honda_bosch_alt_brake_rx_checks[] = {
+  HONDA_COMMON_RX_CHECKS(1)
+  HONDA_ALT_BRAKE_ADDR_CHECK(1)
 };

 const uint16_t HONDA_PARAM_ALT_BRAKE = 1;
@@ -382,12 +396,22 @@ static safety_config honda_bosch_init(uint16_t param) {
 #endif

  safety_config ret;
-  if (honda_bosch_radarless) {
-    ret = honda_bosch_long ? BUILD_SAFETY_CFG(honda_common_rx_checks, HONDA_RADARLESS_LONG_TX_MSGS) : \
-                             BUILD_SAFETY_CFG(honda_common_rx_checks, HONDA_RADARLESS_TX_MSGS);
+  if (honda_bosch_radarless && honda_alt_brake_msg) {
+    SET_RX_CHECKS(honda_common_alt_brake_rx_checks, ret);
+  } else if (honda_bosch_radarless) {
+    SET_RX_CHECKS(honda_common_rx_checks, ret);
+  } else if (honda_alt_brake_msg) {
+    SET_RX_CHECKS(honda_bosch_alt_brake_rx_checks, ret);
  } else {
-    ret = honda_bosch_long ? BUILD_SAFETY_CFG(honda_bosch_rx_checks, HONDA_BOSCH_LONG_TX_MSGS) : \
-                             BUILD_SAFETY_CFG(honda_bosch_rx_checks, HONDA_BOSCH_TX_MSGS);
+    SET_RX_CHECKS(honda_bosch_rx_checks, ret);
+  }
+
+  if (honda_bosch_radarless) {
+    honda_bosch_long ? SET_TX_MSGS(HONDA_RADARLESS_LONG_TX_MSGS, ret) : \
+                       SET_TX_MSGS(HONDA_RADARLESS_TX_MSGS, ret);
+  } else {
+    honda_bosch_long ? SET_TX_MSGS(HONDA_BOSCH_LONG_TX_MSGS, ret) : \
+                       SET_TX_MSGS(HONDA_BOSCH_TX_MSGS, ret);
  }
  return ret;
 }
--- a/board/safety_declarations.h
+++ b/board/safety_declarations.h
@@ -6,6 +6,10 @@

 #define BUILD_SAFETY_CFG(rx, tx) ((safety_config){(rx), (sizeof((rx)) / sizeof((rx)[0])), \
                                                  (tx), (sizeof((tx)) / sizeof((tx)[0]))})
+#define SET_RX_CHECKS(rx, config) ((config).rx_checks = (rx), \
+                                   (config).rx_checks_len = sizeof((rx)) / sizeof((rx)[0]))
+#define SET_TX_MSGS(tx, config) ((config).tx_msgs = (tx), \
+                                 (config).tx_msgs_len = sizeof((tx)) / sizeof((tx)[0]))

 uint32_t GET_BYTES(const CANPacket_t *msg, int start, int len) {
  uint32_t ret = 0U;
--- a/tests/safety/common.py
+++ b/tests/safety/common.py
@@ -862,7 +862,7 @@ class PandaSafetyTest(PandaSafetyTestBase):
            # TODO: Temporary, should be fixed in panda firmware, safety_honda.h
            if attr.startswith('TestHonda'):
              # exceptions for common msgs across different hondas
-              tx = list(filter(lambda m: m[0] not in [0x1FA, 0x30C, 0x33D], tx))
+              tx = list(filter(lambda m: m[0] not in [0x1FA, 0x30C, 0x33D, 0x33DB], tx))
            all_tx.append([[m[0], m[1], attr] for m in tx])

    # make sure we got all the msgs
--- a/tests/safety/test_honda.py
+++ b/tests/safety/test_honda.py
@@ -441,15 +441,6 @@ class TestHondaBoschSafetyBase(HondaBase):
  def _send_brake_msg(self, brake):
    pass

-  # TODO: add back in once alternative brake checksum/counter validation is added
-  # def test_alt_brake_rx_hook(self):
-  #   self.safety.set_honda_alt_brake_msg(1)
-  #   self.safety.set_controls_allowed(1)
-  #   to_push = self._alt_brake_msg(0)
-  #   self.assertTrue(self._rx(to_push))
-  #   to_push[0].RDLR = to_push[0].RDLR & 0xFFF0FFFF # invalidate checksum
-  #   self.assertFalse(self._rx(to_push))
-  #   self.assertFalse(self.safety.get_controls_allowed())
  def test_alt_disengage_on_brake(self):
    self.safety.set_honda_alt_brake_msg(1)
    self.safety.set_controls_allowed(1)
@@ -471,6 +462,28 @@ class TestHondaBoschSafetyBase(HondaBase):
    self.assertTrue(self._tx(self._button_msg(Btn.RESUME, bus=self.BUTTONS_BUS)))


+class TestHondaBoschAltBrakeSafetyBase(TestHondaBoschSafetyBase):
+  """
+    Base Bosch safety test class with an alternate brake message
+  """
+  def setUp(self):
+    super().setUp()
+    self.safety.set_safety_hooks(Panda.SAFETY_HONDA_BOSCH, Panda.FLAG_HONDA_ALT_BRAKE)
+    self.safety.init_tests()
+
+  def _user_brake_msg(self, brake):
+    return self._alt_brake_msg(brake)
+
+  def test_alt_brake_rx_hook(self):
+    self.safety.set_honda_alt_brake_msg(1)
+    self.safety.set_controls_allowed(1)
+    to_push = self._alt_brake_msg(0)
+    self.assertTrue(self._rx(to_push))
+    to_push[0].data[2] = to_push[0].data[2] & 0xF0  # invalidate checksum
+    self.assertFalse(self._rx(to_push))
+    self.assertFalse(self.safety.get_controls_allowed())
+
+
 class TestHondaBoschSafety(HondaPcmEnableBase, TestHondaBoschSafetyBase):
  """
    Covers the Honda Bosch safety mode with stock longitudinal
@@ -481,6 +494,12 @@ class TestHondaBoschSafety(HondaPcmEnableBase, TestHondaBoschSafetyBase):
    self.safety.init_tests()


+class TestHondaBoschAltBrakeSafety(HondaPcmEnableBase, TestHondaBoschAltBrakeSafetyBase):
+  """
+    Covers the Honda Bosch safety mode with stock longitudinal and an alternate brake message
+  """
+
+
 class TestHondaBoschLongSafety(HondaButtonEnableBase, TestHondaBoschSafetyBase):
  """
    Covers the Honda Bosch safety mode with longitudinal control
@@ -562,6 +581,17 @@ class TestHondaBoschRadarlessSafety(HondaPcmEnableBase, TestHondaBoschRadarlessS
    self.safety.init_tests()


+class TestHondaBoschRadarlessAltBrakeSafety(HondaPcmEnableBase, TestHondaBoschRadarlessSafetyBase, TestHondaBoschAltBrakeSafetyBase):
+  """
+    Covers the Honda Bosch Radarless safety mode with stock longitudinal and an alternate brake message
+  """
+
+  def setUp(self):
+    super().setUp()
+    self.safety.set_safety_hooks(Panda.SAFETY_HONDA_BOSCH, Panda.FLAG_HONDA_RADARLESS | Panda.FLAG_HONDA_ALT_BRAKE)
+    self.safety.init_tests()
+
+
 class TestHondaBoschRadarlessLongSafety(common.LongitudinalAccelSafetyTest, HondaButtonEnableBase,
                                        TestHondaBoschRadarlessSafetyBase):
  """