Skip to content
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

Only run tx hooks on msgs in safety_config.tx_msgs. #1851

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+17 −11
Draft

Only run tx hooks on msgs in safety_config.tx_msgs. #1851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b00710a
Only run tx hooks on msgs in safety_config.tx_msgs.
ccdunder Feb 26, 2025
fd667a6
'fix' tests
ccdunder Feb 27, 2025
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
14 changes: 9 additions & 5 deletions opendbc/safety/safety.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ bool safety_rx_hook(const CANPacket_t *to_push) {
return valid;
}

static bool msg_allowed(const CANPacket_t *to_send, const CanMsg msg_list[], int len) {
static bool tx_msg_safety_check(const CANPacket_t *to_send, const CanMsg msg_list[], int len) {
int addr = GET_ADDR(to_send);
int bus = GET_BUS(to_send);
int length = GET_LEN(to_send);
Expand All @@ -238,13 +238,17 @@ static bool msg_allowed(const CANPacket_t *to_send, const CanMsg msg_list[], int
}

bool safety_tx_hook(CANPacket_t *to_send) {
bool whitelisted = msg_allowed(to_send, current_safety_config.tx_msgs, current_safety_config.tx_msgs_len);
bool allowed = tx_msg_safety_check(to_send, current_safety_config.tx_msgs, current_safety_config.tx_msgs_len);
if ((current_safety_mode == SAFETY_ALLOUTPUT) || (current_safety_mode == SAFETY_ELM327)) {
whitelisted = true;
allowed = true;
}

const bool safety_allowed = current_hooks->tx(to_send);
return !relay_malfunction && whitelisted && safety_allowed;
bool safety_allowed = false;
if (allowed) {
safety_allowed = current_hooks->tx(to_send);
}

return !relay_malfunction && allowed && safety_allowed;
}

int safety_fwd_hook(int bus_num, int addr) {
Expand Down
3 changes: 3 additions & 0 deletions opendbc/safety/safety/safety_defaults.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ static safety_config nooutput_init(uint16_t param) {
return (safety_config){NULL, 0, NULL, 0};
}

// GCOV_EXCL_START
// cppcheck-suppress misra-c2012-2.1; Unreachable by design (no msgs match).
static bool nooutput_tx_hook(const CANPacket_t *to_send) {
UNUSED(to_send);
return false;
}
// GCOV_EXCL_STOP

static int default_fwd_hook(int bus_num, int addr) {
UNUSED(bus_num);
Expand Down
3 changes: 3 additions & 0 deletions opendbc/safety/safety/safety_hyundai_canfd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,12 @@ static bool hyundai_canfd_tx_hook(const CANPacket_t *to_send) {
violation |= longitudinal_accel_checks(desired_accel_val, HYUNDAI_LONG_LIMITS);
} else {
// only used to cancel on here
// GCOV_EXCL_START
// TODO: Cover this with tests.
Copy link
Contributor

@sshane sshane Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure our test isn't accidentally sending this on the wrong bus? We should definitely hit this

Copy link
Author

@ccdunder ccdunder Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

oh yeah that's right, rings a bell. the first time i noticed this tx_hooks behavior was after refactoring hyundai_canfd tests and being puzzled that one of the buses was wrong. great observation! i'll look into it.

if ((desired_accel_raw != 0) || (desired_accel_val != 0)) {
violation = true;
}
// GCOV_EXCL_STOP
}

if (violation) {
Expand Down
8 changes: 2 additions & 6 deletions opendbc/safety/safety/safety_rivian.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,8 @@ static bool rivian_tx_hook(const CANPacket_t *to_send) {

// Longitudinal control
if (addr == 0x160) {
if (rivian_longitudinal) {
int raw_accel = ((GET_BYTE(to_send, 2) << 3) | (GET_BYTE(to_send, 3) >> 5)) - 1024U;
if (longitudinal_accel_checks(raw_accel, RIVIAN_LONG_LIMITS)) {
tx = false;
}
} else {
int raw_accel = ((GET_BYTE(to_send, 2) << 3) | (GET_BYTE(to_send, 3) >> 5)) - 1024U;
if (longitudinal_accel_checks(raw_accel, RIVIAN_LONG_LIMITS)) {
Comment on lines +83 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because 0x160 is now blocked since it's not in the tx messages?

Copy link
Author

Choose a reason for hiding this comment

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

correct. the case where addr == 0x160 && !rivian_longitudinal becomes unreachable with this change.

tx = false;
}
}
Expand Down
Loading