Skip to content
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

Gesture refactor, memory optimization, major doc updates #39

Merged
merged 14 commits into from
Jan 8, 2022
Merged

Gesture refactor, memory optimization, major doc updates #39

merged 14 commits into from
Jan 8, 2022

Conversation

fivesixzero
Copy link

@fivesixzero fivesixzero commented Dec 2, 2021

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

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. :)

Copy link

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing @fivesixzero. Thank you so much for all of the work you've put into this this.

I found one small typo that I'd like to fix, I'm going to add one commit to do that.

I successfully tested all examples on a CLUE.

README.rst Outdated Show resolved Hide resolved
Copy link

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. This is a great improvement to the library and documentation. Thank you @fivesixzero for everything you put into this!

I tested the examples successfully on a CLUE. I also built the Proximity Trinkey build using the updated library to try to ensure the size would still fit. As far as I can tell the build worked successfully. I flashed the built UF2 onto a Trinket M0 and verified in the REPL that importing this library does work. I don't have the Proximity Trinkey device to test the build on the real hardware though.

@fivesixzero
Copy link
Author

@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. 😸

@fivesixzero fivesixzero deleted the cleanup-and-doc branch January 8, 2022 19:57
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 9, 2022
Updating https://github.com/adafruit/Adafruit_CircuitPython_APDS9960 to 3.0.0 from 2.2.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_APDS9960#39 from fivesixzero/cleanup-and-doc
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_MMA8451 to 1.3.10 from 1.3.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_MMA8451#21 from tekktrik/doc/add-typing
  > Merge pull request adafruit/Adafruit_CircuitPython_MMA8451#22 from tekktrik/doc/rtd-formatting

Updating https://github.com/adafruit/Adafruit_CircuitPython_OV5640 to 1.0.4 from 1.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_OV5640#12 from tekktrik/doc/add-typing

Updating https://github.com/adafruit/Adafruit_CircuitPython_PIOASM to 0.5.3 from 0.5.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_PIOASM#33 from dannystaple/patch-2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants