Wrong interrupt handling? ADC conversion correct but display via USART not always correct.

Go To Last Post
22 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi everyone,

I'm starting to "play" with ADC. Before setting out my problem, I make some premises. I made a rudimentary programming board for ATMEGA DIP 40 pins. In particular, at this moment, an ATMEGA164A is installed with an external 16Mhz crystal. The board is powered by an external 9V power supply (typical power supply for Arduino and Rasberry). The voltage is stabilized at 5V with a classic ST LM7805CV. Regarding the ADC a 10uH inductance and a 100nF capacitor to the GND were placed. I have activated PIN 39 (PA1 ADC1) as INPUT for the ADC. The input voltage for the ADC is provided by the MM103 sensor a converts barometric pressure to analogic voltage. The voltage reading and the atmospheric pressure estimate seem to be correct. Furthermore, the measurement is not subject to oscillations; the value of the ADC has no fluctuations. To read and to send values, ​​the data are converts to one of the two USARTs of which the microcontroller is provided (USART0). However, frequently the visualization on the monitor of the values ​​turn out to be dirty, as if the buffer has not been cleaned, the effect is similar to the values ​​that are linked together.

Wrong visualization on COM3

 

The strange thing is that with the same settings as the USART if I disable the ADC interrupt and stop having the analog value printed, converted to digital and start sending any text to be printed via USART the text is always clean and exactly as expected. I may be doing something wrong at the level of synchronization between the ADC interrupt is the one that is still being transmitted, and the transmitted data is overlapped. What am I doing wrong? I would say that I can exclude hardware connection problems, soldering, etc. You have some advice for me. I'm just starting to approach the ADC, and it's likely that I'm making some mistakes. Thank you very much.

Below is the code that I run at the ATMEGA164A.

 

/*
 * LedMyBoardAtmel164A-PU.c
 *
 * Created: 12/07/2019 00:01:14
 * Author : loren
 */
#ifndef F_CPU
#define F_CPU 16000000L
#endif

#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <string.h>
#include <stdlib.h>

#define BAUD 115200
#define BRC ((F_CPU/8/BAUD) -1 )
#define TX_BUFFER_SIZE  128

char serialBuffer[TX_BUFFER_SIZE];
uint8_t serialReadPos = 0;
uint8_t serialWritePos = 0;
double result = 0;
char rBuf[4];
char aBuf[7];
int a = 0;

void appendSerial(char c);
void serialWrite(char  c[]);

void ADC_init(void){
	ADMUX = (1 << REFS0) | (1 << MUX0);
	ADCSRA = (1 << ADEN) | (1 << ADIF) | (1 << ADSC) | (1 << ADATE) | (1 << ADIE) | (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0);

}
int main(void)
{

	DDRD |= (1 << PD7);
	/* Inzio l'inizializzazione della USART rifattorizzare con una libreria (.h)
	 * UBRRn registro per impostare il baud rate
	*/
	UBRR0H = (BRC >> 8);
	UBRR0L = BRC;
	UCSR0A = (1 << U2X0);
	UCSR0B = (1 << TXEN0)	| (1 << TXCIE0); /* enable serial transmission and enable interrupt generation when transmission is complete*/
	UCSR0C = (1 << UCSZ01)	| (1 << UCSZ00); /* Set 8bit */

	ADC_init();
	sei();

    /* Replace with your application code */
    while (1)
    {
		PORTD ^= (1 << PD7);
		result = (11500/47.0*(50.0*a/1024.0)/10.0) + 20.0; //20.0 delta for calibration sensor
		dtostre(result, rBuf, '2', '-');
		itoa(a, aBuf, 10);

		serialWrite(rBuf);
		serialWrite("\t");
		serialWrite(aBuf);
		serialWrite("\n\r");
		_delay_ms(1000);
    }

	return 0;
}

