Optimise set_motor_speed function

[VedantParanjape]

Current implementation of this function is very large and a lot of code is repeated, It is a must to optimise this function

https://github.com/SRA-VJTI/sra-board-component/blob/213ae2f7165f70d4d53d17630f6bee5ad46a09bd/src/motor_driver.c#L209

[OSSome01]

We can use macros like MOTOR_A and MOTOR_B for parallel mode. And MOTOR_A_0, MOTOR_A_1.... can be used for normal mode. This will reduce some lines of code, but will help in increasing readability significantly. @VedantParanjape @laukik-hase

[VedantParanjape]

I think this will rather increase lines of code, as in parallel mode I am accepting either MOTOR_A_0 or MOTOR_A_1 for parallel mode.

[OSSome01]

I don't think there will be any change in the number of lines, but for sure the code will be much readable. There are too many if conditions where the mode is checked

[VedantParanjape]

The mode check is necessary as every different mode requires different mcpwm timer.

If possible please make the change you proposed on your fork for better understanding.

[udit7395]

Is it possible to have individual functions for normal and parallel motor functions?

[VedantParanjape]

@udit7395 Yes it is possible.

But having a single function seems neat ! Rather than having separate for different modes. Since, I am storing the mode of motor drivers, so seems good to automate that part.

Example for normal mode: set_motor_speed(MOTOR_A_0, MOTOR_FORWARD, 60);

For Parallel mode: set_motor_speed(MOTOR_A_0, MOTOR_FORWARD, 60);

or

set_motor_speed(MOTOR_A_1, MOTOR_FORWARD, 60);

Both will have the same effect for parallel case

[udit7395]

@VedantParanjape okay. How about this, you create individual functions and call them in the set_motor_speed. This way for the user it is still set_motor_speed and internally the code is more readable. Is this possible ?

[VedantParanjape]

@udit7395 Okay this can be done

if (direction == MOTOR_FORWARD)
            {
                mcpwm_set_duty(unit_val, MCPWM_TIMER_2, MCPWM_OPR_A, 0);
                mcpwm_set_duty_type(unit_val, MCPWM_TIMER_2, MCPWM_OPR_A, MCPWM_DUTY_MODE_0); 
                mcpwm_set_duty(unit_val, MCPWM_TIMER_2, MCPWM_OPR_B, duty_cycle);
                mcpwm_set_duty_type(unit_val, MCPWM_TIMER_2, MCPWM_OPR_B, MCPWM_DUTY_MODE_0); 

                return ESP_OK; 
            }
            else if (direction == MOTOR_BACKWARD)
            {
                mcpwm_set_duty(unit_val, MCPWM_TIMER_2, MCPWM_OPR_A, duty_cycle);
                mcpwm_set_duty_type(unit_val, MCPWM_TIMER_2, MCPWM_OPR_A, MCPWM_DUTY_MODE_0); 
                mcpwm_set_duty(unit_val, MCPWM_TIMER_2, MCPWM_OPR_B, 0);
                mcpwm_set_duty_type(unit_val, MCPWM_TIMER_2, MCPWM_OPR_B, MCPWM_DUTY_MODE_0);

                return ESP_OK; 
            }
            else if (direction == MOTOR_STOP)
            {
                mcpwm_set_duty(unit_val, MCPWM_TIMER_2, MCPWM_OPR_A, 0);
                mcpwm_set_duty_type(unit_val, MCPWM_TIMER_2, MCPWM_OPR_A, MCPWM_DUTY_MODE_0); 
                mcpwm_set_duty(unit_val, MCPWM_TIMER_2, MCPWM_OPR_B, 0);
                mcpwm_set_duty_type(unit_val, MCPWM_TIMER_2, MCPWM_OPR_B, MCPWM_DUTY_MODE_0);

                return ESP_OK; 
            }
            else
            {
                ESP_LOGE(TAG_MOTOR_DRIVER, "invalid motor direction selected");
                return ESP_FAIL;
            }

The following code is being repeated and can be put into a function.

[udit7395]

@VedantParanjape Idk whether you or Omkar would be taking up this issue but implement and see if it helps. Also you could further reduce it by combining set duty and duty type in one function.

[VedantParanjape]

Also you could further reduce it by combining set duty and duty type in one function.

This won't help much, maybe increase readability.

The if else loop for direction can be reduced using ternary operators, I'll look into it.

[laukik-hase]

@VedantParanjape Idk whether you or Omkar would be taking up this issue but implement and see if it helps. Also you could further reduce it by combining set duty and duty type in one function.

This will help make code readable and less error prone. Also, switch case looks neat than an else if ladder. Can we replace the else if ladder by a function which takes the MCPWM timer, unit, operator as parameter?

@udit7395 Do you remember that we used the go to statement for making error handling neat. Can that be an option here?

[udit7395]

@laukik-hase we could like earlier we will take this up in second round. For now just make sure that the code works so that it becomes easy for omkar to test

[udit7395]

@VedantParanjape Since we know the solution change the tag, add appropriate assignee and close the issue accordingly

[VedantParanjape]

@VedantParanjape Since we know the solution change the tag, add appropriate assignee and close the issue accordingly

Can we keep it open, it's possible that we forgot to do this change if it is closed.

I'll assign this to someone. Changed tag to enhancement

[VedantParanjape]

Fixed in b2a671bfe01d3819d31623fe68059c05729c6165