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

One single full period after mcpwm_set_duty 0% (IDFGH-8438) #9904

Closed
3 tasks done
usbalbin opened this issue Oct 3, 2022 · 16 comments
Closed
3 tasks done

One single full period after mcpwm_set_duty 0% (IDFGH-8438) #9904

usbalbin opened this issue Oct 3, 2022 · 16 comments
Assignees
Labels
Awaiting Response awaiting a response from the author Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@usbalbin
Copy link

usbalbin commented Oct 3, 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.

General issue report

Hi!

I am using MCPWM from IDF v4.4. However I have a problem. When changing the duty from > 0% down to 0.0% i get a full period with 100% duty before the expected duty of 0%. I am generating a sine wave using PWM(forming the sine shape by continuously changing the duty) and this messes up the curve form.

Is this a bug in IDF/the hardware? Is there any way to fix/work around this? I would, if possible, prefer as few special cases in my program as possible since this needs to run quite fast :)

@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 3, 2022
@github-actions github-actions bot changed the title One single full period after mcpwm_set_duty 0% One single full period after mcpwm_set_duty 0% (IDFGH-8438) Oct 3, 2022
@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Oct 9, 2022
@suda-morris
Copy link
Collaborator

suda-morris commented Oct 10, 2022

@usbalbin
master branch

test code:

TEST_CASE("MCPWM zero duty", "[mcpwm]")
{
    mcpwm_gpio_init(MCPWM_UNIT_0, MCPWM0A, 0);
    mcpwm_gpio_init(MCPWM_UNIT_0, MCPWM0B, 2);

    // Set PWM freq and duty, start immediately
    mcpwm_config_t pwm_config = {
        .frequency = 1000,
        .cmpr_a = 10,
        .cmpr_b = 10,
        .counter_mode = MCPWM_UP_COUNTER,
        .duty_mode = MCPWM_DUTY_MODE_0,
    };
    mcpwm_init(MCPWM_UNIT_0, MCPWM_TIMER_0, &pwm_config);
    vTaskDelay(pdMS_TO_TICKS(100));
    mcpwm_set_duty(MCPWM_UNIT_0, MCPWM_TIMER_0, MCPWM_GEN_A, 0.0);
    vTaskDelay(pdMS_TO_TICKS(100));
    mcpwm_stop(MCPWM_UNIT_0, MCPWM_TIMER_0);
}

Generated wave:

image

I can see the duty cycle is changed to zero. Also didn't see a full period with 100% duty

@suda-morris suda-morris added the Awaiting Response awaiting a response from the author label Oct 10, 2022
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Selected for Development Issue is selected for development labels Oct 10, 2022
@usbalbin
Copy link
Author

usbalbin commented Oct 11, 2022

I do not have my code infront of me right now. However I believe I was running at 25kHz, with a group resolution of 160MHz and a timer resolution of 160MHz.

I'll try to do some more tests...

@usbalbin
Copy link
Author

usbalbin commented Oct 11, 2022

Only adding

mcpwm_group_set_resolution(MCPWM_UNIT_0, 160000000);
mcpwm_timer_set_resolution(MCPWM_UNIT_0, MCPWM_TIMER_0, 160000000);

right after mcpwm_gpio_init:
image

TEST_CASE("MCPWM zero duty", "[mcpwm]")
{
    mcpwm_gpio_init(MCPWM_UNIT_0, MCPWM0A, 0);
    mcpwm_gpio_init(MCPWM_UNIT_0, MCPWM0B, 2);

    mcpwm_group_set_resolution(MCPWM_UNIT_0, 160000000); // <---- Added
    mcpwm_timer_set_resolution(MCPWM_UNIT_0, MCPWM_TIMER_0, 160000000);// <---- Added

    // Set PWM freq and duty, start immediately
    mcpwm_config_t pwm_config = {
        .frequency = 1000,
        .cmpr_a = 10,
        .cmpr_b = 10,
        .counter_mode = MCPWM_UP_COUNTER,
        .duty_mode = MCPWM_DUTY_MODE_0,
    };
    mcpwm_init(MCPWM_UNIT_0, MCPWM_TIMER_0, &pwm_config);
    vTaskDelay(pdMS_TO_TICKS(100));
    mcpwm_set_duty(MCPWM_UNIT_0, MCPWM_TIMER_0, MCPWM_GEN_A, 0.0);
    vTaskDelay(pdMS_TO_TICKS(100));
    mcpwm_stop(MCPWM_UNIT_0, MCPWM_TIMER_0);
}

@usbalbin
Copy link
Author

usbalbin commented Oct 11, 2022

BTW screenshot is with pwm_config.frequency = 25000, however exact same thing happens with 1kHz. So to me, this seems to be something with group/timer prescaler?

@usbalbin
Copy link
Author

In case it is relevant, this is running on a ESP32-WROOM-32D

@suda-morris
Copy link
Collaborator

But if you set the MCPWM timer resolution to 80MHz, then you can see the 25KHz PWM.

@usbalbin
Copy link
Author

For me it is the same problem when using 80MHz for group and 80MHz for timer

@usbalbin
Copy link
Author

Some tests

group timer pwm problem
160 MHz 160 MHz 1kHz y
160 MHz 160 MHz 25kHz y
80 MHz 80 MHz 25kHz y
160 MHz 1 MHz 1kHz n
160 MHz 16 MHz 1kHz n
160 MHz 40 MHz 1kHz n
160 MHz 60 MHz 1kHz note*
160 MHz 70 MHz 1kHz n
160 MHz 80 MHz 1kHz note**
160 MHz 90 MHz 1kHz y
160 MHz 100 MHz 1kHz y
160 MHz 120 MHz 1kHz y

* the last pulse is only 125ns, which is a bit strange
** Got the 125ns once, but not when trying the same thing again, then everything worked as expected

@suda-morris
Copy link
Collaborator

@usbalbin Thanks for your testing! Looks like the timer resolution must be an integral multiple of the timer's resolution and they two can't be same.

@suda-morris
Copy link
Collaborator

what's more, when calling mcpwm_set_duty, the compare value (duty cycle) won't be updated until the next TEZ (Timer Equal Zero) event, I think this is the reason you see the one more full period pulse.

@suda-morris
Copy link
Collaborator

Could you please test with the following patch on the master branch, hope it can resolve the issue.

➤ git diff ../../deprecated/mcpwm_legacy.c                                                                                                              15:02:46
diff --git a/components/driver/deprecated/mcpwm_legacy.c b/components/driver/deprecated/mcpwm_legacy.c
index 1d9577b8c0..2e2bb402a9 100644
--- a/components/driver/deprecated/mcpwm_legacy.c
+++ b/components/driver/deprecated/mcpwm_legacy.c
@@ -284,6 +284,7 @@ esp_err_t mcpwm_set_duty(mcpwm_unit_t mcpwm_num, mcpwm_timer_t timer_num, mcpwm_
     uint32_t set_duty = mcpwm_ll_timer_get_peak(hal->dev, timer_num, false) * duty / 100;
     mcpwm_ll_operator_set_compare_value(hal->dev, op, cmp, set_duty);
     mcpwm_ll_operator_enable_update_compare_on_tez(hal->dev, op, cmp, true);
+    mcpwm_ll_operator_enable_update_compare_on_tep(hal->dev, op, cmp, true);
     mcpwm_critical_exit(mcpwm_num);
     return ESP_OK;
 }
@@ -305,6 +306,7 @@ esp_err_t mcpwm_set_duty_in_us(mcpwm_unit_t mcpwm_num, mcpwm_timer_t timer_num,
     uint64_t compare_val = real_timer_clk_hz * duty_in_us / 1000000;
     mcpwm_ll_operator_set_compare_value(hal->dev, op, cmp, (uint32_t)compare_val);
     mcpwm_ll_operator_enable_update_compare_on_tez(hal->dev, op, cmp, true);
+    mcpwm_ll_operator_enable_update_compare_on_tep(hal->dev, op, cmp, true);
     mcpwm_critical_exit(mcpwm_num);
     return ESP_OK;
 }

@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed and removed Status: In Progress Work is in progress labels Oct 17, 2022
@usbalbin
Copy link
Author

That does indeed seem to solve the problem!

image

@usbalbin
Copy link
Author

usbalbin commented Oct 17, 2022

However note that 1kHz is not reachable at 160MHz group and timer resolution(would lead to a peak value > 2^16). However a output frequency of 25kHz works fine

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Oct 19, 2022
@usbalbin
Copy link
Author

Thanks!

Will this fix be backported to IDF 4.4?

@suda-morris
Copy link
Collaborator

Thanks!

Will this fix be backported to IDF 4.4?

Yes, we will!

@usbalbin
Copy link
Author

Awesome, Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Response awaiting a response from the author Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

3 participants