void appendSerial(char c)
{
	serialBuffer[serialWritePos] = c;
	serialWritePos++;

	if(serialWritePos >= TX_BUFFER_SIZE)
	{
		serialWritePos = 0;
	}
}

void serialWrite(char c[])
{
	for(uint8_t i = 0; i < strlen(c); i++)
	{
		appendSerial(c[i]);
	}

	if(UCSR0A & (1 << UDRE0))
	{
		UDR0 = 0;
	}
}

ISR(USART0_TX_vect)
{
	if(serialReadPos != serialWritePos)
	{
		UDR0 = serialBuffer[serialReadPos];
		serialReadPos++;

		if(serialReadPos >= TX_BUFFER_SIZE)
		{
			serialReadPos++;
		}
	}
}

ISR(ADC_vect){
	a = ADC;
	ADCSRA = (1 << ADSC);

}

Thanks a lot.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

The way your code is written using interrupts is causing you problems. My recommendation- don’t use interrupts for the adc. Else, declare int a; as volatile int a; and read my tutorial ‘the traps when using interrupts’ in the tutorial section.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

codabat wrote:

		dtostre(result, rBuf, '2', '-');

First, the precision should be 2 not '2'

Second, the '-' should be something like DTOSTR_ALWAYS_SIGN | DTOSTR_PLUS_SIGN | DTOSTR_UPPERCASE

or whatver options you actually want.

Third, your rbuf is not big enough.

codabat wrote:

char rBuf[4];

For 2 digits precision, you would need minimum +n.nnE+nn = 9, plus 1 for null termination,so 10 minimum.

 

 

EDIT:

Fourth

		if(serialReadPos >= TX_BUFFER_SIZE)
		{
			serialReadPos++; <== are you sure?
		}

 

 

 

Last Edited: Tue. Jul 23, 2019 - 08:26 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi @kartman,
thank you for your quick response. I go just now to read your tutorial.

Thank you so much for your kind.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi MrKendo,

you have right! For the macro  DTOSTR_ALWAYS_SIGN | DTOSTR_PLUS_SIGN | DTOSTR_UPPERCASE  I don't found the definitions.

Thank you for your attention and observation, I go to correct the wrong definitions.

 

Thanks a lot.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Setting 'a' as volatile is needed as Kartman says (so the compiler knows a read of this var is needed each time it is used), but it will not help with atomic access.

 

That 'a' takes 2 bytes, and every read of 'a' in main code can be interrupted by the adc, so you could be reading the first byte of 'a', interrupt fires, second byte of 'a' changes in the isr, back to main where the second byte is now read- the two bytes read for 'a' in the main code may not belong together. You read 'a' in two places, so the opportunity to get a corrupt read on 'a' is twice every loop.

 

make a temp copy of 'a', with interrupts off so the adc interrupt cannot change it while you read 'a'-

cli();

int a_tmp = a;

sei();

now use 'a_tmp' instead of 'a'

 

 

But, that is not the cause of your problem- rBuf is 4 bytes, but you are using much more than 4 bytes for 'dtostre', which is clobbering other bss vars that are corrupting your serial buffer indexing (I'm guessing, but take a look in the map file to see what is beyond the rBuf var).

 

 

edit- I'm too late, doesn't look like I can delete this post.

Last Edited: Tue. Jul 23, 2019 - 08:30 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

curtvm wrote:

Setting 'a' as volatile is needed as Kartman says (so the compiler knows a read of this var is needed each time it is used), but it will not help with atomic access.

 

That 'a' takes 2 bytes, and every read of 'a' in main code can be interrupted by the adc, so you could be reading the first byte of 'a', interrupt fires, second byte of 'a' changes in the isr, back to main where the second byte is now read- the two bytes read for 'a' in the main code may not belong together. You read 'a' in two places, so the opportunity to get a corrupt read on 'a' is twice every loop.

 

make a temp copy of 'a', with interrupts off so the adc interrupt cannot change it while you read 'a'-

cli();

int a_tmp = a;

sei();

now use 'a_tmp' instead of 'a'

 

 

But, that is not the cause of your problem- rBuf is 4 bytes, but you are using much more than 4 bytes for 'dtostre', which is clobbering other bss vars that are corrupting your serial buffer indexing (I'm guessing, but take a look in the map file to see what is beyond the rBuf var).

 

 

edit- I'm too late, doesn't look like I can delete this post.

 

Hi @curtvm,

thanks a lot, for your important explanations. It's really important your observation.

Yes, as he also said  @MrKendo the definition of rBuf is totally wrong! I have correct the definition! In addition my statment it's wrong. 

if(serialReadPos >= TX_BUFFER_SIZE)
		{
			serialReadPos++; <== are you sure?
		}

serialReadPos must be serialReadPos = 0;

 

Thank you very much, everyone, I will deepen the very important observation you made to me about reading the variable when an interrupt was triggered and I will read with great pleasure the tutorial that @KartMan showed me.
I rejoice you all very much.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

codabat wrote:
For the macro  DTOSTR_ALWAYS_SIGN | DTOSTR_PLUS_SIGN | DTOSTR_UPPERCASE  I don't found the definitions.
The user manual perhaps?

 

https://www.nongnu.org/avr-libc/user-manual/group__avr__stdlib.html#ga6c140bdd3b9bd740a1490137317caa44

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi clawson, I found them...

I also found it here:

https://www.microchip.com/webdoc...

Very useful, this microchip.com webdoc.

Thank you very much, this group is excellent, and its participants are all excellent and helpful.

I thank you again for your contribution. Anyway, thanks to the advice of all of you now my little firmware works perfectly.
Bye

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi everyone,

I apologize in advance. But after last night's constructive posts, there is still something wrong. Obviously, it's the fault of my inexperience and still little knowledge.

I believe I haven't set the interrupts correctly. Because if I put a counter that is increased in the ADC ISR I would expect to see it grow speedy. Instead, I keep getting the value 1, as if the interrupt was invoked only once.

 

Yet at the end of the ISR function I write again ADCSRA = (1 << ADSC);

Below are the ADC configurations:
 

void ADC_init (void) { 

ADMUX = (1 << REFS0) | (1 << MUX0); 

ADCSRA = (1 << ADEN) | (1 << ADIF) | (1 << ADSC) | (1 << ADATE) | (1 << ADIE) | (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0);

}

while in the main (void), immediately after the serial configuration, I call in sequence:

ADC_init ();

six();

 

Below the ISR function

ISR(ADC_vect){
	a = ADC;
	counter++;
	cCounter = counter;
	ADCSRA = (1 << ADSC);
	
}

 

What am I doing wrong?

I apologize and thank you for your help.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

codabat wrote:

ISR(ADC_vect){
	a = ADC;
	counter++;
	cCounter = counter;
	ADCSRA = (1 << ADSC); <=== WHY

}

When you do ADCSRA = (1 << ADSC) you have just cleared all the other bits in ADCSRA. That can't be right.

I think you're trying to use ADC in free running mode?

I haven't used this myself but someone will know.

Probably just leave ADCSRA alone.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The variables counter and cCounter should be specified as volatile.  Are they?

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

MrKendo wrote:

 

When you do ADCSRA = (1 << ADSC) you have just cleared all the other bits in ADCSRA. That can't be right.

I think you're trying to use ADC in free running mode?

I haven't used this myself but someone will know.

Probably just leave ADCSRA alone.

 

 

Hi MrKendo,

I had understood that (1 << ADSC) started the conversion. I wish he worked in free running. But this should be guaranteed by having set the ADATE bit. I have removed (1 << ADSC) from ISR but I get senseless values. Furthermore, it seems that the sampled value of the sensor (ADC) is always the same (813).

Below the screenshot with the estimated value of the pressure, the value of the ADC and the value of the counter.

USART OUTPUT ISR

So (1 << ADSC) is used when not working in free-running? I'm a little confused. I'm sorry.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Chuck99 wrote:

The variables counter and cCounter should be specified as volatile.  Are they?

 

 

Hi Chuck99,

counter and cCounter are soecified as global volatile.

 

I'm sorry, repeat the code

/*
 * LedMyBoardAtmel164A-PU.c
 *
 * Created: 12/07/2019 00:01:14
 * 
 */ 
#ifndef F_CPU
#define F_CPU 16000000L
#endif

#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <string.h>
#include <stdlib.h>

#define BAUD 115200
#define BRC ((F_CPU/8/BAUD) -1 )
#define TX_BUFFER_SIZE  128

char serialBuffer[TX_BUFFER_SIZE];
uint8_t serialReadPos = 0;
uint8_t serialWritePos = 0;
double result = 0;
char rBuf[11];
char aBuf[7];
char cBuf[10];
volatile int counter = 0;

volatile int a = 0;
volatile int cCounter = 0;
void appendSerial(char c);
void serialWrite(char  c[]);



void ADC_init(void){
	ADMUX |= (1 << REFS0) | (1 << MUX0);
	ADCSRA |= (1 << ADEN) | (1 << ADIF)  | (1 << ADATE) | (1 << ADIE) | (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0);
	ADCSRA |= (1 << ADSC);
}
int main(void)
{
	
	DDRD |= (1 << PD7);
	/* Inzio l'inizializzazione della USART rifattorizzare con una libreria (.h) 
	 * UBRRn registro per impostare il baud rate
	*/
	UBRR0H = (BRC >> 8);
	UBRR0L = BRC;
	UCSR0A = (1 << U2X0);
	UCSR0B = (1 << TXEN0)	| (1 << TXCIE0); /* enable serial transmission and enable interrupt generation when transmission is complete*/
	UCSR0C = (1 << UCSZ01)	| (1 << UCSZ00); /* Set 8bit */
	
	ADC_init();
	sei();
	
    /* Replace with your application code */
    while (1) 
    {
		PORTD ^= (1 << PD7);
		result = (11500/47.0*(49.6*a/1024.0)/10.0) + 29.555; //30.625; //20.0 delta for calibration sensor
		dtostre(result, rBuf, 4, DTOSTR_PLUS_SIGN);
		itoa(a, aBuf, 10);
		itoa(cCounter, cBuf, 10);
		serialWrite(rBuf);
		serialWrite("\t");
		serialWrite(aBuf);
		serialWrite("\t");
		serialWrite(cBuf);
		serialWrite("\n\r");
		_delay_ms(1000);
    }
	
	return 0;
}

void appendSerial(char c)
{
	serialBuffer[serialWritePos] = c;
	serialWritePos++;
	
	if(serialWritePos >= TX_BUFFER_SIZE)
	{
		serialWritePos = 0;
	}
}

void serialWrite(char c[])
{
	for(uint8_t i = 0; i < strlen(c); i++)
	{
		appendSerial(c[i]);
	}
	
	if(UCSR0A & (1 << UDRE0))
	{
		UDR0 = 0;
	}
}

ISR(USART0_TX_vect)
{
	if(serialReadPos != serialWritePos)
	{
		UDR0 = serialBuffer[serialReadPos];
		serialReadPos++;
		
		if(serialReadPos >= TX_BUFFER_SIZE)
		{
			serialReadPos = 0;
		}
	}
}

ISR(ADC_vect){
	a = ADC;
	counter++;
	cCounter = counter;
}

Thank you so much!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

codabat wrote:

char rBuf[11];

Still too small.

+9.9310e+02 is 11 chars, but you need space for a 0 byte on the end (null terminated string). So make it at least 12.

Your counter values seem plausible, if it's doing about 10000 conversions per second.

It will still be converting while you're sitting in your delay_ms(1000).

The counter is type int so it's signed 16 bit, so over 32767 it goes negative.

You could make it unisgned, uint16_t.

(You still have problem of non atomic access).

I don't see why you really need the free running adc.

Why not just take a single reading each time round the loop. Then you don't need the adc interrupt. You will need to initilaise ADCSRA appropriately (don't enable auto-trigger and don't enable interrupt), then take a single reading by

    /* start conversion */
    ADCSRA |= (1 << ADSC);
    /* wait for completion */
    while (ADCSRA & (1 << ADSC));

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are still reading 'a' twice in your main loop where its value can change in the adc interrupt. Volatile does not help you here- that int read takes 2 instructions and can be interrupted between the 2 instructions. Now you added a read of Ccounter also, which is an int that also changes in the adc interrupt. You need to read or copy those vars in the main loop with interrupts turned off.

 

I don't know if this is causing any problems, but it will (I imagine the adc interrupt is firing quite often, so the chances of a 'corrupt' read increase quite a bit).

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As I mentioned in an earlier post, using interrupts for your ADC is really helping you. Is there a specific reason you can't simply read the adc in your main loop and not require interrupts? Doing an ADC conversion is most likely faster than actually printing out the results.

 

If you want to do a circular buffer for your uart properly, look at the Arduino core source code.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dear MrKendo,
My post and your reply are proof of a mistake due to years of programming not on microcontrollers. In my unconscious, an int has a much more extensive range. Incredible oversight LOL.
I confirm, and I agree with you that there is no reason to work in "free-running". I was experiencing its use. Especially for reading atmospheric pressure, it doesn't make much sense.
After having studied the various configurations, I will certainly pass on to implementation as you recommended.

 

Thank you very much.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi curtvm,

You're right. Actually, the complication is that I'm trying to work in "free-running".

The reason is purely to study the possibilities given by interrupts. In a real implementation, it doesn't make sense to work in free-running.
I'm running my atmega164 clocked at 16Mhz, and the ADC is configured with a pre-scaler set to 128, so it samples around 125kHz. Net of conversion times, is it correct to state that the ISR function call is 125,000 times per second?
Thank you, you are all very kind.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:

As I mentioned in an earlier post, using interrupts for your ADC is really helping you. Is there a specific reason you can't simply read the adc in your main loop and not require interrupts? Doing an ADC conversion is most likely faster than actually printing out the results.

 

Dear Kartman,

You're right. The reason is purely to study the possibilities given by interrupts. In a real implementation, it doesn't make sense to work in free-running.

I'm running my atmega164 clocked at 16Mhz, and the ADC is configured with a pre-scaler set to 128, so it samples around 125kHz. Net of conversion times, is it correct to state that the ISR function call is 125,000 times per second?

 

Kartman wrote:

If you want to do a circular buffer for your uart properly, look at the Arduino core source code.

 

Where can I find the code that indicates me?
I don't use Arduino, but for what I had seen, maybe I have to see the serial libraries?
Thank you, you are all very kind.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Why 'counter' and 'cCounter' anyway? What possible purpose does "counter" serve?

 

Oh and in "free running" mode you only ever need to set ADSC once at the very start (as you do in ADC_init()). That starts the whole process. It just keeps making conversion after conversion after that. Unlike "one shot" mode ADSC never changes - it remains set from the point you start the process. That means that if you do need to determine "new reading" it is ADIF you use (or an interrupt). As Kartman has said you don't actually need an interrupt in this at all. Either you could just keep reading ADC whenever you want to update the display (and don't care whether it is a new reading or the same value you last showed) or you could just poll the state of ADIF and only change the display each time you see it become set (after which you then reset it).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi Clawson, You're right.

There is no reason to use a free-running mode. I set the ADATA bit (free-running) to experiment.

Now I will try the modality you recommended to me, which in the case of an atmospheric pressure meter is undoubtedly more suitable, given the type of variability over time of this physical quantity.

I thank you and all those who have spoken with reflections, suggestions, and corrections that are important for me.

 

Thank you very much!