I'm writing a little function so that I can easily get ADC results from the H and L register and straight into a single result. Is the following correct:
uint16_t adc_result (void)
{
uint16_t result
result = (ADCH << 8) + ADCL;
return result;
}
Just use ADC.
while( ADCSRA & (1<<ADSC) ); // Wait until ADC conversion is complete.
result = ADC;
Alexander.
how do you mean just use ADC ? as far as i know there are no ADC libraries.......
My question is abut combining the two registers
Somewhere in the header files for every mcu there will be a statement like (iom328p.h)
#define ADC _SFR_MEM16(0x78)
#define ADCW _SFR_MEM16(0x78)
When you call ADC (16-bit), it will automatically serve you the combined content of the ADCH and ADCL registers.
Alexander.
ok, i sort of see it but lets assume I'm really dumb. I see no code there that adds two 8 bit registers together to form one 16 bit variable, just some #defines
either way is my code correct ?
either way is my code correct ?
Just use ADC. Its faster and simpler.
Your code will join the two registers into one variable yes, but it looks to be doing it in the wrong order.
It looks like ADCH would be read first, then ADCL. (It should be done the other way around)
Since there are two ADC registers and the AVR cpu can only do one 8bit operation at a time its possible that the ADCL/ADCH registers could be updated from an interrupt (or free running mode) right in the middle of you reading them in main().
To stop you getting the high bits from one sample and low bits from another (or vise versa) the AVR will hardware lock the register when it detects you reading ADCL and then unlock it when you read ADCH.
So the correct procedure so to read ADCL first, that way your subsequent ADCH read is guaranteed to be from the same sample.
ok but apart from that my code is correct ?
That code to combine the two 8 bits is behind the _SFR_MEM16() macro.
Eventually it turn out to:
#define _MMIO_WORD(mem_addr) (*(volatile uint16_t *)(mem_addr))
Which uses typecasting to leave the combining trick to the compiler.
Maybe, you will need to read the * at the ADCL/H registers regarding the read sequence.
You should OR the shifted ADCH with ADCL.
result = (ADCH << 8) |ADCL;
Alexander.
I'm writing a little function so that I can easily get ADC results from the H and L register and straight into a single result. Is the following correct:
uint16_t adc_result (void)
{
uint16_t result
result = (ADCH << 8) + ADCL;
return result;
}
Simon, there are a few issues here
1. This is a C thing. You want to do the shift on a 16 bit value. If you shift a 8 bit value by 8 you will get zero (bit fall off the left side). Hence ((uint16)adch) + adcl.
2. On avr, the h/l values of a 16 bit register must be read in a certain order. Otherwise it's not an atomic sampling of the value. Read the AVR datasheet and make sure this is what happens, even after compiler optimizations. (using ADC instead will take care of that).
3. Avr specific again. Reading the h/l value of a 16 bit register uses a global temporary register (again, AVR datasheet). If any ISR does i/o, even on different registers, it may corrupt that temp register. In this case you want to disable interrupts while doing the read. I don't think ADC does that so it's a problem even if you use ADC.
I'm writing a little function so that I can easily get ADC results from the H and L register and straight into a single result. Is the following correct:
uint16_t adc_result (void)
{
uint16_t result
result = (ADCH << 8) + ADCL;
return result;
}
Simon, there are a few issues here
1. This is a C thing. You want to do the shift on a 16 bit value. If you shift a 8 bit value by 8 you will get zero (bit fall off the left side). Hence ((uint16)adch) + adcl.
2. On avr, the h/l values of a 16 bit register must be read in a certain order. Otherwise it's not an atomic sampling of the value. Read the AVR datasheet and make sure this is what happens, even after compiler optimizations. (using ADC instead will take care of that).
3. Avr specific again. Reading the h/l value of a 16 bit register uses a global temporary register (again, AVR datasheet). If any ISR does i/o, even on different registers, it may corrupt that temp register. In this case you want to disable interrupts while doing the read. I don't think ADC does that so it's a problem even if you use ADC.
1) yes I was wondering about that I thought that my result variable being 16 bit would handle it but obviously not.
ok so how do i use ADC, i have never heard of it do i just use ADC as the variable that contains the current analog value ?
Yes use ADC or ADCW (it is the same thing) directly. ADCW has the extra W to remind you that the result is a Word (16 bit long).
Alexander.
so:
uint16_t result;
result = ADC
will transfer the full result to the 16 bit variable "result" ?
but doesn't
result |= (ADCH<<8);
mean that the ADCH can get lost unless it is treated as 16 bit ?
You don't have to add them manually. There's a 16bit access method:
ADCSRA |= (1 << ADSC); /* start conversion */
while (ADCSRA & (1 << ADSC)); /* wait until conversion is done */
my_16bit_variable = ADCW;
The "W" means "word" ;-)
so the answer to my question (bearing in mind i'm dyslexic so trying to tell me too much at once just leaves me blank
) is that I can make my 16 bit variable equal to ADCW (or ADC ?) to get my result ?
my_variable = ADCW;
or as in my function as I'm trying to write my own functions to cover everything (my version of arduino
)
uint16_t adc_result (void)
{
uint16_t result;
result = ADCW
return result;
}
so the answer to my question (bearing in mind i'm dyslexic so trying to tell me too much at once just leaves me blank ) is that I can make my 16 bit variable equal to ADCW (or ADC ?) to get my result ?
my_variable = ADCW;
Yep! If you want to get just the lower or upper 8 bit value you use ADCL or ADCH. And if you want to get both combined you'll use ADCW.
Looking at iom328p.h, ADC and ADCW are the same (see below). As for your code, using ADC or ADCW is not safe if you have ISR that are also accessing some 16 bit registers (even non related). Read the AVR datasheet about the temp i/o buffer. Look at line 34 here are reading a 16 bit register is protected against interrupts
https://github.com/zapta/power-monitors/blob/master/pmon_uno/arduino/hardware_clock.h#ifndef __ASSEMBLER__
#define ADC _SFR_MEM16(0x78)
#endif
#define ADCW _SFR_MEM16(0x78)
#define ADCL _SFR_MEM8(0x78)
#define ADCL0 0
#define ADCL1 1
#define ADCL2 2
#define ADCL3 3
#define ADCL4 4
#define ADCL5 5
#define ADCL6 6
#define ADCL7 7
#define ADCH _SFR_MEM8(0x79)
#define ADCH0 0
#define ADCH1 1
#define ADCH2 2
#define ADCH3 3
#define ADCH4 4
#define ADCH5 5
#define ADCH6 6
#define ADCH7 7
well that makes the writing of a specific function more useful them because I can disable and then re enable the interrupts.
or as in my function as I'm trying to write my own functions to cover everything (my version of arduino )
uint16_t adc_result (void)
{
uint16_t result;
result = ADCW
return result;
}
I woud add
while( ADCSRA & (1<<ADSC) ); // Wait until ADC conversion is complete..
uint16_t adc_result (void)
{
uint16_t result;
while( ADCSRA & (1<<ADSC) ); // Wait until ADC conversion is complete.
result = ADCW
return result;
}
Personally I use the function below for my projects. Single conversion mode.
uint16_t read_ADC(uint8_t ADC_channel){
int16_t adc_val;
ADMUX = (ADMUX & 0xF0) | (ADC_channel & 0x0F); //Select channel to read.
ADCSRA |= (1<<ADSC); // Start ADC conversion.
while( ADCSRA & (1<<ADSC) ); // Wait until ADC conversion is complete.
adc_val = ADCW;
return adc_val;
}
Alexander.
yes that is a good idea. i have written an ADC setup function too, i just didn't see the point of running the setup every time I want a result so have written a setup function and a ADC result read function. I'm writing functions for most things that require register interaction as it will be easier for me to go through my list of functions and choose the right one than to reread the datasheet every time and it saves me rewriting code all of the time.