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

Update uart_ll.h - Fixing division by zero (IDFGH-10990) #12179

Closed
wants to merge 2 commits into from

Conversation

educampos28
Copy link
Contributor

I have faced the division by zero when the baud rate is higher than 1Mbits/s. It happens because the integer division returns zero for sclk_div while slightly below 1.0.

I also added 0.5 to improve the roundup process of converting float to integer.

With that fix, I can reach 3.125Mbits/s.

I have faced the division by zero when the baud rate is higher than 1Mbits/s.    It happens because the integer division returns zero for sclk_div while slightly below 1.0.

I also added 0.5 to improve the roundup process of converting float to integer.

With that fix, I can reach 3.125Mbits/s.
@CLAassistant
Copy link

CLAassistant commented Sep 1, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Sep 1, 2023
@github-actions github-actions bot changed the title Update uart_ll.h - Fixing division by zero Update uart_ll.h - Fixing division by zero (IDFGH-10990) Sep 1, 2023
@songruo
Copy link
Collaborator

songruo commented Sep 6, 2023

Hi @educampos28,

DIV_UP(a, b) should already guarantee that the result is equal or larger than 1, as long as a >= 1 (it is always true for sclk_freq).

Also I have tried on ESP32S3 with a baud rate of 3.125Mbits/s, the code runs fine without crash.

Can you provide more details, such as compile option, sdkconfig, UART source clock selection, baudrate configurations, that would lead to the "division by zero" error?

@educampos28
Copy link
Contributor Author

educampos28 commented Sep 6, 2023

I removed my changes and ran the code again. I think that image will help a lot.

image
image

I did a careful assessment and I figure out the reason is another issue.
Look at the numerator a+b-1. The value of (80000000+(1041666*4095)-1) doesn't fit to 32 bits registers.

@songruo
Copy link
Collaborator

songruo commented Sep 6, 2023

@educampos28 Thanks for the debug! Do you want to fix your PR?

I guess casting max_div * baud to uint64_t would fix the problem.

And would you also apply the fix to all other targets?

Then we can go through the PR process!

@educampos28
Copy link
Contributor Author

it will fix. I prefer to use float in this case to guarantee a better result while rounding the number. Also, that function is called once when the user start their system, which the float math won't be a firmware overload for the firmware.

Sorry, I don't have time to push this fix to other targets.

Use uint64_t instead of float to calculate the clock division for the baudrate
@songruo songruo added the PR-Sync-Merge Pull request sync as merge commit label Sep 11, 2023
@songruo
Copy link
Collaborator

songruo commented Sep 11, 2023

sha=3f40c80c06b2d9057fd651db98f6c5b18f64a8b6

@suda-morris
Copy link
Collaborator

merged in b0bde58

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new labels Sep 20, 2023
espressif-bot pushed a commit that referenced this pull request Sep 21, 2023
CommanderRedYT pushed a commit to CommanderRedYT/esp-idf that referenced this pull request Sep 24, 2023
espressif-bot pushed a commit that referenced this pull request Sep 30, 2023
espressif-bot pushed a commit that referenced this pull request Oct 23, 2023
movsb pushed a commit to movsb/esp-idf that referenced this pull request Dec 1, 2023
hathach pushed a commit to adafruit/esp-idf that referenced this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants