adafruit / Adafruit_CircuitPython_APDS9960

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

Gesture refactor, memory optimization, major doc updates #39

Closed fivesixzero closed 2 years ago

fivesixzero commented 2 years ago

Apologies for the rather large PR, but I figured this would be less painful to review than a dozen smaller PRs over the same (or longer) timeframe.

This PR supersedes #37, which was a first optimization pass, and #38, which produced much more reliable gesture results but resulted in a more complex lib and a much larger mpy file size and and memory footprint.

The primary goals are consistent with the prior PR though and should help to close all of the open issues.

  1. Current code returns very unpredictable gesture results
  2. The gesture engine loops continuously after a gesture() call, effectively locking up the sensor until power cycle
    • 23

  3. Data read from the gesture FIFOs doesn't consider overflow state or the possibility we may be reading in data mid-stream
  4. Register configuration is incomplete and largely undocumented with some confusion-inducing inaccuracies
    • 17

    • 24

    • 30 (maybe)

I did my best to break the work up into individual commits that should make it easier to review each change individually along with proving for reference points during my optimization testing.

This work can be broken down into four major parts: Optimization, Register Accessors, Gesture, and Docs.

Part 1: Optimization/Testing

TL;DR: Final commit (4c97c60) results in a filesize increase of ~200 bytes and a memory footprint reduction of ~3-4k (about 30%).

For optimization testing, I compiled via mpy-cross directly to a Proximity Trinkey and QTPy RP2040 with an APDS-9960 breakout attached. This allowed me to quickly get a look at the compiled file size as well as the memory footprint as well as offering the ability to quickly test any code or config changes in-situ.

I also had a Clue set up for more comprehensive functionality testing for each major optimization, which was handy for color detection tests with the Clue's fancy white LEDs.

The results of the file size and memory footprint tests in this table should (hopefully) help to visualize the impact of each major set of changes.

Commit mpy size mpy delta mem SAMD21 mem delta mem RP2040 mem delta Note
2c5ee6a 3,839 baseline 8,944 baseline 9,904 baseline Current bundle, baseline
2c5ee6a 3,854 -15 8,960 -16 9,872 -32 v2.2.8, latest mpy-cross
064fd99 6,831 +2,992 OOM :( OOM :( 15,296 +5,392 First refactor, #38
424e72c 3,400 -439 8,480 -464 12,480 +2,576 First optimization pass, #37
30eeadd 4,709 +870 6,944 -2,000 6,880 -3,024 Removed register use
9e335a0 3,634 -205 6,000 -2,944 5,488 -4,416 Removed non-essential methods/props
999945e 3,528 -311 5,792 -3,152 5,424 -4,480 Refactored/tested init reset/defaults
faa7969 3,798 -41 5,568 -3,376 5,456 -4,448 gesture() rewrite
c02b1df 3,988 +149 5,872 -3,072 5,776 -4,128 Color engine fixes
4c97c60 4,053 +214 5,936 -3,008 5,840 -4,064 Major docstring and docs/examples updates

My more detailed notes on optimization can be found here:

https://github.com/fivesixzero/circuitpython-experiments/blob/main/clue-nrf52/apds9960-testing/APDS-9960-optimization-notes.md

Part 2: Register Access Refactor

This was a pretty straight-forward memory optimization task with the exception of single-bit or multi-bit value access. I dug into the adafruit_register code to see how the magic happened there and implemented a relevant sub-set of that functionality as instance methods. I kept tabs on I2C comms with a logic analyzer to make sure the right bits were always in the right place, no matter how I tried to break things.

In the end, this was a trade-off. Adding the new instance methods resulted in a pretty big bump in mpy compiled file size but the overall memory footprint was reduced dramatically. I took it as a win but if mpy file size optimization is more important than memory footprint reduction then its an easily reversible fix.

For the record, I would've preferred having all register accessors set up with adafruit_register since it leads to much more readable, resilient code. But the memory footprint cost was just too high to justify given that this needs to work well on low-memory platforms like the SAMD21 Proximity Trinkey. Like everything else here though, this is all up for review/discussion. :)

Oh, and a lot of testing time was put into finding appropriate defaults that should (hopefully) fit 99% of use cases for this driver, along with making sure the driver has a solid "reset" on init. Some notes on what the registers look like can be found here:

https://github.com/fivesixzero/circuitpython-experiments/blob/main/clue-nrf52/apds9960-testing/APDS-default-registers-notes.md

Part 3: gesture() Refactor

This was a long, winding, and ultimately very educational journey!

TL;DR is that the new gesture code is a lot more effective while being as efficient as possible. I took a lot of inspiration from other implementations out there, particularly the Sparkfun APDS-9960 Arduino library.

We could potentially have a much more useful driver for human interaction if we had some C code running in the background handling gestures, like keypad but running a lightweight polling/analysis loop for instance. I only did a very small amount of testing to validate the concept and it seems sound. But I'm not sure it'd be worth the work to implement for a device only built in on 3 boards. I don't think this fits into the fancy new async/await model very well either but that could easily be a lack of creativity on my part.

But hey, it's pretty cool to just wave your hand in the air and get your CircuitPython code to do a thing reliably, yeah? Just can't be done without pin interrupt callbacks or background worker type stuff. Maybe.

For now though this is fine, the gesture-simpletest works better, and memory footprint is still sane. :)

BTW, if you're curious about gesture recognition with this sensor, this Jupyter Notebook might be interesting.

https://github.com/fivesixzero/circuitpython-experiments/blob/main/clue-nrf52/apds9960-testing/APDS-9960-gesture-data-playground.ipynb

Part 4: Docs

Aside proximity operation this is a pretty quirky little sensor with some counterintuitive behaviors. And to make matters more fun its most useful features - gesture and color/light - require the implementer to do a lot of work to make the sensor's data useful.

With that in mind I did a thorough rework of the library's documentation to make sure that it's comprehensive enough to explain some of the works while not inducing information overload. I also spent a ton of time with the datasheet and real-world testing to make sure that the explanations are as accurate as possible. Hopefully that helps?

That's It

Thanks for taking a look at this. Hope it helps. TBH going down this rabbit hole has been a welcome respite while dealing with the loss of a loved one, so hopefully that explains the seemingly ridiculous amount of attention I've given this in the last month or so. :)

fivesixzero commented 2 years ago

@FoamyGuy Thank you for taking the time to wrestle with this massive PR! I was working mostly in isolation so its very reassuring to hear that your testing was succesful as well.

Here's to getting 2022 off to a great bug-squashing start. 😸