Skip to content

Custom SMUX configuration #10

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

Open
wants to merge 11 commits into
base: stephenf7072
Choose a base branch
from

Conversation

stephenf7072
Copy link
Contributor

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
Copy link
Contributor Author

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

@siddacious
Copy link
Contributor

Yeap, printf is not officially supported :\

@stephenf7072
Copy link
Contributor Author

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).

/**
* @brief Debugging function, prints the local custom SMUX configuration
*
* @param inputPixel (use a macro, like "ADC0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the rest of the documentation blocks will need to be updated before merge

@siddacious
Copy link
Contributor

@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
Copy link
Contributor

@siddacious
Copy link
Contributor

@stephenf7072
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

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

  • Printf statements
  • Documentation blocks (what exactly needs changing?)

Anything else?

@siddacious
Copy link
Contributor

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
Copy link
Contributor Author

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

@christakahashi
Copy link

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
Copy link
Member

ladyada commented Apr 10, 2021

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

@christakahashi
Copy link

@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
Copy link
Contributor Author

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.

@leCandas leCandas mentioned this pull request Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants