search

microsoft / pxt-microbit

A Blocks / JavaScript code editor for the micro:bit built on Microsoft MakeCode
https://makecode.microbit.org
Other
710 stars 586 forks source link

Document `input.runningTimeMicros()` overflows after 18 minutes #5372

Open microbit-carlos opened 10 months ago

microbit-carlos commented 10 months ago

This caught me by surprised and it's a MakeCode specific limitation, as CODAL returns a 32 bit unsigned int and MakeCode drops the 2 most significant bits: https://github.com/microsoft/pxt-microbit/blob/975c890f2b4b1cf6534ee893b91c8e740739824f/libs/core/control.cpp#L239-L245 I guess the signed bit makes sense, but don't know why the second bit is dropped as well.

It was done in commit https://github.com/microsoft/pxt-microbit/commit/15b4b47e6dbeb40026e6211a3f8e5706c456791e as part of PR https://github.com/microsoft/pxt-microbit/pull/2572 (512b4dd2af68171f8843d4c602c061d333b62971).

This should probably be documented, as 18 minutes is a relatively short time that can be easily reached.

Docs: https://github.com/microsoft/pxt-microbit/blob/975c890f2b4b1cf6534ee893b91c8e740739824f/docs/reference/input/running-time-micros.md#L1-L16

microbit-carlos commented 10 months ago

@pelikhan out of curiosity, do you know why the 2 most significant bits from CODAL/DAL system_timer_current_time_us are masked out in MakeCode? (it was done in https://github.com/microsoft/pxt-microbit/commit/512b4dd2af68171f8843d4c602c061d333b62971) https://github.com/microsoft/pxt-microbit/blob/975c890f2b4b1cf6534ee893b91c8e740739824f/libs/core/control.cpp#L239-L245

The signed bit makes sense, but not sure why bit 30 is removed as well, was there a technical reason?

pelikhan commented 10 months ago

@mmoskal

mmoskal commented 10 months ago

Integers up to 0x3fff_ffff are unboxed. Anything larger is boxed as a double. We could change this function to return a double (I think the codal call returns uint64_t).

microbit-carlos commented 10 months ago

Ah, that's interesting to know, thanks!

And yes, both DAL and CODAL return uint64_t (codal-microbit-v2 set CODAL_TIMESTAMP in target.json).

Not sure if changing the size range of input.runningTimeMicros() would be considered a breaking change (probably no, as it's still a number in JS/TS), so up to you, although if it's left as it is, it's worth noting the overflow in the docs.

abchatra commented 1 month ago

We need to document this. Not planning to fix.