MicroPython
Potential issue with TICKS_DIFF macro #10723
-
|
While browsing the source code to see how the system timer of the RP2 is used, I stumbled by chance on the TICKS_DIFF macro of softtimer.c: I presume it is meant to calculate the relative difference between two values to determine when a timeout has occurred. Assuming that is true, it seemed more complicated than it should be, and something like this should work as well: The above uses properties of binary numbers and two's complement arithmetic. I adapted a test code I made in the past to compare the results of two different ways to calculate the same thing: The code above is compiled with gcc. It exercises some borderline conditions, such as values close to UINT32_MAX/2 when the signal bit of an int32_t changes and UINT32_MAX when the counter rolls over. In all these cases, both macros produce the same results. Here is the results I obtained: In all cases, the difference is close to UINT32_MAX/2 so the probability of them to occur is extremely low. I added the two lines below to the test vector which produced the following result: So with a difference of UINT32_MAX/4, which is still pretty big, things work as expected. One suggestion: use inline static instead of macros: |
Beta Was this translation helpful? Give feedback.
Replies: 2 comments 7 replies
-
|
The Macro you mention is only used in softtimer.c, with similar code being used for the ticks_diff() method of the time module, where the sign of the result matters. I do not understand why you suggest using a method which will produce wrong results under certain conditions, even if these seem rare. Sooner or later this will be an issue, latest after ~25 days, since it is used tick the ticks_ms counter. |
Beta Was this translation helpful? Give feedback.
-
I saw similar checks in a few other places after your mentioned similar math being used by ticks_diff(). For example, Unnecessary complicated comparison IMO (the compiler optimizer certainly reduces it). Anyway these tests prevent bugs from happening, so it is not a critical issue (it is not broken, do not fix it). FWIW, for anyone interested, there is an article on the principle behind the math of the simpler macro. It is in Portuguese (the title is a little pompous BTW), but Google makes a pretty decent translation; it is a pitty it messes up with the format. |
Beta Was this translation helpful? Give feedback.
-
@cve2022 Not sure exactly what you mean by this. Yes, we did verify what the compiler did when we wrote this, but not everyone compiles MicroPython with recent GCC (or GCC at all). There is a typo in the comment (missing minus sign). I think you're right about the softtimer macro though. The mistake here was that the diff calculation was copied from time_ticks_diff (where it is necessary because the period is not the full range of uint32_t, rather it's
I believe the proposed fix's result ( I spoke to Damien about this we will fix softtimer.c. Thanks for bringing it to our attention. |
Beta Was this translation helpful? Give feedback.
-
If you subtract
I am not a compiler expert, but I believe most compilers, even those not so recent, would reduce the expression above during the optimization phase and generate the code without that addition on the left size. I learned, the hard way (maaany years ago, with VHDL though), the compilers are smarter than us (or at least than me 😃).
I think the comment is still wrong, delta is not negative, inferring from
OK, that is new to me. I know Python handles integers differently (virtually unlimited int value) but I was not aware of the small int. Still learming... That could explain the results I obtained with time.ticks_diff(), though I would expect the abs values to be equal and odd, not even. However, the Pico returns different values for the same arguments. Is this correct?
I meant by "wrong" in the sense the expected result would be positive, but it becomes negative as an int_32, thus the maximum legal delay for it work as intended is 231-1. BTW, interesting seeing you using math notation [...) . 😃 |
Beta Was this translation helpful? Give feedback.
-
This is obviously true on the integers, but not when working mod(2**32).
delta is unsigned, but there is a subtle reliance on unsigned overflow happening on both sides of the equality going on here. You can see that for an input of -5 (i.e. a valid input) corresponding to
delta is a mp_uint_t that represents a negative number. It is "negative" in the sense that the user provided a negative number and the result must be a time "earlier" than the first argument. The important result is that the function behaves exactly as described in the comment in this function. For any delta that does not trip the overflow check, and for T a valid result from // - ticks_diff(ticks_add(T, delta), T) == delta
// - ticks_diff(T, ticks_add(T, delta)) == -deltaWe would definitely welcome a counterexample!
I don't know the specifics of the equivalent concept in other Python implementations but in MicroPython (depending on which object representation is chosen, see mpconfig.h) will store "small" integers inline in mp_obj_t. Typically there are is 31 or 30 bits available. This means that integer operations do not have to allocate on the heap, making them dramatically faster and avoiding other memory issues. In order to allow usage of ticks_ms, ticks_diff, etc without heap allocations, we confine ticks to always be small integers.
This is an invalid input to ticks_diff. It is only specified to work on values returned from the
The Pico and the ESP32 use different object representations, so their small int limit is different.
I'm still not sure if you're agreeing or disagreeing that this is the correct result?
I'm not sure how else to write it so that it's unambiguous! |
Beta Was this translation helpful? Give feedback.
-
I understand it now. I assumed delta would always be a positive number, which was the use case where I used the
Understood.
Maybe both. :) I realize now the use cases are different, that negative value would be correct in ticks_diff() use case while it was wrong in mine. So, none of us was wrong 😃 , we were just talking of different things with some commonalities.
I wonder if you teach maths or some related discipline. And I also wonder if many readers will understand the notation. The last time I saw it was during my graduation in physics, some thirty years ago. Most, me included, would simply use the numeric range, e.g., |
Beta Was this translation helpful? Give feedback.
-
|
This is to mark the thread as answered based on @jimmo 's first answer above. |
Beta Was this translation helpful? Give feedback.
This is to mark the thread as answered based on @jimmo 's first answer above.