drivers: add driver for APDS99XX ambient light and proximity sensors by gschorcht · Pull Request #10420 · RIOT-OS/RIOT

gschorcht · 2018-11-17T10:32:27Z

Contribution description

This PR adds a driver for Broadcom APDS99XX ambient light and proximity sensor series. It supports the following sensors:

Sensor	Driver variant	Proximitiy [counts]	Ambient light [counts]	RGB [counts]	Illuminance [Lux]
APDS9900	`apds9900`	x	x	-	x
APDS9901	`apds9901`	x	x	-	x
APDS9930	`apds9930`	x	x	-	x
APDS9950	`apds9950`	x	x	x	-
APDS9960	`apds9960`	x	x	x	-

The driver variants for the various sensors are defined as pseudomodules. To use the driver, the sensor used must be specified by the application by one of these pseudomodules, e.g.,

USEMODULE += apds9900

The driver functionality is divided in two parts:

Basic driver apds99xx
The base driver apds99xx only supports a basic set of functions and has therefore a smaller size. The procedure for retrieving new data is polling. Ambient light and proximity sensing are supported.
Fully functional driver apds99xx_full
The fully functional driver apds99xx_full supports all the features supported by the base driver, as well as all other sensor features, including interrupts and their configuration. Data-ready interrupts can be used to retrieve data. In addition, threshold interrupts can be used and configured.

For each of the two driver variants there is a corresponding test application: tests/driver_apds99xx and tests/driver_apds99xx_full.

The basic driver apds99xx is automatically enabled when the driver variant for the used sensor is specicied.

Testing procedure

To test the driver, the test application can be used without interrupts as following

USEMODULE=apds9960 make flash -C ```tests/driver_apds99xx``` BOARD=...

or with interrupt

USEMODULE=apds9960 make flash -C ```tests/driver_apds99xx``` BOARD=...

where the sensor used has be specified accordingly in the USEMODULE statement.

Issues/PRs references

Fixes issue #10164

drivers/include/apds99xx.h

leandrolanzieri

Nice work! Here are some comments, I'm still reviewing it though, it's a pretty big driver. I will also try to get a sensor to test this.

drivers/apds99xx/apds99xx.c

leandrolanzieri · 2018-12-18T20:30:51Z

drivers/apds99xx/apds99xx.c

+    #else
+    EXEC_RET(_reg_write(dev, APDS99XX_REG_PPCOUNT, &dev->params.prx_pulses, 1));
+    EXEC_RET(_update_reg(dev, APDS99XX_REG_CONTROL, APDS99XX_REG_PDIODE, 2));
+    #endif


Here and above, please add the comment of the corresponding tag (/* MODULE_APDS9960 */)

leandrolanzieri · 2018-12-19T10:22:07Z

drivers/apds99xx/apds99xx.c

+                  __func__, d->params.dev, APDS99XX_I2C_ADDRESS, ## __VA_ARGS__);
+
+#define EXEC_RET(f) { \
+    int _r; \


It would be better not to assign the value to _r inside of the if statement. Also, is the intermediate variable really needed here and below?

Thanks for your review.

It would be better not to assign the value to _r inside of the if statement.

I'm wondering why? What is the reason.

Also, is the intermediate variable really needed here and below?

Yes, if we want to print out the error code in the debug message.
But more important, we have to return the error code we got from executed function in case of error.

drivers/include/apds99xx.h

drivers/apds99xx/apds99xx.c

leandrolanzieri

Just some little comments, otherwise looks good. I don't think I will be getting the sensor in time for testing this for the feature freeze. @aabadie any chances you could?

drivers/include/apds99xx.h

drivers/apds99xx/apds99xx.c

drivers/include/apds99xx.h

obgm · 2018-12-20T10:25:03Z

FWIW: I am currently running the provided tests with an APDS 9960. Both, the basic (polling) version as well as the int-controlled version seem to work as expected. (Did not check the color values, though.)

gschorcht · 2018-12-20T11:34:23Z

@leandrolanzieri Thanks for reviewing.

ArtaFakhari · 2019-08-09T18:13:25Z

Hello Everyone
Does the APDS-9960 work reliably on RIOT-OS?

benpicco · 2019-09-15T20:08:00Z

drivers/apds99xx/apds99xx.c

+    double ga = 0.49;   /* glas or lens attenuation factor */
+    double b = 1.862;
+    double c = 0.746;
+    double d = 1.291;


If you multiply them all by 1000, you wouldn't need to use double here, no?

dismissed stale review after 1 year

aabadie · 2020-02-03T20:09:09Z

Please rebase, I have the hardware to test this PR (apds9960) :)

drivers/apds99xx/apds99xx.c

drivers/include/apds99xx.h

sys/auto_init/saul/auto_init_apds99xx.c

smlng · 2020-03-05T10:39:10Z

