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

ledc.c: Fix frequency calculation. (IDFGH-10569) #11810

Closed

Conversation

IhorNehrutsa
Copy link
Contributor

@IhorNehrutsa IhorNehrutsa commented Jul 5, 2023

Round int instead of truncate.
ledc_get_freq() returns value 2Hz when the actual 3Hz was setted (Tested with oscilloscope).

Add ledc_calc_duty_resolution() -
Helper function to calculate the maximum possible LEDC duty resolution in bits for ledc_timer_config()

ledc.c: Fix frequency calculation.
Round int instead of truncate.
@CLAassistant
Copy link

CLAassistant commented Jul 5, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jul 5, 2023
@github-actions github-actions bot changed the title ledc.c: Fix frequency calculation. ledc.c: Fix frequency calculation. (IDFGH-10569) Jul 5, 2023
@IhorNehrutsa
Copy link
Contributor Author

The same rounding technique can see in

* NOTE: We are also going to round up the value when necessary, thanks to:
* (freq_hz * precision) / 2
*/
return ( ( (uint64_t) src_clk_freq << LEDC_LL_FRACTIONAL_BITS ) + ((freq_hz * precision) / 2 ) )
/ (freq_hz * precision);

@suda-morris suda-morris added the PR-Sync-Merge Pull request sync as merge commit label Jul 6, 2023
@IhorNehrutsa
Copy link
Contributor Author

sha=9caa55a368a38199f4ab188a5a490ac90a31b4ab

What does it mean? I can't find this sha.

@IhorNehrutsa
Copy link
Contributor Author

It look like we need to merge into v5.0-beta1

image

@suda-morris
Copy link
Collaborator

sha=9caa55a368a38199f4ab188a5a490ac90a31b4ab

What does it mean? I can't find this sha.

By this, we're asking the bot to sync the PR int our internal Gitlab server. We will merge this PR to the master first and later will backport to 5.1 and 5.0

Helper function to calculate the maximum possible LEDC duty resolution in bits for ledc_timer_config()
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels Jul 11, 2023
@IhorNehrutsa
Copy link
Contributor Author

@suda-morris
Is any restrictions?
May you guide me on the time of approving?
Thanks

@songruo
Copy link
Collaborator

songruo commented Jul 11, 2023

@IhorNehrutsa The fix has been merged into master (it might take a while to sync to Github). Backports to release/v5.1 and release/v5.0 will be done soon.

espressif-bot pushed a commit that referenced this pull request Jul 14, 2023
ledc.c: Fix frequency calculation.
Round int instead of truncate.

Merges #11810
espressif-bot pushed a commit that referenced this pull request Jul 22, 2023
ledc.c: Fix frequency calculation.
Round int instead of truncate.

Merges #11810
@IhorNehrutsa
Copy link
Contributor Author

@suda-morris, @songruo
Will the second commit of this PR be merged?
Or one PR = one commit?
Thanks.

espressif-bot pushed a commit that referenced this pull request Jul 27, 2023
ledc.c: Fix frequency calculation.
Round int instead of truncate.

Merges #11810
@suda-morris suda-morris added PR-Sync-Rebase Pull request sync as rebase commit and removed Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable PR-Sync-Merge Pull request sync as merge commit PR-Sync-Rebase Pull request sync as rebase commit labels Aug 4, 2023
@IhorNehrutsa
Copy link
Contributor Author

IhorNehrutsa commented Sep 5, 2023

Hi, @songruo, @suda-morris.
Is this PR ready? I can't understand its status.
Thanks.

espressif-bot pushed a commit that referenced this pull request Oct 25, 2023
Helper function to find the maximum possible duty resolution in bits
for ledc_timer_config()

Merges #11810
@suda-morris
Copy link
Collaborator

hi @IhorNehrutsa Sorry for the late response. It's merged in 9ced546

@IhorNehrutsa IhorNehrutsa deleted the release/v5.0 branch November 2, 2023 07:14
@IhorNehrutsa
Copy link
Contributor Author

By this, we're asking the bot to sync the PR int our internal Gitlab server. We will merge this PR to the master first and later will backport to 5.1 and 5.0

The fix has been merged into master (it might take a while to sync to Github). Backports to release/v5.1 and release/v5.0 will be done soon.

Still absent in the release/v5.1 and release/v5.0
We can not use the release/v5.2 and master branches because they don't have stable tags.

image

movsb pushed a commit to movsb/esp-idf that referenced this pull request Dec 1, 2023
Helper function to find the maximum possible duty resolution in bits
for ledc_timer_config()

Merges espressif#11810
@IhorNehrutsa
Copy link
Contributor Author

Are there any chances of getting this PR in release/v5.1 and release/v5.0 branches?

@songruo
Copy link
Collaborator

songruo commented Jan 9, 2024

@IhorNehrutsa release/v5.2 will come out very soon! I would expect it to be there in one month (here is the beta branch). Then would you be able to use release/v5.2 branch?

@IhorNehrutsa
Copy link
Contributor Author

IhorNehrutsa commented Jan 9, 2024

@songruo Thanks for the fast answer.
release/v5.2 looks like still unstable. MicroPython maintainers prefer to use stable versions.
MicroPython port to the ESP32
esp32/PWM: Reduce inconsistencies between ports. #10854 is waiting for this PR one year. :(

Please apply it to release/v5.1 and make a new tag v5.1.3
Thank you.

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.

5 participants