Replace enum integration times with uint8_t.

[itsayellow]

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.

[siddacious]

I believe this is incorrect, and the proper value would be 51ms, unless I'm misreading something:

>>> from math import ceil
>>> for lsb in [0xFF, 0xF6, 0xEB, 0xD5, 0xC0, 0x00]:
...     cycles=(0x100-lsb)
...     int_time = cycles * 2.4
...     print("LSB:%d, cycles:%d, integration time: %.1f, delay(ms): %d"%(lsb, cycles, int_time, ceil(int_time)))
...
LSB:255, cycles:1, integration time: 2.4, delay(ms): 3
LSB:246, cycles:10, integration time: 24.0, delay(ms): 24
LSB:235, cycles:21, integration time: 50.4, delay(ms): 51
LSB:213, cycles:43, integration time: 103.2, delay(ms): 104
LSB:192, cycles:64, integration time: 153.6, delay(ms): 154
LSB:0, cycles:256, integration time: 614.4, delay(ms): 615

The same would apply to TCS34725_INTEGRATIONTIME_101MS which would be TCS34725_INTEGRATIONTIME_104MS.

The corresponding delay times would need to be adjusted as well.

[itsayellow]

That's weird, the comment says 20 cycles, but the hex is not right for that. Same with the comment saying 42 cycles but is really 43 cycles.
Sorry I was looking at the comment and should've verified the hex.

One question, do we want to stick with the hex values and change the times, comments? Or do we want the intention of the comments preserved and change the hex value?

[itsayellow]

Some nice times to have would be multiples of 100ms, because both 50Hz and 60Hz periods divide evenly into 100ms.

I'm not sure where some of these times came from, they don't seem useful, they just seem like they happened to be on one chart in the datasheet.

[itsayellow]

How about adding these, they would be useful:

hex	integ (ms)	60Hz cycles	50Hz cycles
D6	100.80	6.05	5.04
AD	199.20	11.95	9.96
83	300.00	18.00	15.00
59	400.80	24.05	20.04
30	499.20	29.95	24.96
06	600.00	36.00	30.00

for i in range(255):
    register = 256-i
    integ = i*2.4
    hz60cycles = integ*1e-3 / (1/60)
    hz50cycles = integ*1e-3 / (1/50)

    print(f"hex {register:02X}    integ {i*2.4:.2f}    60Hz cycles {hz60cycles:.2f}    50Hz cycles {hz50cycles:.2f}")

[siddacious]

I agree that these values seem somewhat arbitrary. As far as I can tell the register in question takes a uint8_t, so there shouldn't be a reason to restrict it to an enum.

Would you like to take a crack at updating the code to take a uint8_t instead of tcs34725IntegrationTime_t?

[itsayellow]

Sure! Maybe I'll combine the code from my other pull request (#38) into this one, it seems like it should be part of that?

[siddacious]

I agree, that would make sense.

[itsayellow]

It's been a while since I've done C++ and I have a question: I'm guessing having the helpful tags like TCS34725_INTEGRATIONTIME_2_4MS are nice, even if we don't need to have them be part of an enumerated type. After making the ATIME register an int, should I make them #defines? Or should I make them consts? It seems nice to have them around, but I'm not sure what's the good C++ style way of doing it

[siddacious]

C++ isn't the sharpest tool in my toolbox either so I can't speak to there being a good/better way to do it, but I think the most important thing is that the documentation (inline and examples) show how to use the method and how different argument values will change the behavior.

I agree that the tags are helpful to make the abstract LSB value understandable, however I don't have a strong opinion about how much effort should be put into making them available. The important thing as above is that the behavior is well documented and understandable.
I'm happy with either of these options:

• change the argument, remove the enum and replace the entries with corrected macros as discussed
• Change the argument type and the argument (and possibly method) name to be the number of additional integration cycles. The docs for the argument would mention that the argument is adding additional 2.4ms integration cycles to the default of 1

My preference would be for the later but either is fine. In either case I think setIntegrationTime or its successor deserves its own example.

[itsayellow]

I picked the former. Both options have a little bit of weirdness (256-value) or (value+1). I went with the one that matches the datasheet so that at least the weirdness is consistent with the documentation. In addition there are copious #defines so if you just want to set it to a constant they are all documented for you.

[siddacious]

This code looks good to go. I've got a sensor on the way so I'll merge after testing.

Great work :)

[itsayellow]

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]

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]

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

[ladyada]

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]

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]

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

[itsayellow]

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]

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.