Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kevinjwalters opened this issue Mar 4, 2020 · 6 comments

Comments

@kevinjwalters
Copy link

kevinjwalters commented Mar 4, 2020

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
Copy link
Member

ladyada commented Mar 4, 2020

ya could add integration_time_ms if you'd like :)

@evaherrada
Copy link
Collaborator

Is this a duplicate of #24?

@kevinjwalters
Copy link
Author

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
Copy link

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
Copy link

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
Copy link

FoamyGuy commented Jan 8, 2022

I think this has been resolved by #39

@FoamyGuy FoamyGuy closed this as completed Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants