Gpiozero: Negative `value` from LightSensor

[e28eta]

Operating system: Raspbian Buster
Python version: 3.7.3
Pi model: Pi 4 Model B Rev 1.2
GPIO Zero version: 1.5.1
Pin factory used: RPiGPIOFactory

I was testing out a LightSensor in the python repl - adjusting the charge_time_limit and printing the value, and was surprised to see negative values (because the value documentation said it’d return [0, 1]).

>>> sensor = LightSensor(17, charge_time_limit=0.001)
>>> while True:
...   v = sensor.value
...   print(v)
...   if v < 0:
...     print(sensor._queue.queue)
...   sleep(1)

The output, as I slowly moved my hand closer it reached a point where the value went negative:

0.5771700005207094
[...] # I slowly moved my hand closer & closer to the photoresistor:
0.11900900042383
0.04637999952683458
-0.04319300044153351
deque([-0.028656000016781036, -0.04319300044153351, -0.03911899986997014, -0.07424900033947779, -0.09972999962337781], maxlen=5)
-0.20521100052428665
deque([-0.20893299936142284, -0.20400699961464852, -0.20521100052428665, -0.21548799961601617, 0.11950800014892593], maxlen=5)

I was reliably able to get negative readings :point_up:

It does read 0.0, especially if I fully cover the sensor quickly.

0.5779669998228201
0.5908740002050763
0.0
0.0
0.0
0.0
0.6688179999182466
0.6710590003203833

I was poking around trying to diagnose, and I think this is the relevant code?

https://github.com/gpiozero/gpiozero/blob/497b7169f63b14581590a00344a8a7b80da477f6/gpiozero/input_devices.py#L726-L737

I’m not sure how the deque is getting negative values, unless wait sometimes runs longer than requested? Or maybe one or more ticks happens between assigningstart and the call to wait?

I’d believe my photoresistor/capacitor pair are poorly matched, and charge_time_limit=0.001 is too close to the tick resolution, but maybe it’s worth adding some guard rails to keep the return value within the documented range?

Or maybe add documentation on minimum value for charge_time_limit?

Thanks!

[lurch]

If you're getting a value of -0.2, then I guess that means that 1.0 - (self.pin_factory.ticks_diff(self._charge_time, start) / self.charge_time_limit) == -0.2, and you said you have charge_time_limit=0.001, so:
1.0 - (self.pin_factory.ticks_diff(self._charge_time, start) / 0.001) == -0.2
-1.0 + (self.pin_factory.ticks_diff(self._charge_time, start) / 0.001) == 0.2
self.pin_factory.ticks_diff(self._charge_time, start) / 0.001 == 1.2
self.pin_factory.ticks_diff(self._charge_time, start) == 0.0012
=> self._charge_time == start + 0.0012 i.e. the measured charge_time is actually bigger than charge_time_limit.

So yeah, this is probably a side-effect of charge_time_limit having such a small value (and Python not being well-suited to measuring such small time differences), but it probably also makes sense for the code to be changed to

 if self._charge_time is None: 
     return 0.0 
 else: 
     return 1.0 - min(1.0,
         (self.pin_factory.ticks_diff(self._charge_time, start) / 
         self.charge_time_limit))

ping @waveform80

[e28eta]

Thanks!

Also, I wanted to compliment y’all on a beautiful code base. It was a pleasure to read through.

[waveform80]

If you're getting a value of -0.2, then I guess that means that 1.0 - (self.pin_factory.ticks_diff(self._charge_time, start) / self.charge_time_limit) == -0.2, and you said you have charge_time_limit=0.001, so:
1.0 - (self.pin_factory.ticks_diff(self._charge_time, start) / 0.001) == -0.2
-1.0 + (self.pin_factory.ticks_diff(self._charge_time, start) / 0.001) == 0.2
self.pin_factory.ticks_diff(self._charge_time, start) / 0.001 == 1.2
self.pin_factory.ticks_diff(self._charge_time, start) == 0.0012
=> self._charge_time == start + 0.0012 i.e. the measured charge_time is actually bigger than charge_time_limit.

So yeah, this is probably a side-effect of charge_time_limit having such a small value (and Python not being well-suited to measuring such small time differences), but it probably also makes sense for the code to be changed to

 if self._charge_time is None: 
     return 0.0 
 else: 
     return 1.0 - min(1.0,
         (self.pin_factory.ticks_diff(self._charge_time, start) / 
         self.charge_time_limit))

ping @waveform80

Looks bang on the money to me; one other thing I ought to do is shift the query of factory.ticks() to immediately before the line which flips the pin to input just to reduce the chances of this, but yes the min() limit is also needed.

Also, I wanted to compliment y’all on a beautiful code base. It was a pleasure to read through.

Except for devices.py ;)

[lurch]

...perhaps negative light values imply a black hole? :rofl: "darker than dark"

PR just submitted.