-
Notifications
You must be signed in to change notification settings - Fork 4
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
Launch Control Palmer #23
base: roman
Are you sure you want to change the base?
Conversation
This reverts commit 4ed266b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some stylistic things
src/vcu/inc/controls.h
Outdated
@@ -14,6 +14,15 @@ | |||
#define MAX_ACCEL_VAL 1000 | |||
#define MIN_ACCEL_VAL 0 | |||
|
|||
// Launch control contants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error
src/vcu/inc/controls.h
Outdated
#define LC_cGR 347 // Gear ratio times 100 | ||
#define LC_ACCEL_BEGIN 950 // 95% | ||
#define LC_ACCEL_RELEASE 50 // 5% | ||
#define LC_BRAKE_BEGIN 100 // We want a lower threshold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still want this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this comment was to indicate that we want lower threshold than normal. I can fix it to make it clearer
src/vcu/src/controls.c
Outdated
} else { // Only use limits and launch control when we're not doing regen | ||
// Even if we are doing launch control, calculate limits for loggind also so that we are not doing | ||
// launch control when we're supposed to be limiting | ||
|
||
int32_t voltage_limited_torque = get_voltage_limited_torque(torque_command); | ||
// static uint32_t last_vt = 0; | ||
// if (HAL_GetTick() - last_vt > 10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove this commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also there is a typo here "loggind"
src/vcu/src/controls.c
Outdated
@@ -90,13 +103,14 @@ void execute_controls(void) { | |||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: remove commented code
can_spec_my18.yml
Outdated
position: 0 | ||
length: 1 | ||
c_type: bool | ||
launch_ctrl_slip_ratio: # Ranges from 105 to 115 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it range between these values? Will this ever change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talk about this is confluence docs. This number was something that Luis suggested but could use some more backing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, might be good to at least show how you might come up with this on confluence.
src/vcu/src/controls.c
Outdated
@@ -22,12 +28,16 @@ void init_controls_defaults(void) { | |||
control_settings.volt_lim_min_gain = 0; | |||
control_settings.volt_lim_min_voltage = 300; | |||
control_settings.torque_temp_limited = false; | |||
|
|||
lc_settings.using_launch_ctrl = false; | |||
lc_settings.slip_ratio = 112; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would recommend bringing 112 out to a DEFAULT_SLIP_RATIO constant
src/vcu/src/controls.c
Outdated
if (pedalbox_min(accel) < LC_ACCEL_BEGIN) { | ||
printf("[LAUNCH CONTROL ERROR] Accel min (%d) too low\r\n", pedalbox_min(accel)); | ||
return true; | ||
} else if (pedalbox.brake_2 > LC_BRAKE_BEGIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only brake_2? Don't we want the max of the 2 brakes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have learned the reason, but would still like a comment here explaining why we only need to check brake 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't remember the reason myself...should definitely go in a comment
src/vcu/src/controls.c
Outdated
return false; | ||
} | ||
|
||
void set_lc_state_before() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function ever used? Can we delete it?
src/vcu/src/controls.c
Outdated
} | ||
break; | ||
case SPEEDING_UP: | ||
if (torque_command < 1000) sendSpeedCmdMsg(500, torque_command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In confluence, we have "LC_SPEEDING_UP_TORQUE", but here it is not a constant. Can we fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can we make 300 and 500 some sort of constants too and add them to the documentation in confluence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also technically the if statement here with < 1000 should probably be reflected in the state machine somehow. I think you intended to back off on the torque command if we start to exceed 1000, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah I realized I had made it a constant and forgot to push it. I'll fix the others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yeah my intention we to command max(commanded torque, 1000)
Hey Dani, I think in terms of state machine logic, this looks good. Nick and I both went through the diagrams in confluence and it seems to match. Some stylistic things are in order. Also make sure to pull our changes before resolving issues. Also, nice work on the diagrams! Super easy to read. |
This PR has been open for too long, @daniwhite . |
This pull request updates the VCU code to include launch control for testing at palmer on Sunday Sept. 9.