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

unsigned long millis() timer does not wrap / ignores top 10 bits / causes regression #2455

Closed
dirkx opened this issue Feb 13, 2019 · 10 comments
Labels
Status: Stale Issue is stale stage (outdated/stuck)

Comments

@dirkx
Copy link
Contributor

dirkx commented Feb 13, 2019

The fix for 2250, commit daea578, changed the unsigned long millis() into returning (unsigned long) micros()/1000.

This potentially seems to have two side effects; firstly it means the top 10 bits of this unsigned long are no longer used (micros() is also an unsigned long; wrapping every 1 hour and 12 minutes - so millis() follows suit in the bottom 21 bits).

And secondly it breaks the time honoured (and very common code in Arduino libraries) of the type:

 unsigned long last = 0;
 
 void loop() {
     // Use a substract to make sure that this also works when millis() wraps
    // around. See https://playground.arduino.cc/Code/TimingRollover
     if (millis() - last > 5*1000UL) {
           Serial.println("5 second tock");
           last = millis();
    };   

Such as used in all sort of TCP clients, SSL session key renewals, MQTT ping/acks, websocket callbacks and so on.

Board: All ESP
Core Installation version: master from git as of today.
IDE name: Arduino IDE, Platformio
Flash Frequency: All
PSRAM enabled: either
Upload Speed: 115200
Computer OS: OSX

The ideal solution would perhaps be to use a slower timer; or to derive the millis() form micros - but let it keep its own top 10 bit accounting.

@dirkx
Copy link
Contributor Author

dirkx commented Feb 13, 2019

Pull that introduced this: #2250

@stickbreaker
Copy link
Contributor

See PR #2438

@dirkx
Copy link
Contributor Author

dirkx commented Feb 13, 2019

Changing it to
return (unsigned long) (micros64() / 1000);
ought to work if that is a decent 64 bit timer.

@stickbreaker
Copy link
Contributor

stickbreaker commented Feb 13, 2019

@dirkx the problem is that the compiler takes short cuts(optimization). The result is a 32bit value, one param is a 32 value, so it down converts the other param (64bit value) to a 32bit value(loosing sigs) then does the division. That pr's (#2438) only change is to force the compiler to do 64bit math before the down conversion to 32bits.

Try that pr's solution, just change the divisor to 1000ULL. Problem's gone.

Chuck.

@dirkx
Copy link
Contributor Author

dirkx commented Feb 13, 2019 via email

@dirkx
Copy link
Contributor Author

dirkx commented Feb 23, 2019

#2438 solves the issue - and also fixes the MQTT and DHCP hangs.

@abqmichael
Copy link

Has this fix been released? I am still encountering the issue of the value of millis() wrapping around at 2^32/1000 on esp32 version 1.0.2. The bug was quite confounding for me to uncover and I'm sure anybody using any form of event-based programming will run into it quickly.

Esp32 version 1.0.2 is the most updated version that is available on Arduino.

@abqmichael
Copy link

Oops. I stand corrected. I did a fresh install of the ESP32 board library and it looks like the fix is in version 1.02.

My problem, apparently, was that though Arduino thought it had the newest library, my install was pointing to an old one. So I uninstalled the ESP32 board library and deleted a subdirectory with the driver code from my Arduino directory. Then did a fresh install, re-ran my tests and have broken through the prior barrier.

@stale
Copy link

stale bot commented Sep 1, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Stale Issue is stale stage (outdated/stuck) label Sep 1, 2019
@stale
Copy link

stale bot commented Sep 15, 2019

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Issue is stale stage (outdated/stuck)
Projects
None yet
Development

No branches or pull requests

3 participants