Skip to content
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

Handle analogRead(n) for n in 8..15 #238

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

WestfW
Copy link

@WestfW WestfW commented Jul 18, 2020

Let's try this again...

The code in analogRead() does not not handle integer arguments of 8..15, which are valid analog pins on Grand Central. Worse, it will do "bad things", like setting PINMUX to "analog" on pins that don't even have analog support.
#236

@WestfW
Copy link
Author

WestfW commented Jul 18, 2020

This "bug" exists on other platforms with more than 8 analog inputs as well, since the analogRead() code only adds the offset for n<=5, n=6, and n=7.
On all of them, A0..An are contiguous, so it doesn't NEED the (p-8)+A8 math. But it won't hurt, either. (and actually, it gets eliminated by the optimizer.)

@WestfW
Copy link
Author

WestfW commented Jul 20, 2020

Edit: Never mind. It was my own sketch that was complaining about an undefined PIN_A8.
I've confirmed that the changed version (as per the PR) compiles on all of the Adafruit SAMD platforms...

Hmm. feather m4 doesn't compile. It has NUM_ANALOG_INPUTS less than 8, so it shouldn't invoke the code that it's complaining about.
Does the preprocessor handle #if 6u > 8 correctly?
I don't understand why NUM_ANALOG_INPUTS has to be explicitly unsigned in the first place?

@ladyada
Copy link
Member

ladyada commented Oct 30, 2020

is this ready for review?

@WestfW
Copy link
Author

WestfW commented Nov 3, 2020

It WAS ready, and I'm confident that it works for GCM4 and such, but I've recently become a bit worried that it might not work on all the new platforms with >8 analog inputs :-(

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.

2 participants