Honda: fix controls mismatch for stock PCM configs (#840)

* Honda: fix controls mismatch for stock PCM configs

* start tests

* all tests

* fix pylint

* misra

* Update board/safety/safety_honda.h

board/safety/safety_honda.h

@@ -95,6 +95,8 @@ static int honda_rx_hook(CANPacket_t *to_push) {
   bool valid = addr_safety_check(to_push, &honda_rx_checks,
                                  honda_get_checksum, honda_compute_checksum, honda_get_counter);
 
+  const bool pcm_cruise = ((honda_hw == HONDA_BOSCH) && !honda_bosch_long) || ((honda_hw == HONDA_NIDEC) && !gas_interceptor_detected);
+
   if (valid) {
     int addr = GET_ADDR(to_push);
     int len = GET_LEN(to_push);
@@ -115,9 +117,21 @@ static int honda_rx_hook(CANPacket_t *to_push) {
       }
     }
 
-    // state machine to enter and exit controls
+    // enter controls when PCM enters cruise state
+    if (pcm_cruise && (addr == 0x17C)) {
+      const bool cruise_engaged = GET_BIT(to_push, 38U) != 0U;
+      if (!cruise_engaged) {
+        controls_allowed = 0;
+      }
+      if (cruise_engaged && !cruise_engaged_prev) {
+        controls_allowed = 1;
+      }
+      cruise_engaged_prev = cruise_engaged;
+    }
+
+    // state machine to enter and exit controls for button enabling
     // 0x1A6 for the ILX, 0x296 for the Civic Touring
-    if ((addr == 0x1A6) || (addr == 0x296)) {
+    if (!pcm_cruise && ((addr == 0x1A6) || (addr == 0x296))) {
       // check for button presses
       int button = (GET_BYTE(to_push, 0) & 0xE0U) >> 5;
       switch (button) {

tests/safety/common.py

@@ -60,6 +60,9 @@ def setUpClass(cls):
       cls.safety = None
       raise unittest.SkipTest
 
+    # make sure interceptor is detected
+    cls._rx(cls._interceptor_msg(0, 0x201)) # pylint: disable=no-value-for-parameter
+
   @abc.abstractmethod
   def _interceptor_msg(self, gas, addr):
     pass
@@ -261,7 +264,7 @@ class PandaSafetyTest(PandaSafetyTestBase):
 
   @classmethod
   def setUpClass(cls):
-    if cls.__name__ == "PandaSafetyTest":
+    if cls.__name__ == "PandaSafetyTest" or cls.__name__.endswith('Base'):
       cls.safety = None
       raise unittest.SkipTest
 

tests/safety/test_honda.py

@@ -1,7 +1,8 @@
 #!/usr/bin/env python3
 import unittest
-from typing import Optional
 import numpy as np
+from typing import Optional
+
 from panda import Panda
 from panda.tests.safety import libpandasafety_py
 import panda.tests.safety.common as common
@@ -16,30 +17,20 @@ class Btn:
 HONDA_BOSCH = 1
 
 
-class TestHondaSafetyBase(common.PandaSafetyTest):
-  MAX_BRAKE: float = 255
-  PT_BUS: Optional[int] = None  # must be set when inherited
-  STEER_BUS: Optional[int] = None  # must be set when inherited
+# Honda safety has several different configurations tested here:
+#  * Nidec
+#    * normal
+#    * alt SCM messages
+#    * interceptor
+#    * interceptor with alt SCM messages
+#  * Bosch
+#    * Bosch with Longitudinal Support
 
-  STANDSTILL_THRESHOLD = 0
-  RELAY_MALFUNCTION_ADDR = 0xE4
-  RELAY_MALFUNCTION_BUS = 0
-  FWD_BUS_LOOKUP = {0: 2, 2: 0}
 
-  cnt_speed = 0
-  cnt_gas = 0
-  cnt_button = 0
-  cnt_brake = 0
-  cnt_acc_state = 0
-
-  @classmethod
-  def setUpClass(cls):
-    if cls.__name__ == "TestHondaSafetyBase":
-      cls.packer = None
-      cls.safety = None
-      raise unittest.SkipTest
+class HondaButtonEnableBase(common.PandaSafetyTest):
+  # pylint: disable=no-member,abstract-method
 
-  # override these inherited tests. honda doesn't use pcm enable
+  # override these inherited tests since we're using button enable
   def test_disable_control_allowed_from_cruise(self):
     pass
 
@@ -49,44 +40,8 @@ def test_enable_control_allowed_from_cruise(self):
   def test_cruise_engaged_prev(self):
     pass
 
-  def _pcm_status_msg(self, enable):
-    pass
-
-  def _speed_msg(self, speed):
-    values = {"XMISSION_SPEED": speed, "COUNTER": self.cnt_speed % 4}
-    self.__class__.cnt_speed += 1
-    return self.packer.make_can_msg_panda("ENGINE_DATA", self.PT_BUS, values)
-
-  def _acc_state_msg(self, main_on):
-    values = {"MAIN_ON": main_on, "COUNTER": self.cnt_acc_state % 4}
-    self.__class__.cnt_acc_state += 1
-    return self.packer.make_can_msg_panda("SCM_FEEDBACK", self.PT_BUS, values)
-
-  def _button_msg(self, buttons, main_on=False):
-    values = {"CRUISE_BUTTONS": buttons, "COUNTER": self.cnt_button % 4}
-    self.__class__.cnt_button += 1
-    return self.packer.make_can_msg_panda("SCM_BUTTONS", self.PT_BUS, values)
-
-  def _brake_msg(self, brake):
-    values = {"BRAKE_PRESSED": brake, "COUNTER": self.cnt_gas % 4}
-    self.__class__.cnt_gas += 1
-    return self.packer.make_can_msg_panda("POWERTRAIN_DATA", self.PT_BUS, values)
-
-  def _gas_msg(self, gas):
-    values = {"PEDAL_GAS": gas, "COUNTER": self.cnt_gas % 4}
-    self.__class__.cnt_gas += 1
-    return self.packer.make_can_msg_panda("POWERTRAIN_DATA", self.PT_BUS, values)
-
-  def _send_steer_msg(self, steer):
-    values = {"STEER_TORQUE": steer}
-    return self.packer.make_can_msg_panda("STEERING_CONTROL", self.STEER_BUS, values)
-
-  def _send_brake_msg(self, brake):
-    # must be implemented when inherited
-    raise NotImplementedError
-
   def test_buttons_with_main_off(self):
-    for btn in [Btn.SET, Btn.RESUME, Btn.CANCEL]:
+    for btn in (Btn.SET, Btn.RESUME, Btn.CANCEL):
       self.safety.set_controls_allowed(1)
       self._rx(self._acc_state_msg(False))
       self._rx(self._button_msg(btn, main_on=False))
@@ -116,16 +71,6 @@ def test_disengage_on_main(self):
     self._rx(self._acc_state_msg(False))
     self.assertFalse(self.safety.get_controls_allowed())
 
-  def test_disengage_on_brake(self):
-    self.safety.set_controls_allowed(1)
-    self._rx(self._brake_msg(1))
-    self.assertFalse(self.safety.get_controls_allowed())
-
-  def test_steer_safety_check(self):
-    self.safety.set_controls_allowed(0)
-    self.assertTrue(self._tx(self._send_steer_msg(0x0000)))
-    self.assertFalse(self._tx(self._send_steer_msg(0x1000)))
-
   def test_rx_hook(self):
 
     # TODO: move this test to common
@@ -151,8 +96,8 @@ def test_rx_hook(self):
     # reset wrong_counters to zero by sending valid messages
     for i in range(MAX_WRONG_COUNTERS + 1):
       self.__class__.cnt_speed += 1
-      self.__class__.cnt_gas += 1
       self.__class__.cnt_button += 1
+      self.__class__.cnt_powertrain_data += 1
       if i < MAX_WRONG_COUNTERS:
         self.safety.set_controls_allowed(1)
         self._rx(self._button_msg(Btn.SET))
@@ -207,7 +152,116 @@ def test_tx_hook_on_pedal_pressed(self):
           self._rx(self._gas_msg(0))
 
 
-class TestHondaNidecSafety(TestHondaSafetyBase, common.InterceptorSafetyTest):
+class HondaPcmEnableBase(common.PandaSafetyTest):
+  # pylint: disable=no-member,abstract-method
+
+  def test_buttons(self):
+    """
+      Buttons shouldn't do anything in this configuration,
+      since our state is tied to the PCM's cruise state.
+    """
+    for controls_allowed in (True, False):
+      for main_on in (True, False):
+        # not a valid state
+        if controls_allowed and not main_on:
+          continue
+
+        for btn in (Btn.SET, Btn.RESUME, Btn.CANCEL):
+          self.safety.set_controls_allowed(controls_allowed)
+          self._rx(self._acc_state_msg(main_on))
+          self._rx(self._button_msg(btn, main_on=main_on))
+          self.assertEqual(controls_allowed, self.safety.get_controls_allowed())
+
+
+class HondaBase(common.PandaSafetyTest):
+  MAX_BRAKE: float = 255
+  PT_BUS: Optional[int] = None  # must be set when inherited
+  STEER_BUS: Optional[int] = None  # must be set when inherited
+
+  STANDSTILL_THRESHOLD = 0
+  RELAY_MALFUNCTION_ADDR = 0xE4
+  RELAY_MALFUNCTION_BUS = 0
+  FWD_BUS_LOOKUP = {0: 2, 2: 0}
+
+  cnt_speed = 0
+  cnt_button = 0
+  cnt_brake = 0
+  cnt_powertrain_data = 0
+  cnt_acc_state = 0
+
+  @classmethod
+  def setUpClass(cls):
+    if cls.__name__ == "HondaBase":
+      cls.packer = None
+      cls.safety = None
+      raise unittest.SkipTest
+
+  def _powertrain_data_msg(self, cruise_on=None, brake_pressed=None, gas_pressed=None):
+    # preserve the state
+    if cruise_on is None:
+      # or'd with controls allowed since the tests use it to "enable" cruise
+      cruise_on = self.safety.get_cruise_engaged_prev() or self.safety.get_controls_allowed()
+    if brake_pressed is None:
+      brake_pressed = self.safety.get_brake_pressed_prev()
+    if gas_pressed is None:
+      gas_pressed = self.safety.get_gas_pressed_prev()
+
+    values = {
+      "ACC_STATUS": cruise_on,
+      "BRAKE_PRESSED": brake_pressed,
+      "PEDAL_GAS": gas_pressed,
+      "COUNTER": self.cnt_powertrain_data % 4
+    }
+    self.__class__.cnt_powertrain_data += 1
+    return self.packer.make_can_msg_panda("POWERTRAIN_DATA", self.PT_BUS, values)
+
+  def _pcm_status_msg(self, enable):
+    return self._powertrain_data_msg(cruise_on=enable)
+
+  def _speed_msg(self, speed):
+    values = {"XMISSION_SPEED": speed, "COUNTER": self.cnt_speed % 4}
+    self.__class__.cnt_speed += 1
+    return self.packer.make_can_msg_panda("ENGINE_DATA", self.PT_BUS, values)
+
+  def _acc_state_msg(self, main_on):
+    values = {"MAIN_ON": main_on, "COUNTER": self.cnt_acc_state % 4}
+    self.__class__.cnt_acc_state += 1
+    return self.packer.make_can_msg_panda("SCM_FEEDBACK", self.PT_BUS, values)
+
+  def _button_msg(self, buttons, main_on=False):
+    values = {"CRUISE_BUTTONS": buttons, "COUNTER": self.cnt_button % 4}
+    self.__class__.cnt_button += 1
+    return self.packer.make_can_msg_panda("SCM_BUTTONS", self.PT_BUS, values)
+
+  def _brake_msg(self, brake):
+    return self._powertrain_data_msg(brake_pressed=brake)
+
+  def _gas_msg(self, gas):
+    return self._powertrain_data_msg(gas_pressed=gas)
+
+  def _send_steer_msg(self, steer):
+    values = {"STEER_TORQUE": steer}
+    return self.packer.make_can_msg_panda("STEERING_CONTROL", self.STEER_BUS, values)
+
+  def _send_brake_msg(self, brake):
+    # must be implemented when inherited
+    raise NotImplementedError
+
+  def test_disengage_on_brake(self):
+    self.safety.set_controls_allowed(1)
+    self._rx(self._brake_msg(1))
+    self.assertFalse(self.safety.get_controls_allowed())
+
+  def test_steer_safety_check(self):
+    self.safety.set_controls_allowed(0)
+    self.assertTrue(self._tx(self._send_steer_msg(0x0000)))
+    self.assertFalse(self._tx(self._send_steer_msg(0x1000)))
+
+
+# ********************* Honda Nidec **********************
+
+
+class TestHondaNidecSafetyBase(HondaBase):
   TX_MSGS = [[0xE4, 0], [0x194, 0], [0x1FA, 0], [0x200, 0], [0x30C, 0], [0x33D, 0]]
   FWD_BLACKLISTED_ADDRS = {2: [0xE4, 0x194, 0x33D, 0x30C]}
 
@@ -216,6 +270,12 @@ class TestHondaNidecSafety(TestHondaSafetyBase, common.InterceptorSafetyTest):
 
   INTERCEPTOR_THRESHOLD = 344
 
+  @classmethod
+  def setUpClass(cls):
+    if cls.__name__ == "TestHondaNidecSafetyBase":
+      cls.safety = None
+      raise unittest.SkipTest
+
   def setUp(self):
     self.packer = CANPackerPanda("honda_civic_touring_2016_can_generated")
     self.safety = libpandasafety_py.libpandasafety
@@ -286,7 +346,26 @@ def test_tx_hook_on_interceptor_pressed(self):
       self.safety.set_gas_interceptor_detected(False)
 
 
-class TestHondaNidecAltSafety(TestHondaNidecSafety, common.InterceptorSafetyTest):
+
+class TestHondaNidecSafety(HondaPcmEnableBase, TestHondaNidecSafetyBase):
+  """
+    Covers the Honda Nidec safety mode
+  """
+
+
+class TestHondaNidecInterceptorSafety(HondaButtonEnableBase, TestHondaNidecSafety, common.InterceptorSafetyTest):
+  """
+    Covers the Honda Nidec safety mode with a gas interceptor
+  """
+  def setUp(self):
+    TestHondaNidecSafety.setUpClass()
+    common.InterceptorSafetyTest.setUpClass()
+
+
+class TestHondaNidecAltSafety(HondaPcmEnableBase, TestHondaNidecSafetyBase):
+  """
+    Covers the Honda Nidec safety mode with alt SCM messages
+  """
   def setUp(self):
     self.packer = CANPackerPanda("acura_ilx_2016_can_generated")
     self.safety = libpandasafety_py.libpandasafety
@@ -304,7 +383,33 @@ def _button_msg(self, buttons, main_on=False):
     return self.packer.make_can_msg_panda("SCM_BUTTONS", self.PT_BUS, values)
 
 
-class TestHondaBoschSafetyBase(TestHondaSafetyBase):
+class TestHondaNidecAltInterceptorSafety(HondaButtonEnableBase, TestHondaNidecSafety, common.InterceptorSafetyTest):
+  """
+    Covers the Honda Nidec safety mode with alt SCM messages and gas interceptor
+  """
+  def setUp(self):
+    self.packer = CANPackerPanda("acura_ilx_2016_can_generated")
+    self.safety = libpandasafety_py.libpandasafety
+    self.safety.set_safety_hooks(Panda.SAFETY_HONDA_NIDEC, Panda.FLAG_HONDA_NIDEC_ALT)
+    self.safety.init_tests_honda()
+    common.InterceptorSafetyTest.setUpClass()
+
+  def _acc_state_msg(self, main_on):
+    values = {"MAIN_ON": main_on, "COUNTER": self.cnt_acc_state % 4}
+    self.__class__.cnt_acc_state += 1
+    return self.packer.make_can_msg_panda("SCM_BUTTONS", self.PT_BUS, values)
+
+  def _button_msg(self, buttons, main_on=False):
+    values = {"CRUISE_BUTTONS": buttons, "MAIN_ON": main_on, "COUNTER": self.cnt_button % 4}
+    self.__class__.cnt_button += 1
+    return self.packer.make_can_msg_panda("SCM_BUTTONS", self.PT_BUS, values)
+
+
+
+# ********************* Honda Bosch **********************
+
+
+class TestHondaBoschSafetyBase(HondaBase):
   PT_BUS = 1
   STEER_BUS = 0
 
@@ -351,7 +456,10 @@ def test_alt_disengage_on_brake(self):
     self.assertTrue(self.safety.get_controls_allowed())
 
 
-class TestHondaBosch(TestHondaBoschSafetyBase):
+class TestHondaBoschSafety(HondaPcmEnableBase, TestHondaBoschSafetyBase):
+  """
+    Covers the Honda Bosch safety mode with stock longitudinal
+  """
   def setUp(self):
     super().setUp()
     self.safety.set_safety_hooks(Panda.SAFETY_HONDA_BOSCH, 0)
@@ -367,7 +475,10 @@ def test_spam_cancel_safety_check(self):
     self.assertTrue(self._tx(self._button_msg(Btn.RESUME)))
 
 
-class TestHondaBoschLongSafety(TestHondaBoschSafetyBase):
+class TestHondaBoschLongSafety(HondaButtonEnableBase, TestHondaBoschSafetyBase):
+  """
+    Covers the Honda Bosch safety mode with longitudinal control
+  """
   NO_GAS = -30000
   MAX_GAS = 2000
   MAX_BRAKE = -3.5