androidthings / contrib-drivers

Open source peripheral drivers
Apache License 2.0
558 stars 174 forks source link

Fixed readLedStatus to return current LED status rather than transition status #110

Closed Gadgetoid closed 6 years ago

Gadgetoid commented 6 years ago

Working from https://cdn-shop.adafruit.com/datasheets/CAP1188.pdf, page 43 gives an overview of the LED Status register - currently used in this library to get the current status of LEDs - and describes it as "The LED Status Registers indicate when an LED has completed its configured behavior"

IE: if you set up an LED to transition/fade slowly between on/off states then this register will indicate when that transition has completed.

I have corrected readLedStatus() to read the REG_LED_CONTROL register which will correctly reflect the current desired state of the LED, although it will not always represent the actual state of the LED since it may be transitioning.

I have preserved the original transition status functionality in readLedTransitionStatus() and better documented its expected behaviour.

This was tested on a Pimoroni Drum HAT/Cap1188- however this required another tweak to pass a custom i2c address which I will submit in a separate PR.

googlebot commented 6 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
Gadgetoid commented 6 years ago

I signed it!

googlebot commented 6 years ago

CLAs look good, thanks!

proppy commented 6 years ago

I wonder if it's possible to add tests that break with the previous/buggy behavior? Or if it's not necessary because this is mostly due to a miss-interpretation of the datasheet. /cc @jdkoren

Gadgetoid commented 6 years ago

I don't believe a test would be necessary since this is unlikely to be regressed, but one could be implemented like so as long as a hardware device is connected and LED linking and fading is disabled:

setLedStatus(0, true);
setLedStatus(1, true);
check if getLedStatus() & 0b00000011 == 0b00000011

Under certain circumstances it's still possible for the LED_STATUS register to match the contents of the LED_CONTROL register, even though they are both describing different things.

mangini commented 6 years ago

Hey, @sarahmwittman , can you please review this CL?