Skip to content

Fix lack of test coverage for angle_meas scaling in subaru.h #3084

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

Closed
jacobwaller wants to merge 4 commits into from
Closed

Fix lack of test coverage for angle_meas scaling in subaru.h #3084

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5473f2d
Fix mutant tests & demonstrate passing
Jan 29, 2026
0747932
Use cleaner test
Jan 29, 2026
cbe6a93
unneeded code
Jan 29, 2026
c9983d7
DRY
Jan 31, 2026
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
2 changes: 1 addition & 1 deletion opendbc/safety/modes/subaru.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ static void subaru_rx_hook(const CANPacket_t *msg) {

int angle_meas_new = (GET_BYTES(msg, 4, 2) & 0xFFFFU);
// convert Steering_Torque -> Steering_Angle to centidegrees, to match the ES_LKAS_ANGLE angle request units
angle_meas_new = ROUND(to_signed(angle_meas_new, 16) * -2.17);
angle_meas_new = ROUND(-2.17 * to_signed(angle_meas_new, 16));
Copy link
Copy Markdown
Contributor Author

@jacobwaller jacobwaller Jan 29, 2026

Choose a reason for hiding this comment

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

This change is not needed, but is there to demonstrate that the tests added cover the mutant tests for this line

Feel free to accept this suggestion to undo the change:

Suggested change
angle_meas_new = ROUND(-2.17 * to_signed(angle_meas_new, 16));
angle_meas_new = ROUND(to_signed(angle_meas_new, 16) * -2.17);

update_sample(&angle_meas, angle_meas_new);
}

Expand Down
25 changes: 25 additions & 0 deletions opendbc/safety/tests/test_subaru.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ def gen2_long_additional_tx_msgs():
def fwd_blacklisted_addr(lkas_msg=SubaruMsg.ES_LKAS):
return {SUBARU_CAM_BUS: [lkas_msg, SubaruMsg.ES_DashStatus, SubaruMsg.ES_LKAS_State, SubaruMsg.ES_Infotainment]}

def get_raw16_from_canpacket(msg) -> int:
# msg is cdata 'CANPacket_t *'
b4 = int(msg[0].data[4])
b5 = int(msg[0].data[5])
return b4 | (b5 << 8)

def to_signed16(x: int) -> int:
x &= 0xFFFF
return (x ^ 0x8000) - 0x8000

class TestSubaruSafetyBase(common.CarSafetyTest):
FLAGS = 0
Expand All @@ -76,6 +85,7 @@ class TestSubaruSafetyBase(common.CarSafetyTest):
DEG_TO_CAN = 100

INACTIVE_GAS = 1818
ANGLE_MEAS_SCALE = -2.17

def setUp(self):
self.packer = CANPackerSafety("subaru_global_2017_generated")
Expand Down Expand Up @@ -111,6 +121,21 @@ def _pcm_status_msg(self, enable):
values = {"Cruise_Activated": enable}
return self.packer.make_can_msg_safety("CruiseControl", self.ALT_MAIN_BUS, values)

def test_angle_meas_scaling(self):
Copy link
Copy Markdown
Contributor

@sshane sshane Jan 29, 2026

Choose a reason for hiding this comment

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

Don't we have a test to make sure angle_meas_new is parsed properly in common.py?

Copy link
Copy Markdown
Contributor Author

@jacobwaller jacobwaller Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not really sure if that's the intention of those tests as they don't catch the mutation:
Changing only one line to trigger a diff in opendbc master:

❯ git diff
diff --git a/opendbc/safety/modes/subaru.h b/opendbc/safety/modes/subaru.h
index 9b01d39d..6871cf06 100644
--- a/opendbc/safety/modes/subaru.h
+++ b/opendbc/safety/modes/subaru.h
@@ -101,7 +101,7 @@ static void subaru_rx_hook(const CANPacket_t *msg) {
 
     int angle_meas_new = (GET_BYTES(msg, 4, 2) & 0xFFFFU);
     // convert Steering_Torque -> Steering_Angle to centidegrees, to match the ES_LKAS_ANGLE angle request units
-    angle_meas_new = ROUND(to_signed(angle_meas_new, 16) * -2.17);
+    angle_meas_new = ROUND(-2.17 * to_signed(angle_meas_new, 16));
     update_sample(&angle_meas, angle_meas_new);
   }

Results in the mutation tests failing as before:

/Users/jacobwaller/Documents/personal/comma/opendbc/opendbc/safety/modes/subaru.h:104:34: warning: Survived: Replaced * with / [cxx_mul_to_div]
    angle_meas_new = ROUND(-2.17 * to_signed(angle_meas_new, 16));
                                 ^

Copy link
Copy Markdown
Contributor Author

@jacobwaller jacobwaller Jan 29, 2026

Choose a reason for hiding this comment

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

Oh I see, that steering angle test is only being done for angle-based cars

class AngleSteeringSafetyTest(VehicleSpeedSafetyTest):

... other code

  def test_steering_angle_measurements(self):
    self._common_measurement_test(self._angle_meas_msg, -self.STEER_ANGLE_MAX, self.STEER_ANGLE_MAX, self.DEG_TO_CAN,
                                  self.safety.get_angle_meas_min, self.safety.get_angle_meas_max)

Copy link
Copy Markdown
Contributor Author

@jacobwaller jacobwaller Jan 29, 2026

Choose a reason for hiding this comment

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

Alright @sshane I updated to use the helper in common.py

One note -- I wanted to just add this to all the cars, but not everything has an easily accessible STEER_ANGLE_MAX or a way to grab the angle_meas. In fact, from what I can see, some cars don't even have angle_meas in safety (hyundai.h for example)? I might be missing something there though.

As a workaround, I just added it in test_subaru.py

Copy link
Copy Markdown
Contributor

@sshane sshane Jan 29, 2026

Choose a reason for hiding this comment

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

If the safety test/mode is for angle, it should subclass from that base angle test

Copy link
Copy Markdown
Contributor Author

@jacobwaller jacobwaller Jan 30, 2026

Choose a reason for hiding this comment

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

The case we're looking at here is for torque-based, I'm hesitant to subclass from that because subarus don't support angle based steering yet.

Copy link
Copy Markdown
Contributor Author

@jacobwaller jacobwaller Jan 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @sshane , sorry to ping you again :). I refactored the scaling check into a dedicated test (AngleScalingSafetyTest) and made both AngleSteeringSafetyTest and TestSubaruSafetyBase inherit from it.

This should keep these tests a little more DRY, let me know if this is kind of what you were thinking

for angle in [0, 1, 10, 50, 90, 100, 180, 500]:
msg = None
# Fill the 6-sample buffer with nearly identical samples
for _ in range(6):
msg = self._angle_meas_msg(angle)
self._rx(msg)

raw16 = get_raw16_from_canpacket(msg)
signed = to_signed16(raw16)
expected = int(round(self.ANGLE_MEAS_SCALE * signed))

self.assertEqual(self.safety.get_angle_meas_min(), expected)
self.assertEqual(self.safety.get_angle_meas_max(), expected)


class TestSubaruStockLongitudinalSafetyBase(TestSubaruSafetyBase):
def _cancel_msg(self, cancel, cruise_throttle=0):
Expand Down