uasyncio tests can fail on windows ports #8301
Comments
I noticed this recently too (looking at the unix port). This seems like a long time to block. It seems like the event poll hook should actually Since Linux/Mac/Windows aren't realtime systems, I don't think it is reasonable to expect accurate timers below 100ms on these system. And even above that point, there can be hiccups where a thread might not be resumed for longer than that. |
|
I tested some things and the Appveyor test failures are remedied by increasing the sleep periods (mostly using >100mSec instead of the typical 20 to 50mSec used now, to account for the default 15.6mSec resolution windows uses) in the tests I've seen failing. This does mean tests run longer but that's not really an issue I think. The other option (bascially increasing the kernel timer interrupt rate, which affects the whole system) also works and makes the tests pass. It's convenient in the sense that it makes the windows port suitable for testing asyncio with small wait periods. I'm inclined to implement the first option, and add a section in the readme which shows how to create a variant which implements the second. @dpgeorge what do you think? |
I agree. Being power efficient is more important.
I agree. If these systems are intended to be semi-realtime then that's up to the Linux (or other OS) implementation to be tuned so that it does have better timing accuracy (either tuned by the distribution, or tuned by the user for their application). MicroPython should then do the best job it can within that system (and in particular not change the kernel systick on Windows).
My aim writing those tests was to have short delays so the tests didn't take too long. Having lots of tests and running them over and over again (when developing) does eat up time! Changing to ~100ms for delays in those tests would probably be OK though. And it might even get these timing tests to pass on mac (which has similar issues with time resolution). OTOH, I also think it's fair enough to increase the kernel systick on Windows when running under AppVeyor (eg with a special flag passed to MicroPython when running). Just consider it as creating a special Windows test environment to run the test suite. Anyway, I'm happy to increase the timing of the relevant tests to ~100ms. Hopefully that's enough. |
|
It would be more work are require a separate build, but a more deterministic way to run tests that depend on the passage of time is to create a "fake" clock that doesn't use the system clock at all. Instead, the clock state is driven programatically. This means tests that use this would have to be modified to tell the clock to tick X times, then test a condition, tick again, test something else, etc... But it eliminates the intermittent failures that inevitably come with relying on a real clock. |
Another alternative is to have a build (or command line option) that runs the clock at a rate * R but still report "normal" ticks to the program. So if you ask for R=10 then the clock runs 10 times slower but tick values are /10: t0 = time.ticks_ms()
time.sleep_ms(0.01) # actually sleeps for 100ms
t1 = time.ticks_ms()
dt = time.ticks_diff(t1, t0) # returns 0.01 |
Wasn't aware of this, but makes sense.
This is essentially the same as slowing down the tests, which, I found out today, still isn't sufficient to get them to always pass on windows: all my 100mSec sleeps now actually take 110mSec (which wasn't the case earlier). Combined with the overall test time increase, I'm thinking it's time to give up on this idea.
That's what I usually do: have a single swappable time source from which everything else derives (this is great e.g. when the entire application runs on top of streaming data from an ADC and you use each sample as a tick, and then for the tests that just gets swapped for a fake timer to which the actual time gets set in the tests, very easy and versatile - as long as you don't need 'real clock' time somewhere).
Not necessarily, or at least: not explicitly. For the type of tests MicroPython has it should be suffcient to make all Another alternative: change all timing tests to detect if they run on the unix/windows port and if so change all equality comparisons to not be exact but in a range like +-10mSec of the expected value. There aren't that many tests so should be fine. |
I'm surprised this doesn't work. The tests already have Yet another option: change the tests so they don't test timing, just ordering of events. Then have a few special tests that do test timing, one set which tests rough timing (for windows/mac/etc) and another that tests accurate timing (for bare metal). |
That's actually +-5mSec but that's covered in #8617 already :) |
You are right! So changing it to |
Fix displayio when CIRCUITPY_REPL_LOGO is disabled
Since #8030 the dev variant yields 'random' test failures for uasyncio, probably depending on machine/version/...
For example on my machine the uasyncio_basic consistently prints
took 30 50 0as last line, instead of the expectedtook 20 40 0. Some builds of the mingw port also fail in Appveyor for me. The reason for this is mentioned in #7796 :The dev variant enables both uasyncio and the scheduler, and uasyncio calls
poll_poll_internaland that usesMICROPY_EVENT_POLL_HOOKwhich in turn callsmp_hal_delay_us(500). That last one is the culprit.Setting
MICROPY_ENABLE_SCHEDULERto 0 leavesMICROPY_EVENT_POLL_HOOKundefined, so doesn't compile because moduselect.c uses that unconditionally. Just adding#define MICROPY_EVENT_POLL_HOOKfixes that. It also makes the uasyncio tests pass. But it also turnspoll_poll_internalinto a busy loop. Likewise formp_hal_delay_msbecause it's conditional onifdef MICROPY_EVENT_POLL_HOOKwhich should argueably should be#if MICROPY_ENABLE_SCHEDULER.Things also grew a bit messy over time with there being multiple ways to sleep on Windows:
mp_hal_delay_mscallsSleepExbutmp_hal_delay_ususes an implementation which uses which uses a timer. Which I wrote, but I don't remember why: I don't think it's better/worse/very different from SleepEx. Could be that things used to be different back then. Anyway these 2 should be consolidated.Then the question remains how to fix this
The text was updated successfully, but these errors were encountered: