Hello! I'm using ADC Free Running Mode to detect 3.4ms events.
If I have too much code in my ISR, I start getting very noisy readings when I read the ADC register.
Why would that be? Do I need to stop interrupts inside my ISR?
Should I slow down the sampling rate more (the
ADCSRA setting).
Another idea is to sum up readings of ADCH values and act upon every 16th average of ADCH, so the average ISR duration will be good although every 16th ISR will be slow. This could eliminate false readings I suspect.
I need to detect when the value is lower than a threshold and hasn't happened within a minimum debounce time. When I added this extra code (plus some more stuff) my values read from ADCH went haywire. Obviously there isn't a whole lot of time left in that ISR before the next one is triggered.
void setup() {
Serial.begin(9600);
Serial.println("hello");
ADCSRA = 0; // clear ADCSRA register
ADCSRB = 0; // clear ADCSRB register
ADMUX = (ADMUX & 0xF0) | 0x00; // We will start with pin A0
ADMUX |= (1 << REFS0); // set reference voltage
ADMUX |= (1 << ADLAR); // left align ADC value to 8 bits from ADCH register
// sampling rate is [ADC clock] / [prescaler] / [conversion clock cycles]
// for Arduino Uno ADC clock is 16 MHz and a conversion takes 13 clock cycles
ADCSRA |= (1 << ADPS2); // 16 prescaler for 76.9 KHz
ADCSRA |= (1 << ADATE); // enable auto trigger
ADCSRA |= (1 << ADIE); // enable interrupts when measurement complete
ADCSRA |= (1 << ADEN); // enable ADC
ADCSRA |= (1 << ADSC); // start ADC measurements
}
volatile uint8_t x[2];
uint8_t currentChannel = 0;
//// ADC interrupt service routine
ISR(ADC_vect) {
// Read the ADC value
x[currentChannel] = ADCH;
// Update to the next channel
currentChannel = (currentChannel + 1) % 2;
ADMUX = (ADMUX & 0xF0) | currentChannel;
}
I could submit more code if needed.
I really appreciate the help that is here in this forum.
The numbers were acting random. I wonder if the it was getting stacked up on each other. The Arduino didn't crash or anything. Just the sensor readings were gibberish.
Puting in an accumulator to sum 8 readings and acting upon the 8th value did the trick. That reduces the noise and only the 8th call is slow...
But thankfully for this forum's AI of finding similar topics, the problem was pointed out by @Robin2 ten years ago.
After switching ADMUX to the next port, there needs to be enough time to get a good reading. The trick is to do a throw away a reading and use the second.
ISR(ADC_vect) {
// Read the ADC value
x[currentChannel] = ADCH;
//Don't put extra code here
currentChannel = (currentChannel + 1) % 2;
ADMUX = (ADMUX & 0xF0) | currentChannel;
//Do put extra code here if needed
}
There needs to be enough time between changing the ADMUX register and reading ADC register. So to potentially keep that time as long possible put the additional code after the ADMUX change rather than before. Still need to test if this helps or not.
You may also try something like the following to discard the first conversion after a channel change:
ISR(ADC_vect) {
static bool discardNextConversion = true ; // static initialized Once only.
if ( discardNextConversion ) discardNextConversion = false ;
else {
// Read the ADC value
x[currentChannel] = ADCH;
// Update to the next channel
currentChannel = (currentChannel + 1) % 2;
ADMUX = (ADMUX & 0xF0) | currentChannel;
discardNextConversion = true ;
}
An ISR must be short so any additional processing of x[ ] should, if possible, be done in the loop(). Bulking out the ISR to delay the capturing of a sample is not a good idea and will anyway probably have no real effect. Instead of free running mode you can also drive the ADC via a timer if that suits your needs better.
Edit
If you want to discard multiple conversions following the channel change:
ISR(ADC_vect) {
static uint8_t conversionsToDiscard = 3 ; // static initialized Once only.
if ( conversionsToDiscard > 0 ) conversionsToDiscard-- ;
else {
// Read the ADC value
x[currentChannel] = ADCH;
// Update to the next channel
currentChannel = (currentChannel + 1) % 2;
ADMUX = (ADMUX & 0xF0) | currentChannel;
conversionsToDiscard = 3 ;
}
In free running mode, the next conversion is automatically started, the conversion is complete, the value is saved in the <ADCH, ADCL> registers and then the next conversion begins.
Before I read the registers' values, I check that the current conversion is really omplete by monitoring the ADIF-bit. Without doing so, one risks of reading dubious values from ADC Registers! It is because while asynchronous (random) reading is asserted on ADC registers, the registers might be in the proces of being updated.
A couple of questions, I was thinking that I'd just do an if(ADCSRA & (1 << ADIF)) then if true then skip otherwise do a reading.
Interestingly, copilot suggests a tight while loop which to me seems to risk a runaway while(forever)? Also it hints at clearing that flag. Is that necessary?
//Inside the ISR
// Wait for ADC conversion to complete
while (!(ADCSRA & (1 << ADIF))) {
// Do nothing, just wait
}
// Clear ADIF by writing a 1 to it (this is how you clear the flag)
ADCSRA |= (1 << ADIF); //is this truly needed???
// Read the ADC value
uint8_t adcValue = ADCH; // Since ADLAR is set, we only need to read ADCH
...
}
Reading this article ( https://www.avrfreaks.net/s/topic/a5C3l000000UFImEAO/t072614 ) implies that the interrupt only fires when reading but I think what happened in my bloated interrupt routine is that i made the interrupt late. I think returning from the interrupt if ADIF isn't ready is probably the safest and efficient as long as a sluggish interrupt is fairly rare.
Looking at my original bloated code, I now see further improvement is to offload (queue) the work to the main loop as much as possible.
I do like the summing of 8 readings in a row to act as noise control so I think I'll keep that Light Dependent Resistors aren't considered noiseless.
I marked my variables as volatile hoping that would be sufficient but your mention of noInterrupts() reminds me that volatile alone isn't sufficient especially for wider variables!!
The noInterrupts() is primarily used in the main program (outside of ISRs) to protect critical sections of code that should not be interrupted. For example, when updating shared variables that are also accessed within ISRs, you would typically use noInterrupts() before the critical section and interrupts() after it.
Maybe say a bit more about your project and which microcontroller you are using. You've mentioned Light Dependent Resistors and a timing constraint of 3.4ms. There may be other or better options. For example the attiny1614 ( TinyAVR 1-series) has two ADCs and these are generally better featured and more flexible than the ones of the earlier AVR series which you appear to be using.
Almost all users of this Forum own Arduino UNO (ATmega328P) as a Learning System which can be easily programmed using the Arduino IDE. If any other AVR MCU are urged to use, the OP has to know many other things to include the ATtiny1614 in the Arduino IDE before programming it.
I'm using the Mega one. I thought that was one of the newer ones.
Now that I have ISR logic working, I believe I can slow down the sampling rate now. So having two ADC units isn't needed. I round robin between my sensors.
Even a new Arduino Mega will use a chip with the old AVR architecture. Chips such as the ATmega4809 (used on say the Nano Every) or the ATtiny1614, which I previously mentioned, use a newer architecture. You'll see TinyAVR and MegaAVR mentioned. I suppose the first thing to point out is that you need different programmer using UPDI but these are simple to build.
Anyway, if you have sorted out the ADC issues (BTW your timing constraints in the low milliseconds did not seem to be pushing the device to its limits) then you probably don't need to consider something completely new.
This one would save a LD or STO opcodes but at the cost of an extra jump. (I'm done a lot of z80 asm some 8086 too but no AVR ASM).