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

MCPWM ESP32-S3 Function: mcpwm_set_duty_in_us (IDFGH-8151) #9648

Closed
3 tasks done
misiek1994 opened this issue Aug 26, 2022 · 1 comment
Closed
3 tasks done

MCPWM ESP32-S3 Function: mcpwm_set_duty_in_us (IDFGH-8151) #9648

misiek1994 opened this issue Aug 26, 2022 · 1 comment
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Bug bugs in IDF

Comments

@misiek1994
Copy link

misiek1994 commented Aug 26, 2022

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

4.4.2

Operating System used.

Linux

How did you build your project?

Eclipse IDE

If you are using Windows, please specify command line type.

No response

What is the expected behavior?

Function: mcpwm_set_duty_in_us should set pwm duty to specific value in us.

What is the actual behavior?

mcpwm_set_duty_in_us is not setting properly pwm.

Steps to reproduce.

mcpwm_gpio_init(MCPWM_UNIT_0, MCPWM0A, SERVO_PULSE_GPIO_1);
mcpwm_config_t pwm_config = {
    .frequency = 50, // frequency = 50Hz, i.e. for every servo motor time period should be 20ms
    .cmpr_a = 0,     // duty cycle of PWMxA = 0
    .counter_mode = MCPWM_UP_COUNTER,
    .duty_mode = MCPWM_DUTY_MODE_1,
};
mcpwm_init(MCPWM_UNIT_0, MCPWM_TIMER_0, &pwm_config);

//doesnt work
ESP_ERROR_CHECK(mcpwm_set_duty_in_us(MCPWM_UNIT_0, MCPWM_TIMER_0, MCPWM_OPR_A, 10000));
//works!
ESP_ERROR_CHECK(mcpwm_set_duty(MCPWM_UNIT_0, MCPWM_TIMER_0, MCPWM_OPR_A, 50.0));

Build or installation Logs.

No response

More Information.

In function:

esp_err_t mcpwm_set_duty_in_us(mcpwm_unit_t mcpwm_num, mcpwm_timer_t timer_num, mcpwm_generator_t gen,                 uint32_t duty_in_us)
{
    //the driver currently always use the timer x for operator x
    const int op = timer_num;
    //the driver currently always use the comparator A for PWMxA output, and comparator B for PWMxB output
    const int cmp = gen;
    MCPWM_GEN_CHECK(mcpwm_num, timer_num, gen);
    mcpwm_hal_context_t *hal = &context[mcpwm_num].hal;

    mcpwm_critical_enter(mcpwm_num);
    int real_group_prescale = mcpwm_ll_group_get_clock_prescale(hal->dev);
    unsigned long int real_timer_clk_hz =
            SOC_MCPWM_BASE_CLK_HZ / real_group_prescale / mcpwm_ll_timer_get_clock_prescale(hal->dev, timer_num);
    mcpwm_ll_operator_set_compare_value(hal->dev, op, cmp, duty_in_us * real_timer_clk_hz / 1000000);
    mcpwm_ll_operator_enable_update_compare_on_tez(hal->dev, op, cmp, true);
    mcpwm_critical_exit(mcpwm_num);
    return ESP_OK;
}

Probably overflow in line: mcpwm_ll_operator_set_compare_value(hal->dev, op, cmp, duty_in_us * real_timer_clk_hz / 1000000);

Changing to: mcpwm_ll_operator_set_compare_value(hal->dev, op, cmp, real_timer_clk_hz / 1000000 * duty_in_us); fixed the problem, but there might be another when real_timer_clk_hz is not multiplication of 1MHz.

@misiek1994 misiek1994 added the Type: Bug bugs in IDF label Aug 26, 2022
@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 26, 2022
@github-actions github-actions bot changed the title MCPWM ESP32-S3 Function: mcpwm_set_duty_in_us MCPWM ESP32-S3 Function: mcpwm_set_duty_in_us (IDFGH-8151) Aug 26, 2022
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Aug 29, 2022
@suda-morris
Copy link
Collaborator

@misiek1994 Nice catch, I can also confirm this is a multiplication overflow. We should use uint64_t when converting us to compare ticks.

@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Aug 30, 2022
Jason2866 added a commit to Jason2866/esp-idf that referenced this issue Sep 3, 2022
* timer: propagate isr register failure

Closes espressif#9651

* mcpwm: fix multiplication overflow in converting us to compare ticks

Closes espressif#9648

* heap: remove misleading info about malloc being equivalent to heap_caps_malloc(p, MALLOC_CAP_8BIT)

The actual memory allocated for malloc() depends on a lot of factors, see heap_caps_malloc_default()

Closes espressif#7659

* esp-rom: fixed error in miniz header documention for tdefl_init

Closes espressif#8435

* temperature_sensor: Fix issue that value is not accurate on ESP32-S3

* esp_https_ota: fix bug where `http_client_init_cb` is called after `esp_http_client_perform()` instead of before.

Closes espressif#9581

* Tasmota changes

* Fix linker error for C3

* Avoid bootloop if chip is unknown
   In case the PSIRAM chip is unknown, return an error and disable PSRAM
   instead of calling abort() and causing a bootloop
* Support for xiaomi single core ESP32
* Fix linker error for rom_temp_to_power
* fix linker error r_lld_ext_adv_dynamic_aux_pti_process

* Hide download percent when not interactive

* list(APPEND esptool_elf2image_args --dont-append-digest)

* Use native Apple ARM toolchains

* add package.json

* add submodules

* 8575d75

Co-authored-by: morris <[email protected]>
Co-authored-by: Marius Vikhammer <[email protected]>
Co-authored-by: Cao Sen Miao <[email protected]>
Co-authored-by: Harshit Malpani <[email protected]>
Co-authored-by: Mahavir Jain <[email protected]>
Jason2866 added a commit to Jason2866/esp-idf that referenced this issue Sep 3, 2022
* timer: propagate isr register failure

Closes espressif#9651

* mcpwm: fix multiplication overflow in converting us to compare ticks

Closes espressif#9648

* heap: remove misleading info about malloc being equivalent to heap_caps_malloc(p, MALLOC_CAP_8BIT)

The actual memory allocated for malloc() depends on a lot of factors, see heap_caps_malloc_default()

Closes espressif#7659

* esp-rom: fixed error in miniz header documention for tdefl_init

Closes espressif#8435

* temperature_sensor: Fix issue that value is not accurate on ESP32-S3

* esp_https_ota: fix bug where `http_client_init_cb` is called after `esp_http_client_perform()` instead of before.

Closes espressif#9581

* Tasmota changes

* Fix linker error for C3

* Avoid bootloop if chip is unknown
   In case the PSIRAM chip is unknown, return an error and disable PSRAM
   instead of calling abort() and causing a bootloop
* Support for xiaomi single core ESP32
* Fix linker error for rom_temp_to_power
* fix linker error r_lld_ext_adv_dynamic_aux_pti_process

* Hide download percent when not interactive

* list(APPEND esptool_elf2image_args --dont-append-digest)

* Use native Apple ARM toolchains

* add package.json

* add submodules

* 8575d75

* Arduino tinyusb v0.14.0 stripped

Co-authored-by: morris <[email protected]>
Co-authored-by: Marius Vikhammer <[email protected]>
Co-authored-by: Cao Sen Miao <[email protected]>
Co-authored-by: Harshit Malpani <[email protected]>
Co-authored-by: Mahavir Jain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Bug bugs in IDF
Projects
None yet
Development

No branches or pull requests

3 participants