fix(misc): Improve micros() int size #9803
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
👋 Hello leandromattioli, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 56 files 56 suites 4m 17s ⏱️ Results for commit 5ebb3a4. ♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Please rebase the PR so the CI can run properly |
leandromattioli
force-pushed
the
improve-micros-int-size
branch
from
fee85f6 to
1b15db2
Compare
|
I shall confess that I have mixed feelings about it, because it "breaks" Arduino compliance. But, as said, it shall cause no harm, given that a code like |
Allow delayMicroseconds() to use a 64 bits parameter. It has no bad effect if the parameter has less bits.
|
I think it can be a good PR. I added the possibility to execute |
Change from 32 to 64 bits the time parameter for delayMicroseconds() Follows proposed change within the PR.
|
Hi @leandromattioli ! Thank you for your proposal! After some discussion, we have decided to not accept your changes.
We hope you understand :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
The
micros()function on AVR-based Arduino boards is known to overflow after about 70 minutes, and that's because of the integer size of the return value. The ESP32 implementation, however, uses theesp_timer_get_time()internally, which currently returns an int64_t (why is it a signed integer?). Inspecting its implementation on the esp-idf repository, I think its type should be uint64_t, since it is just an alias foresp_timer_impl_get_time(), which in turn calls theticks_to_us. The latter seems to be an unsigned 64-bit wide integer:typedef uint64_t (*ticks_to_us_func_t)(uint64_t ticks);(from esp-idf components/hal/include/hal/systimer_hal.h)
So, I think there's really no need to stick with 32 bits (unsigned long is 32 bits in ESP32 architecture/compiler), since in the end of the chain we have the 64-bit
ticks_to_us()function. Apparently, we are just truncating the upper 32 bits with no good reason (being intentionally identical to AVR Arduinos in this case doesn't seem correct). I propose changing themicros()return type to unsigned long long, in this repository. I would create another pull request to changeint64_ttouint64_twhen applicable in the esp_timer_get_time() chain of aliases/function calls (so there would be 2 pull requests, one for this repository and other for the esp-idf repository).If someone is intentionally relying on the 70 minutes overflow, which seems a very strange use case in my opinion, the code would only be affected if the
autokeyword is used: if an unsigned long long value is used on an unsigned long variable, we would still have the 32 bits truncated.Future (and more ordinary) code, however, would be grateful not to have this 70 minutes limitation.
And if I understood it incorrectly, I am sorry to spend your time analyzing this pull-request.
Tests scenarios
I have tested the new micros() function on two types of ESP32 DevKit modules (ESPduino and ESP32 DevKit V1), althouh I haven't wait 70 minutes to confirm that micros() will no longer overflow.