Workaround for "ADC disables digital input" not working

[spencekonde]

Workaround for "ADC disables digital input" not working

[SpenceKonde]

Huh. That should definitely work, so either the workaround is not implemented correctly there, or the workaround doesn't work there. Either way, it needs to be fixed for 1.3.0.

[technoblogy]

PD0 only works as an input if I do:

analogRead(7);

or disable the ADC with;

ADC0.CTRLA &= ~(ADC_ENABLE_bm);

[technoblogy]

By the way, I'm on DxCore version 1.2.0. I assume that's more recent than 1.2.0-a.

[SpenceKonde]

1.2.0 and 1.2.0-a are the same version. -a pulls in a new compiler toolchain to give support for the smaller AVRxDA parts and AVR128DB, but only works on windows and maybe mac, not linux because of some stupid problem i don't understand....

[SpenceKonde]

What value do you see for ADC0.PORTMUX after startup? The workaround should be setting it to 0x7F (127), and that worked in my testing; if it is set to that, it sounds like the ADC on some parts doesn't like the fact that I was setting it to a "reserved" value and is treating it as 0... If it isn't set to 0x7F, it sounds like the workaround just isn't being used. though that in and of itself is strange; I don't see how that could be the case.

[technoblogy]

Do you mean ADC0.MUXPOS and ADC0.MUXNEG?

[SpenceKonde]

In my tests, only MUXPOS mattered - actually, we know MUXNEG doesn't matter, since analogRead() doesn't touch it.

[SpenceKonde]

Argh, yes. ADC0.MUXPOS

[technoblogy]

ADC0.MUXPOS = 127
ADC0.MUXNEG = 0

So perhaps MUXNEG is doing it?

Anyway, I think setting to a reserved value is dangerous. It may have an effect, such as shadowing another value, or it may be used in the future. Why not set both to GND?

[SpenceKonde]

Interesting. Try setting ADC0.MUXPOS=0x40 (ground, which was what I was going to suggest). I think the reason I didn't do that was that I wanted something floating, but it turns out that it doesn't get connected to the sampling cap unless you do a reading with it anyway.
No need to touch MUXNEG - analogRead(7); wouldn't modify MUXNEG either (in fact, we never touch that anywhere in the core); setting that just wastes 3 instruction words.

[SpenceKonde]

And.... waaaaitaminute.... analogRead(), with the workaround enabled, sets it back to 0x7F... so something doesn't add up here....

[technoblogy]

ADC0.MUXPOS = 0x40;

doesn't fix the problem, nor does:

 ADC0.MUXPOS = 0x40; ADC0.MUXNEG = 0x40;

[technoblogy]

What about disabling the ADC with:

ADC0.CTRLA &= ~(ADC_ENABLE_bm);

until the user calls analogRead(...).

[SpenceKonde]

That's possible. I wonder what the key is - if we can avoid it, I'd rather not add more to analogRead() than we have to....

I wonder if either of these would do the trick in setup()?

ADC0.COMMAND=0x02;

ADC0.COMMAND=0x03;

ADC0.COMMAND=0x01;
ADC0.COMMAND=0x02;

[technoblogy]

ADC0.COMMAND=0x01;

seems to do the trick. So this whole test works:

void setup() {
  ADC0.COMMAND=0x01;
  pinMode(12, INPUT_PULLUP);              // PD0
  pinMode(7, OUTPUT);                     // PA7
}

void loop() {
 if (digitalRead(12) == HIGH) digitalWrite(7, HIGH); else digitalWrite(7, LOW);
 delay(1000);
}

[technoblogy]

Full list:

ADC0.COMMAND=0x01; // Works

ADC0.COMMAND=0x01; ADC0.COMMAND=0x02; // Works

ADC0.COMMAND=0x02; // Doesn't work

ADC0.COMMAND=0x03; // Doesn't work

I'm going to stop for the night now, but will be around tomorrow if you need more tests!

[SpenceKonde]

Sounds like

ADC0.COMMAND=0x01; 
ADC0.COMMAND=0x02;

it is.

(just setting it to 0x01 and forgetting about it is no good - that will make the first ADC reading you take come out wrong).

[SpenceKonde]

Actually, huh... it would make every analogRead() wrong, wouldn't it (setting one pin high, another one low, and alternately reading them should demonstrate this if my understanding is correct)... that's actually kinda nasty isn't it.... On second thought, if someone does anything that generates an analog result, and doesn't clean up after themselves before calling analogRead, they deserve what they get; manually manipulating a peripheral voids any promise of the relevant API functions returning correct results.

[technoblogy]

I'm confused - according to the Silicon Errata, calling analogRead(12) should stop subsequent digital input from working on that pin, whatever fix you've done in setup. However, this works OK (on 1.2.0):

void setup() {
  pinMode(7, OUTPUT);                     // PA7
  pinMode(12, INPUT_PULLUP);              // PD0
  analogRead(12);
}

void loop() {
 if (digitalRead(12) == HIGH) digitalWrite(7, HIGH); else digitalWrite(7, LOW);
 delay(1000);
}

ie the LED on pin 7 follows the state of pin 12.

[SpenceKonde]

That's because the workaround already moves ADC0.POSMUX away after every analogRead().

The only current bug is that the workaround does not get applied until a conversion is started - even if we then immediately stop it!

With
ADC0.COMMAND=0x01;
ADC0.COMMAND=0x02;
in setup
can you verify that after a delay(10); ADC0.INTFLAGS is 0?
If it is, then i'll check in that fix - all without actually getting out any of my own hardware. Which I'm not allowed to do until I finish a bunch of other action items!

[SpenceKonde]

Er, rather, that the workaround doesn't actually successfully work around the errata until a conversion has been started, even if we immediately stop it.

[technoblogy]

With
ADC0.COMMAND=0x01;
ADC0.COMMAND=0x02;
in setup
can you verify that after a delay(10); ADC0.INTFLAGS is 0?

Confirmed.