lancaster-university / microbit-dal

http://lancaster-university.github.io/microbit-docs
Other
256 stars 130 forks source link

Light sensor should notify caller when the first set of data has been collected #426

Open pelikhan opened 5 years ago

pelikhan commented 5 years ago

On first read, the light sensor already returns 255. https://github.com/Microsoft/pxt-microbit/issues/1786

microbit-mark commented 5 years ago

@jaustin has a DAL reproducer here https://gist.github.com/jaustin/b670a3cd5bd2d159301a9552020307e9 this outputs an initial value of 0255 over serial.

The docs state "The ADC takes around 4ms to settle and give accurate values with minimal current. This places restrictions on our sensing window." is this part of the issue and do we need to allow time to settle?

martinwork commented 5 years ago

The 4ms settle time is after the light sensor class switches an leds column channel to input, so it needs a 5ms window to read each of three channels, which are averaged to calculate the reading. The display class reduces the system tick to 5ms and passes every fourth tick to the light sensor to read one channel. So the light sensor gets to read one channel every 20ms and is fully updated every 60ms. For the very first read, there's a wait of about 20ms before the display passes the first read message. So the total settle time after the first read is about 80ms and a sudden change in light level will be seen to occur in 3 stages over 60ms.

gbentley commented 4 years ago

Just a laymans' comment to say that it's counter-intuitive for students that the initial state of the light sensor is 255. My students needed to grab the initial light level, and I had to dig a bit deeper to find why it wasn't working as expected. Calling the light sensor, adding a time delay, then calling it again and getting the initial reading based on the 2nd call worked.

microbit-mark commented 3 years ago

The initial value in CODAL for lightLevel read is 0, we should maintain consistency across micro:bit versions. Can we do the same here @finneyj?

whaleygeek commented 3 years ago

@microbit-mark Just a note, based on my experience of building lots of projects - any initial value that doesn't match the real reading, will always mess up any usage in the micro:bit. i.e. any scheme like this that you introduce, will always require a 'take a dummy reading and delay for a bit' to ensure any thresholding inside the user app is not affected. This is non intuitive for the user. Whether this is 0 or 255 or something else, it will always be wrong for the app user, if it doesn't match the actual light reading.

As a related question - do any other sensors suffer this 'first reading duff' issue (e.g. microphone)? Especially if the DAL has a time-based filter on the data that requires a few cycles to fill up it's pipeline?

Usually the 'microbittish' way to do these things, is the least amount of surprise to the user, and to do all the heavy lifting in the DAL so that something that is always useable is presented to the user app (so that the user app doesn't have to do any magic trickery to make it work properly).

I wonder if a better way would be to suspend the fiber that is requesting the reading until a valid reading is available? Given that is of the order of 60ms, it's not a million miles away from the magnitude of fiber delay that you get at the end of a forever() block anyway. Comments @finneyj ??

If this is done in the on-start block, this naturally means you would block the fiber for say 60ms until you get a valid reading (we don't want other fibers starting early until the onstart has finished, it causes terrible problems).

whaleygeek commented 3 years ago

FYI @microbit-mark there is a MakeCode reproducer in this ticket, where I originally logged this issue: https://github.com/microsoft/pxt-microbit/issues/3451 - I note this DAL issue has been open for a lot longer, but I think my ticket puts the issue in a context that shows why this is a problem for the average user.

pelikhan commented 3 years ago

The current behavior should be treated as a bug really. We should wait for data on the first read.

pelikhan commented 3 years ago

Note that MakeCode implementation builds down to

        return uBit.display.readLightLevel();
finneyj commented 3 years ago

Makes sense to delay the fiber reading a little while - that's precisely the behaviour for v2.

I remember discussing this on a call a long time ago, and am certain there was a reason why we didn't do this at the time. I honesty can't think what it would be though...

pelikhan commented 3 years ago

Probably fell off the plate or we ran out of time.