Replace enum integration times with uint8_t.

adafruit / Adafruit_TCS34725

Arduino library driver for Adafruit's TCS34725 RGB Color Sensor Breakout

http://www.adafruit.com/products/1334

Other

149 stars 149 forks source link

Replace enum integration times with uint8_t. #37

Closed itsayellow closed 3 years ago

itsayellow commented 4 years ago

Some of the enum's for setting ATIME (names TCS34725_INTEGRATIONTIME_*MS for tcs34725IntegrationTime_t) had the wrong integration times listed.

added some enum names that include the proper integration times
kept the old enum names as duplicate names for the same values, even though their listed time didn't match the actual integration time, in order not to break legacy code.
fixed the delays in the case statements in Adafruit_TCS34725.cpp to match the (rounded-up) actual integration times. This has the benefits of waiting the appropriate amount of time (not too long as before.) e.g. it used to delay 700ms for a 614.4ms integration.

Note that the datasheet in Table 6 incorrectly lists 700ms as the integration time for ATIME=0, even though this disagrees with the integration time formula in the datasheet section "RGBC Operation". I alerted AMS to the error and they agreed this was an error and replied that it would be fixed in the next version of the datasheet.

itsayellow commented 4 years ago

Thanks! I still have to test the integration_time example code myself in the hardware, although it does compile fine. I've tested similar code (on my own Adafruit_TCS34725 branch) and it seems to work well.

itsayellow commented 4 years ago

Ok, I finally had a chance to run the new integration_time example code on my Uno. It looks solid and correct to me! Let me know if you have any issues.

itsayellow commented 3 years ago

@siddacious any ETA on the merge of this? It's been patiently waiting a while... 😄

ladyada commented 3 years ago

hi i will be reviewing it - i dont understand why the enum was replaced with a uint8_t which risks folks using any number rather than the predefined values. you can always cast the enum back to a uint8_t for calculations

itsayellow commented 3 years ago

hi i will be reviewing it - i dont understand why the enum was replaced with a uint8_t which risks folks using any number rather than the predefined values. you can always cast the enum back to a uint8_t for calculations

It's been a long time, but if I'm remembering correctly...The register itself will take any value, not just the list of enums. In addition, the enums weren't useful times. It seems like they were just copied from the datasheet where the datasheet gives "examples" of register values and corresponding times. In my code it was far more useful to work with a value than an (arbitrary) set of enum choices.

ladyada commented 3 years ago

oooh ok. alright that makes more sense. did you test this PR with hardware and which hardware if so?

itsayellow commented 3 years ago

oooh ok. alright that makes more sense. did you test this PR with hardware and which hardware if so?

I did test. I tested it with an Arduino Uno R3 and Adafruit TCS34725

caternuson commented 3 years ago

Sorry. Just now seeing this.

To further document reasoning - the ATIME register does just take a uint8_t which sets integration time in 2.4ms intervals: although in a somewhat backwards and confusing way, 0xFF is the "lowest" integration time. Also, at 0x00 not sure how 256 * 2.4ms = 614.4ms ends up being 700ms? The math for the other values in the example table works out OK.

But in general, does make sense to just be able to set any uint8_t value. And the #defines are just convenient pre-set values. I also tested on a UNO and it works fine. Going to approve and merge.

Thanks @itsayellow and sry this tooks so long.