search

adafruit / Adafruit_CircuitPython_APDS9960

Adafruit Bundle driver for APSD9960 Gesture breakout board
MIT License
10 stars 17 forks source link

integration_time property uses raw sensor values and differs to C/Arduino library #17

Closed kevinjwalters closed 2 years ago

kevinjwalters commented 4 years ago

The C/Arduino library has a method called setADCIntegrationTime which takes a uint16_t iTimeMS and converts it into values that the APDS9660 uses, full code can be seen here https://github.com/adafruit/Adafruit_APDS9960/blob/master/Adafruit_APDS9960.cpp#L130-L149 (NB that code does not do a particularly explicit conversion from FP to (8 bit) integer).

The CircuitPython library currently just sends the raw value but the property is a bit misleading and called integration_time. The library defaults this to 0x01 which translates to (256 - 1) * 2.78ms = 708.9ms.

I'm not sure if this should be corrected to match the Arduino library with its more intuitive and user-friendly values? Changing this value now would disrupt existing users as there's no (clean!) way to code this with the existing property name. Is there a way to code this to happen in CP 5+ libraries only? Is that a good approach?

ladyada commented 4 years ago

ya could add integration_time_ms if you'd like :)

evaherrada commented 4 years ago

Is this a duplicate of #24?

kevinjwalters commented 4 years ago

Not sure, they might need retitling, this one could be for a integration_ms and #24 could be used to change the default value in the constructor?

jposada202020 commented 3 years ago

Taking a look at this I propose the following,

  1. Create a new class function to calculate the value according to formula in datasheet pag 20.
  2. As the class uses kwargs we could add the intergration_time_ms parameter and document this adding the correct default value
  3. For legacy we leave the value for integration time as 0x1 with the corresponding functions. Let me know if this is ok, I could work on this. Thanks.
fivesixzero commented 2 years ago

I've been looking closely at this driver this week and in my dev environment I've been experimenting with several things along the lines of @jposada202020 's proposals above.

This is a pretty interesting (and occasionally very finicky) sensor with a ton of config options, many of which aren't entirely intuitive. Exposing all of these to users could be useful but it'd cause the library to grow quite a bit in size which isn't so useful. But I've been looking into options for implementing (or improving implementation of) a few critical ones, like the integration time mentioned here for the color/light sensor, that could have a pretty big impact on "normal use case" reliability. Finding sensible defaults (overridable by well-documented kwargs) is also part of that too.

It'll probably be a bit before I emerge from the rabbit hole with a PR or two. Its a bit time consuming since I want to make sure to test all three core features (prox, color/als, gesture) independently as well as together. I'm also looking at fully implementing the four internal interrupts, the configurable external interrupt, and proper interrupt clear for all of those. So far I've tested out the proximity interrupts and they're really useful, with the caveat that setting things up is a bit counter-intuitive at first. Should be a fun, educational journey. :D

FoamyGuy commented 2 years ago

I think this has been resolved by #39