Updated MCPWM Implementation for servo control to support ESP-IDF 5.0 (#90)#94
Updated MCPWM Implementation for servo control to support ESP-IDF 5.0 (#90)#94purviyeshi wants to merge 13 commits into
Conversation
| } | ||
|
|
||
| static esp_err_t set_angle_servo_helper(int servo_pin, int servo_max, int servo_min_pulsewidth, int servo_max_pulsewidth, unsigned int degree_of_rotation, mcpwm_unit_t mcpwm_num, mcpwm_timer_t timer_num, mcpwm_generator_t gen) | ||
| static esp_err_t set_angle_servo_helper(int servo_pin, int servo_max, int servo_min_pulsewidth, int servo_max_pulsewidth, int cmp_num, unsigned int degree_of_rotation) |
There was a problem hiding this comment.
cmp_num is used and just 0 or 1 is the value, then how will it identify which out of 4 motors are going to turn?
SuperChamp234
left a comment
SuperChamp234
left a comment
There was a problem hiding this comment.
The enable_servo() code can be refactored for creating multiple objects, for each servo respectively. Please look into this.
| int read_servo(servo_config *config); | ||
|
|
||
| #endif No newline at end of file | ||
| #endif |
There was a problem hiding this comment.
Please fix these types of newline diffs. They are unnecessary.
There was a problem hiding this comment.
There is a lot of repetitive logic in this code. Can’t we just create an object of the servo for three different GPIOs? Hardcoding this seems unnecessary.
aPR0T0
left a comment
aPR0T0
left a comment
There was a problem hiding this comment.
Can you please share testing results?
Refactor the structs in mario-repo
|
@SuperChamp234 I think this fork is ready to be merged, please share your reviews |
|
Same here @5iri |
|
this should be tested on current board to ensure this even works actually. will update this if we need this or not once we actually can test with the complete setup |
I think you should get this PR merged as current MCPWM code is deprecated. Check the build logs, it gives a warning. Not a high priority as Wall-E doesn't use PWM, but keep it in mind |
|
from what I heard from @Shankari02, this pr is not complete as this is not resolved for servos. I'll test this code and update the progress. |
5 participants
Added v5.1 ESP-IDF support for servo control