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

FOC minor optimizations #467

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kubark42
Copy link
Contributor

@kubark42 kubark42 commented Apr 10, 2022

  1. Several precalculations
  2. Replace some divisions by multiplication
  3. Fix some ;; typos
  4. Abstract out a magic number
  5. Eliminate some instances of volatile keyword.

Saves a bit of flash as well:

Before

   text	   data	    bss	    dec	    hex	filename
 396140	   3828	 160256	 560224	  88c60	/Users/kenz/Documents/vesc/build/100_250/100_250.elf

After

   text	   data	    bss	    dec	    hex	filename
 396076	   3828	 160256	 560160	  88c20	/Users/kenz/Documents/vesc/build/100_250/100_250.elf

Tested and returns sane sys ident. Runs well in

@@ -2541,8 +2535,10 @@ void mcpwm_foc_adc_int_handler(void *p, uint32_t flags) {

UTILS_LP_FAST(motor_now->m_motor_state.v_bus, GET_INPUT_VOLTAGE(), 0.1);

volatile float enc_ang = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vedderb I can't see why this volatile would have been necessary. The variables are used immediately after in the function, so there's not opportunity for things to be updated elsewhere. The functions themselves shouldn't be optimized away, not unless the underlying variables they're using should somehow themselves be volatile.

foc_math.c Outdated
motor->p_v2_v3_inv_avg_half = (0.5 / motor->p_lq + 0.5 / motor->p_ld) * 0.9; // With the 0.9 we undo the adjustment from the detection
motor->p_inv_inv_ld_lq = 1.0 / (1.0 / motor->p_lq - 1.0 / motor->p_ld);

motor->p_inv_lo_current_max = 1.0 / conf_now->lo_current_max;
Copy link
Owner

Choose a reason for hiding this comment

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

The lo-limits are special as they are updated in mc_interface when the different limits are hit, so they cannot be precalculated here. There should probably be some comment explaining that in the struct. This works because mcpwm_foc does not have it's own instance of the configuration - it is mc_interface that owns it and mcpwm_foc only has a pointer to it. The mcpwm_foc_set_configuration-function is only used to tell mcpwm_foc that the configuration has changed so that some checks and updates can be done on it if needed. This function is not called when the lo_limits are updated.

It might be worth rethinking this part at some point if there is a simpler/better solution.

Copy link
Contributor Author

@kubark42 kubark42 Apr 11, 2022

Choose a reason for hiding this comment

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

It might be worth moving things which are not explicitly user configuration out of the conf object and into a separate struct. The memory could be allocated in a similar way, so that there is only one instance shared globally. The upside is that VESC can promise that if a variable is in conf that it cannot be changed by anything other than an external program.

I see how this variable is set in update_override_limits(). I think it would make sense to have these variables in a globally accessible struct which is written through setters. Modules could subscribe to update notifications, so they would recalculate only when the value has changed.

@kubark42 kubark42 marked this pull request as ready for review April 11, 2022 17:56
float d_gain_scale = 1.0;
if (conf_now->foc_d_gain_scale_start < 0.99) {
float max_mod_norm = fabsf(state_m->duty_now / max_duty);
Copy link
Owner

Choose a reason for hiding this comment

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

The name max_mod_norm makes a lot of sense here. After the changes it is much more difficult to see what this section does.

Copy link
Contributor Author

@kubark42 kubark42 Apr 11, 2022

Choose a reason for hiding this comment

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

Can we address that with comments? TBH, max_mod_norm didn't speak to me, and even after working with the math it's not clear to me what this section is doing. The math is clear, but the why of it all is obscure without a deeper understanding of the code. For instance, d_gain_scale is set and modified up to three times, and why that is good, desirable behavior is left as an exercise to the reader.

It's also not clear if this section runs once every never, or it runs almost every loop. Answering those questions might help make it clear if it's worth saving a division or not.

Copy link
Owner

Choose a reason for hiding this comment

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

d_gain_scale is a scaling factor of the d-axis controller gain (as the name suggests). max_mod_norm is a normalized maximum modulation (duty cycle), meaning a value between 0 and 1 that maps where abs_duty is between 0 and max_duty.

The section scales down the D axis gain when max_mod_norm is above foc_d_gain_scale_start down to foc_d_gain_scale_max_mod when max_mod_norm is 1. The reason for doing that is that one way of making the controller more stable when you hit max modulation is to scale down the d axis gain compared to the q axis gain. This section runs every iteration as the motor approaches full speed as that is when the modulation approaches maximum.

Before it was quite easy for me to see what it does, but the changes make it quite a bit more obscure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants