adafruit / Adafruit_AS7341

Other
29 stars 18 forks source link

Custom SMUX configuration #10

Open stephenf7072 opened 3 years ago

stephenf7072 commented 3 years ago

I've added a bunch of functions to abstract the complicated stuff away from setting custom SMUX configurations. These can also be saved as uint32_t config "words" for each ADC. Pretty heavy stuff, but I have have double checked all the underlying definitions against the SMUX datasheet and am confident in them.

There is also an optional argument added to the startReading() function (and changes to the checkReadingProgress() function), which will run the two integrations with low and high channels (as it previously did), or a single integration of high, low, or a custom SMUX configuration.

There is a detailed example showing how everything works. I don't have hardware to test it exactly as it is, I tested it on something a little more complex (although it does at least compile for me). A confirmation that it works for someone else would be appreciated.

It should be noted that it's important to use the macros rather than numbers. Ie. PIXEL1 != 1, and ADC0 != 0. It occurs to me that this could be protected against by enumerated custom types, but given it's an advanced functionality, and there is array referencing/indexes involved, I think it best to leave as-is.

stephenf7072 commented 3 years ago

Has anyone seen this? The checks fail because printf is (surprisingly?) not recognised for some platforms.

siddacious commented 3 years ago

Yeap, printf is not officially supported :\

stephenf7072 commented 3 years ago

I'm probably more interested to hear what you think of my improvements and SMUX configuration - if you have an intention of incorporating it I'll remove the printf stuff (it's only in the example).

siddacious commented 3 years ago

@stephenf7072 Based on what I see in the example, I'm definitely inclined to work towards a merge. Before I ask for a bunch of changes however, I've got some code to share that might shed some light on the less-well documented parts of the SMUX API.

Hold tight!

siddacious commented 3 years ago

@stephenf7072 https://github.com/siddacious/auto_taco/blob/master/chiplib_src.zip

siddacious commented 3 years ago

@stephenf7072 https://github.com/siddacious/auto_taco/blob/master/chiplib_src.zip

@stephenf7072 lmk when you've seen this

stephenf7072 commented 3 years ago

Seen it, cheers, and had a very quick skim. Alas, I didn't know that existed - I was working off the SMUX datasheet which I got from AMS (linked in the example I think). Better be upfront - I don't have time at the moment for major rework and testing.

siddacious commented 3 years ago

@stephenf7072 No worries; I don't think there is anything in that code that invalidates anything you've done, I was just interested in sharing some info I was able to get from AMS with another fan of the sensor ;)

stephenf7072 commented 3 years ago

Ah yep, cool. So items on the merging punchlist are:

Anything else?

siddacious commented 3 years ago

That should be it; it looks like you might need to revisit the argument names in the documentation blocks as it looked like some didn't match. I'd recommend using the instructions and info in the README to use doxygen to test locally before pushing. If you do so, I'd use our doxyfile here: https://raw.githubusercontent.com/adafruit/travis-ci-arduino/master/Doxyfile.default

stephenf7072 commented 3 years ago

Ok thanks, I'll give it a go when I get a chance.

christakahashi commented 3 years ago

Is this getting merged any time? I was going to add my own implementation but if someone's done it I'd rather not bother.

ladyada commented 3 years ago

hi it would be most helpful if you can test this PR and give feedback - testing and verificaiton is what takes us time :)

christakahashi commented 3 years ago

@ladyada Seems to work. setting up smux entries and then calling startReading and polling progress. I get numbers I expect from the channels I've selected, given my setup.

I didn't test the configword stuff. it seems needlessly complicated for a benefit I don't understand. Seems more efficient to store the 20 register bytes exactly (vs 6*4 bytes for the "configword"s) or more readable to write function calls connecting pixels to ADCs. I also didn't test the print functions and don't agree that library functions should have printing as side effects (except in obvious cases like printing libraries).

stephenf7072 commented 3 years ago

Finalising the small details for the merge is still on my to-do list but I've had too many other things on (and still do). Thanks for your interest though @christakahashi, I'm glad it was useful to someone. The config word stuff is perhaps fairly specific to my end application, storing the configuration as a uint32_t makes the process of reading a config word from a CSV file (or as stored in EEPROM) more portable/generic, if a bit less efficient.

Ie. I have a series of tests with parameters according to lines in a CSV file. Things like aTime, aStep are single fields. SMUX config is also a single field (the uint32_t config word) which can be generated using Excel or a small Arduino sketch which leverages printLocalSMUXConfig().

I'm pretty sure the only printing instances are in dedicated printing functions. It's just that printf isn't supported by all Arduinos.