tests/driver_apds99xx_full/main.c

+    apds99xx_t dev;
+
+    /* save the reference to the main thread */
+    t_main = (thread_t*)sched_threads[sched_active_pid];


better use t_main = thread_getpid(); here?

The reference to the main thread is required in apds99xx_isr. Using thread_get(pid) doesn't work since it replaces the const attribute.

I changed the interrupt handling to a mutex based approach.

smlng · 2020-03-05T10:42:44Z

drivers/apds99xx/apds99xx.c

+#endif
+
+    /* wait for 6 ms after power on reset */
+    xtimer_usleep(6 * US_PER_MS);


would be nice to get rid of xtimer usage, especially as this is the only one. Isn't there a state register or gpio that we could poll (while(...) {} style) instead?

The problem is that this time is specified in the data sheet. It is the time the sensor needs for initialization after power-on to become ready for operation. Unfortunately there is no status flag.

Possible solutions:

We simply assume that RIOT takes longer to load than this 6 ms before the device is accessed for the first time.

We allow multiple tries of the _is_available function and use the I2C ACK to determine when the device is available. However, at an I2C speed of 400 kbps, each failed attempt on the bus would only take 0.025 ms, which could mean a maximum of 250 failed attempts for 6 ms.

I have removed the dependency on xtimer with commit d909fad.

aabadie

I found some minor typos and small issues in the Python scripts (as reported by the CI).

Maybe also add ALS ==> ALSO in the list of codespell ignored words.

I'll give this PR a try later today.

tests/driver_apds99xx_full/tests/01-run.py

tests/driver_apds99xx/tests/01-run.py

sys/auto_init/saul/auto_init_apds99xx.c

drivers/include/apds99xx.h

aabadie · 2020-03-09T10:18:19Z

Also: please rebase :)

aabadie · 2020-03-09T10:30:45Z

I could test this PR on the adafruit-clue board (after some rebase and fix of conflicts). This board embeds an apds9960 variant. Here is what I get:

tests/driver_apds99xx works like a charm
tests/driver_apds99xx_full doesn't seem to work: no interrupt is generated when I get my hand close to the proximity sensor. I configured the interrupt pin following the board schematic, so it should work. Any idea what could be the cause @gschorcht ?

gschorcht · 2020-03-10T16:12:15Z

tests/driver_apds99xx_full doesn't seem to work: no interrupt is generated when I get my hand close to the proximity sensor. I configured the interrupt pin following the board schematic, so it should work. Any idea what could be the cause @gschorcht ?

@aabadie Do you see ALS measurements?

ambient = 639 [cnts]
red = 208 [cnts], green = 295 [cnts], blue = 263 [cnts]
+-------------------------------------+

If yes, interrupts are working since these measurements use the data ready interrupt. The proximity interrupt is triggered, when the proximity value exceeds 200 counts. This happens at a distance of about 2 cm. In that case, the output also shows the proximity value, for example:

proximity = 205 [cnts]
+-------------------------------------+
ambient = 322 [cnts]
red = 95 [cnts], green = 168 [cnts], blue = 135 [cnts]
+-------------------------------------+

gschorcht · 2020-03-10T17:17:46Z

@aabadie @smlng Thanks for reviewing. I have rebased the PR and hopefully addressed all your remarks.

aabadie · 2020-03-10T18:20:46Z

I have rebased the PR and hopefully addressed all your remarks.

Thanks! I'll have a look and test again tomorrow.

tests/driver_apds99xx_full/main.c

aabadie · 2020-03-11T08:55:48Z

Tested this PR again this morning on adafruit-clue and I can't have the interrupt based test application to work. If I comment out the mutex_lock, messages are displayed as expected: the proximity value is only displayed when I put my finger close to the sensor (which means the interrupt source bit is set).
So there's something wrong with the interrupt pin: I have no idea what could be the problem. I suspect something with the board: there are white leds that are on when waiting for the interrupt and they go down when an interrupt occurs.

I trust you that this test is working anyway.

drivers/Makefile.dep

sys/auto_init/saul/auto_init_apds99xx.c

gschorcht · 2020-03-11T11:55:14Z

Tested this PR again this morning on adafruit-clue and I can't have the interrupt based test application to work. If I comment out the mutex_lock, messages are displayed as expected: the proximity value is only displayed when I put my finger close to the sensor (which means the interrupt source bit is set).
So there's something wrong with the interrupt pin: I have no idea what could be the problem. I suspect something with the board: there are white leds that are on when waiting for the interrupt and they go down when an interrupt occurs.

The Adafruit-Clue seems to be a very interesting board. According to the schematic P0.09 should be the APDS_IRQ pin. GPIO_IN_PU mode is supported according to nrf5240 product specification. Might it be a problem that the pin could also be used as NFC1 pin, something that has to be considered during initialization? The WHITE_LED signal is controlled by P0.10/NFC2. Might it be that the pins are mixed up?

