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.

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

Bug in new brake disengage logic #63

Open
d-loret opened this issue Dec 10, 2022 · 4 comments
Open

Bug in new brake disengage logic #63

d-loret opened this issue Dec 10, 2022 · 4 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@d-loret
Copy link
Contributor

d-loret commented Dec 10, 2022

Suggestion to simplify handling of brake disengage in Actuator's state machine

I might be missing something, but I also wonder if we considered just adding one additional state before any of the profile mode states. Something analogous to the HOLDING state but before you enter any of the profile states, say WAIT_BRAKE_DISENGAGE. In HandleNewProfX() you could store the command and what state you should transition after the wait, and transition to WAIT_BRAKE_DISENGAGE. In ProcessWaitBrakeDisengage you transition to the stored prof state immediately if servo_enabled is true. Otherwise, you wait for the max possible brake time (the current ~1s). And then just generate the trapezoid in ProcessProfX() if it is executing for the first time (e.g. maybe indicated by a flag that HandleNewProfX() sets).

I say this because right now you could go from, for example, PROF_POS -> PROF_POS_DISENGAGING -> PROF_POS, which is a little confusing.

@d-loret d-loret added bug Something isn't working enhancement New feature or request labels Dec 10, 2022
@d-loret
Copy link
Contributor Author

d-loret commented Dec 10, 2022

@alex-brinkman

@alex-brinkman
Copy link
Contributor

Mind breaking these into distinct issues so they can be closed independently

@d-loret d-loret changed the title Bug in new brake disengage logic and message printing issues Bug in new brake disengage logic Jan 30, 2023
@d-loret
Copy link
Contributor Author

d-loret commented Jan 30, 2023

Updated issue and split printing issue into separatae ticket: #75

@alex-brinkman
Copy link
Contributor

I believe this is solved now since the EPD actuator class refactor was merged:

 // Only transition to disengaging if it is needed (i.e. brakes are still
  // engaged)
  if (!state_->gold_actuator_state.servo_enabled) {
    TransitionToState(ACTUATOR_SMS_PROF_POS_DISENGAGING);
    last_cmd_ = cmd;
    return true;
  }
  ...

  TransitionToState(ACTUATOR_SMS_PROF_POS);

@d-loret can you confirm and close?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants