commaai / commaai/opendbc

Tests don't run correctly if PCM and gas are in same message

Closed
#1,769 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Python
Stars
3.4k
Forks
2.3k
Avg merge
3d 2h
Merged PRs (30d)
38

Description

On Toyota, the PCM status message (`PCM_CRUISE`) also has the gas pressed bit, so if you for example change some code that should fail a test:

```c
if (!cruise_engaged) {
controls_allowed = 0;
}
```

to

```c
if (!cruise_engaged) {
controls_allowed = 1;
}
```

it passes, as `generic_rx_checks` thinks the gas is pressed and just sets controls_allowed to zero for you.

I also had to be careful in the Bolt camera harness PR which has gas and PCM cruise in the same message: https://github.com/commaai/panda/pull/962

I think the real solution here would be to have tests that only ensure that each safety model sets all required global variables correctly, then we have one test that tests all the behavior (that's independent of all the car safety models).

I'm just not sure if it would be considered a regression to our testing, as now we test fully end to end (CAN message to behavior). This proposal would test CAN message to setting variables/some safety state, then the safety state to safety behavior and enforcement.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reviewing generic_rx_checks and the Bolt camera harness PR (#962), focusing on how PCM_CRUISE and gas bits are handled in the same message. Define tests that verify safety models set required global variables and separate tests that verify behavior from that safety state; done means the accidental pass described here is prevented without losing coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
testing-qa
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.