Modular Assistive Driving System (MADS) #52
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ts on replay drive, and optimizations
…n button pressed interchangeably
…at the end of the tests
…afety code The commit performs a comprehensive revision of the safety replay script, specifically focusing on introducing debug variables and enhancing the logging capabilities for improved debugging. Furthermore, changes were made to the Honda safety code. The test helpers within libpanda were also expanded for inclusion of additional test conditions.
…s 'safety_mads.h' The Sunnypilot's 'safety_mads.h' file has been updated to include 'ACC_MAIN_OFF' as a new cause for disconnection in the 'DisengageReason' enumeration. If an 'acc_main_off' signal is received, the 'mads_exit_controls' function halts all requests for lateral control engagement. Additionally, the status of 'controls_requested_lat' now mirrors 'controls_allowed_lat' after a button press.
Renamed StateTransition to EdgeTransition for clarity and updated related logic. Introduced event handlers for button presses and ACC state changes, reducing duplicated control flow code. Improved encapsulation and maintainability by restructuring state update functions.
Removed redundant event handler functions and unnecessary timestamp fields to streamline the code. Simplified button and binary state updates by integrating logic directly into transition checks. Commented out unused fields
The logic for setting the `controls_requested_lat` variable in safety_mads.h has been refined. Previously, it switched state based on the current value of `controls_allowed_lat`. Now, it also takes into account the current state of `acc_main`, ensuring a more nuanced control request mechanism that accounts for different operational scenarios.
Refactor button state transitions to better handle lateral control requests when ACC is active. Ensure controls are correctly disengaged under specific conditions, by setting `controls_requested_lat` more reliably during state transitions. This change improves safety by preventing inadvertent disengagement when ACC is not active.
This commit introduces a new test to ensure that controls remain enabled when the LKAS/LFA button is pressed while ACC main is on. It checks that LKAS button operations don't interfere with control permissions in this specific configuration, improving test coverage and preventing potential safety issues.
Enhanced mismatch detection logic by tracking cases where 'controls_allowed' is true while 'controls_allowed_lat' is false, updating the script to print relevant debug information. Additionally, changed the data type of 'mads_acc_main' and 'mads_acc_main_prev' from int to bool for improved type accuracy and consistency.
…at_active() which has the final word on whether we can allow lat or not.
… disengagement for toyota
…BRAKE' The commit modifies the usage of the 'DISABLE_DISENGAGE_LATERAL_ON_BRAKE' variable globally and replaces it with 'DISENGAGE_LATERAL_ON_BRAKE'. This change promotes correct and clear semantics, since the variable now indicates a state rather than the negation of a state.
devtekve
reviewed
Dec 12, 2024
board/safety.h
Outdated
| @@ -114,7 +114,7 @@ static const safety_hooks *current_hooks = &nooutput_hooks; | |||
| safety_config current_safety_config; | |||
|
|
|||
| static bool is_lat_active(void) { | |||
| return controls_allowed || mads_is_lateral_control_allowed_by_mads(); | |||
| return (controls_allowed && !mads_system_enabled()) || mads_is_lateral_control_allowed_by_mads(); | |||
There was a problem hiding this comment.
Suggested change
| return (controls_allowed && !mads_system_enabled()) || mads_is_lateral_control_allowed_by_mads(); | |
| return controls_allowed || mads_is_lateral_control_allowed_by_mads(); |
This is not supposed to happen. Do the whole "decision" if lat is active inside the MADS method instead
sunnyhaibin
commented
Dec 12, 2024
board/sunnypilot/safety_mads.h
Outdated
Comment on lines
155
to
159
|
|
||
| // Unified Engagement Mode | ||
| if ((m_mads_state.op_controls_allowed.transition == MADS_EDGE_RISING) && m_mads_state.unified_engagement_mode) { | ||
| m_mads_state.controls_requested_lat = true; | ||
| } |
There was a problem hiding this comment.
Suggested change
| // Unified Engagement Mode | |
| if ((m_mads_state.op_controls_allowed.transition == MADS_EDGE_RISING) && m_mads_state.unified_engagement_mode) { | |
| m_mads_state.controls_requested_lat = true; | |
| } |
board/sunnypilot/safety_mads.h
Outdated
| // Use buttons or main cruise state transition properties to request lateral control | ||
| static void m_mads_update_state(void) { | ||
| // Main cruise | ||
| if ((m_mads_state.acc_main.transition == MADS_EDGE_RISING) && m_mads_state.main_cruise_allowed) { |
There was a problem hiding this comment.
Suggested change
| if ((m_mads_state.acc_main.transition == MADS_EDGE_RISING) && m_mads_state.main_cruise_allowed) { | |
| if (m_mads_state.acc_main.transition == MADS_EDGE_RISING) { |
board/sunnypilot/safety_mads.h
Outdated
Comment on lines
148
to
154
| if ((m_mads_state.mads_button.transition == MADS_EDGE_RISING) && (*m_mads_state.acc_main.current || m_mads_state.always_allow_mads_button)) { | ||
| m_mads_state.controls_requested_lat = !m_mads_state.controls_allowed_lat; | ||
|
|
||
| if (!m_mads_state.controls_requested_lat) { | ||
| mads_exit_controls(MADS_DISENGAGE_REASON_BUTTON); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| if ((m_mads_state.mads_button.transition == MADS_EDGE_RISING) && (*m_mads_state.acc_main.current || m_mads_state.always_allow_mads_button)) { | |
| m_mads_state.controls_requested_lat = !m_mads_state.controls_allowed_lat; | |
| if (!m_mads_state.controls_requested_lat) { | |
| mads_exit_controls(MADS_DISENGAGE_REASON_BUTTON); | |
| } | |
| } | |
| if (m_mads_state.mads_button.transition == MADS_EDGE_RISING) { | |
| m_mads_state.controls_requested_lat = true; | |
| if (!m_mads_state.controls_requested_lat) { | |
| mads_exit_controls(MADS_DISENGAGE_REASON_BUTTON); | |
| } | |
| } |
|
Need to fix tests with the new changes. |
The mads_state_update function has been refactored to improve its readability and performance. The changes include the modification of how binary state and button state are updated, along with how vehicle movement and braking are evaluated. Additionally, the acc_main, mads_button, and braking members of the MADSState struct now store values instead of pointers to values, which should boost execution speed.
sunnyhaibin
force-pushed
the
mads-new-diff-2
branch
from
29966f3 to
cf115df
Compare
|
Superseded by #57 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reimplementation of
TODO
InheritMadsCommonNonPCMMainCruiseBasein longitudinal unit tests