-
Notifications
You must be signed in to change notification settings - Fork 146
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
Replace enum integration times with uint8_t. #37
Conversation
Adafruit_TCS34725.h
Outdated
@@ -143,14 +143,18 @@ typedef enum { | |||
0xFF, /**< 2.4ms - 1 cycle - Max Count: 1024 */ | |||
TCS34725_INTEGRATIONTIME_24MS = | |||
0xF6, /**< 24ms - 10 cycles - Max Count: 10240 */ | |||
TCS34725_INTEGRATIONTIME_48MS = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 #define
s? Or should I make them const
s? It seems nice to have them around, but I'm not sure what's the good C++ style way of doing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks good to go. I've got a sensor on the way so I'll merge after testing.
Great work :)
Thanks! I still have to test the |
Ok, I finally had a chance to run the new |
@siddacious any ETA on the merge of this? It's been patiently waiting a while... 😄 |
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. |
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 |
Sorry. Just now seeing this. To further document reasoning - the ATIME register does just take a But in general, does make sense to just be able to set any Thanks @itsayellow and sry this tooks so long. |
Some of the enum's for setting ATIME (names
TCS34725_INTEGRATIONTIME_*MS
fortcs34725IntegrationTime_t
) had the wrong integration times listed.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.