aabadie · 2020-03-11T12:41:14Z

Might it be that the pins are mixed up?

I had the same idea and tried to invert the pins but it didn't work either. I won't block this PR because of this anyway.

aabadie · 2020-03-11T13:09:45Z

I think we are good with this one. @gschorcht, please squash!

aabadie

ACK!

gschorcht · 2020-03-11T15:32:06Z

Thank you all for reviewing and testing.

BTW, I hope I can implement the gesture recognition function of APDS9960 in next week. I will provide it as separate PR.

gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Nov 17, 2018

gschorcht force-pushed the drivers_apds99xx branch from 0edd9e3 to 22f035b Compare November 17, 2018 10:47

gschorcht added the Area: SAUL Area: Sensor/Actuator Uber Layer label Nov 18, 2018

aabadie added this to the Release 2019.01 milestone Dec 3, 2018

obgm mentioned this pull request Dec 17, 2018

drivers: Broadcom APDS-99xx ambient light and proximity sensor driver #10164

Closed

obgm reviewed Dec 19, 2018

View reviewed changes

drivers/include/apds99xx.h Outdated Show resolved Hide resolved

leandrolanzieri requested changes Dec 19, 2018

View reviewed changes

aabadie reviewed Dec 19, 2018

View reviewed changes

drivers/apds99xx/apds99xx.c Outdated Show resolved Hide resolved

leandrolanzieri previously requested changes Dec 20, 2018

View reviewed changes

aabadie modified the milestones: Release 2019.01, Release 2019.04 Jan 25, 2019

miri64 modified the milestones: Release 2019.04, Release 2019.07 May 7, 2019

MrKevinWeiss modified the milestones: Release 2019.07, Release 2019.10 Jul 16, 2019

benpicco requested a review from leandrolanzieri September 15, 2019 20:04

benpicco reviewed Sep 15, 2019

View reviewed changes

benpicco mentioned this pull request Sep 16, 2019

core/bitarithm: introduce bitarithm_get/set_masked(), convert pca9685 #12238

Closed

kb2ma modified the milestones: Release 2019.10, Release 2020.01 Oct 9, 2019

fjmolinas modified the milestones: Release 2020.01, Release 2020.04 Jan 16, 2020

aabadie mentioned this pull request Feb 4, 2020

boards/adafruit-clue: add initial support #13276

Merged

gschorcht force-pushed the drivers_apds99xx branch from b5771d1 to 7584760 Compare February 11, 2020 09:16

gschorcht added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 21, 2020

smlng reviewed Mar 5, 2020

View reviewed changes

aabadie requested changes Mar 9, 2020

View reviewed changes

aabadie self-assigned this Mar 9, 2020

gschorcht force-pushed the drivers_apds99xx branch from f75e78d to b6e8ca9 Compare March 10, 2020 17:14

aabadie reviewed Mar 11, 2020

View reviewed changes

tests/driver_apds99xx_full/main.c Outdated Show resolved Hide resolved

aabadie reviewed Mar 11, 2020

View reviewed changes

drivers/Makefile.dep Outdated Show resolved Hide resolved

aabadie reviewed Mar 11, 2020

View reviewed changes

sys/auto_init/saul/auto_init_apds99xx.c Outdated Show resolved Hide resolved

gschorcht added 4 commits March 11, 2020 14:21

drivers: add APDS99XX ALS and proximity

7548a6e

sys/auto_init: add APDS99XX ALS and proximity

811850b

tests: add APDS99XX ALS and proximity

5f85088

tools/codespell: ignore ALS (Ambient Light Sensing)

bd3ccc4

gschorcht force-pushed the drivers_apds99xx branch from e372058 to bd3ccc4 Compare March 11, 2020 13:22

aabadie approved these changes Mar 11, 2020

View reviewed changes

aabadie merged commit 1e80376 into RIOT-OS:master Mar 11, 2020

gschorcht deleted the drivers_apds99xx branch March 11, 2020 15:32

Conversation

gschorcht commented Nov 17, 2018

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Dec 19, 2018

Choose a reason for hiding this comment

Uh oh!

gschorcht Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

obgm commented Dec 20, 2018

Uh oh!

gschorcht commented Dec 20, 2018

Uh oh!

ArtaFakhari commented Aug 9, 2019

Uh oh!

benpicco Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smlng Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

gschorcht Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

smlng Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

gschorcht Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschorcht Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aabadie commented Mar 9, 2020

Uh oh!

aabadie commented Mar 9, 2020

gschorcht Dec 19, 2018 •

edited

Loading

gschorcht Mar 10, 2020 •

edited

Loading

gschorcht commented Mar 10, 2020 •

edited

Loading

aabadie commented Mar 10, 2020 •

edited by gschorcht

Loading