Combining ADC Result Over 2 Registers Into 1 Variable

[Lance]

I'm writing some functions to use the ADC module on a PIC16F917. It's an 8 bit device, and the ADC is 10 bits. So the result is split across two registers, 8 bits in one and two in the other. I want my read function to return one 10 bit value (I'm thinking of using a short integer but the compiler keeps complaining about that). My first idea was to create an addressable bitfield and tie it to the return variable using a union, however I would have to manually address each of the bits in the registers. As it is I can only directly go to the registers as a whole.

Is there a clean way for me to combine these two into a single 16 bit value (short integer)? Right now I'm going to try putting each register into a variable of the same size as my result, shifting the high register over the correct amount, and XORing them together.

Also, are there any tips or tricks anyone has for using the ADC module? Thanks.

[scrat]

Which compiler/library are you using? The Microchip C16 ones?

[Lance]

No, I'm using the Hi-Tech C compiler.

[Psi]

I'm writing some functions to use the ADC module on a PIC16F917. It's an 8 bit device, and the ADC is 10 bits. So the result is split across two registers, 8 bits in one and two in the other. I want my read function to return one 10 bit value (I'm thinking of using a short integer but the compiler keeps complaining about that).

Check if your environment already has a way to access the adc data as 16bits. I dunno about PICs, but with avrdude you can use ADCL for low bits, ADCH for high or ADCW for all 16 bits together. There may be something like that already available to you.

[Lance]

I ran a search through the manual for the term ADC but didn't find anything other than a brief assembly code example. That would take all the fun out of this anyway. 

I'm gonna try my idea of XORing the two registers after shifting one of them over. It makes sense in theory.

If I say had this in my upper and lower registers:

Code: [Select]

00000011
11111111
My plan is to stick them into a short unsigned int like so:

Code: [Select]

0000000000000011
0000000011111111

Shift the upper result register over:

Code: [Select]

0000001100000000
0000000011111111

And then XOR them:

Code: [Select]

0000001111111111

Seems solid to me.

[Psi]

ah, so you're trying to do it in asm?

[Lance]

No, I'm doing it in C. The only mention of the string "ADC" in the manual PDF was around some assembly examples.

[Psi]

(where myvariable is 16bit)

myvariable = ADCL;
myvariable = myvariable | (ADCH << 8 );    // or, if you're paranoid...   myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

[Lance]

(where myvariable is 16bit)

myvariable = ADCL;
myvariable = myvariable | (ADCH << 8 );    // or, if you're paranoid...   myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

Is your value set to be left or right justified, and how many bits is the ADC? It looks like a right justified value to me. Would I be correct in saying that an OR takes fewer operations than an XOR? Using an OR in my situation would be fine.

This is what I came up with:

Code: [Select]

unsigned short int ADCREGU=0; //Holds upper ADRESH value
unsigned short int ADCREGL=0; //Holes lower ADRESL value
unsigned short int ADCRES=0; //Final Result

ADCREGU=ADRESH;
ADCREGL=ADRESL;

ADCREGU=ADCREGU << 8;
ADCRES=ADCREGU^ADCREGL;

return(ADCRES);

I don't understand what the purpose of this is:

Code: [Select]

myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

[scrat]

Try searching for "ADC" through the included header files (especially if there's one .h file with "adc" in its name), you'll find #define constants, structures and variables defined there.
I bet there's one referring to the entire 16-bit ADC data, made up by the high and low byte.

[Psi]

Is your value set to be left or right justified

right, which is normal afaik

how many bits is the ADC?

10bit, same as yours

Would I be correct in saying that an OR takes fewer operations than an XOR? Using an OR in my situation would be fine.

I dunno, ya would need to see how many asm instructions both ways translate into.
That's just the way i do it.
I doubt it will matter, ya shouldn't be that tight on instructions that you need to worry about it.
If you wanted to investigate you could check if there's a single XOR asm instruction for the pic cpu core your using.
My guess is that OR is going to be quicker.

This is what I came up with:

Code: [Select]

unsigned short int ADCREGU=0; //Holds upper ADRESH value
unsigned short int ADCREGL=0; //Holes lower ADRESL value
unsigned short int ADCRES=0; //Final Result

ADCREGU=ADRESH;
ADCREGL=ADRESL;

ADCREGU=ADCREGU << 8;
ADCRES=ADCREGU^ADCREGL;

return(ADCRES);


That looks like it should work fine but

- You might want to check the PIC datasheet as i'm not sure if this applies to PICs but... With AVRs you should always read the Low ADC byte first because it buffers the result. So reading the low byte locks the high byte from changing until you read it. If you read the High byte first there is a chance the low byte may get updated with new data before you can read it. In which case you end up with the high byte from sample 1 and the low byte from sample 2.  (but it may not apply to PICs)

- Again, i'm going on my AVR experience but.. you don't really need the ADCREGL variable, you can use ADRESL directly and save the memory.

- You don't strictly need to zero those 3 variables, since they all get assigned to, but it's a good habit to get into. If you were tight on space you could safely remove the initialization to zero for those 3 lines.

I don't understand what the purpose of this is:

Code: [Select]

myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

Some old cpus can do unusual or undefined things when you do bitwise shift operations. If you are paranoid about checking that the "seemingly" new bits that appear in a shift operation are zeros you can use a mask.
That is what those two AND operations do.
  & 255 masks off all data but the bits your after 0000 0000 1111 1111 = 255
  & 768 does the same 0000 0011 0000 0000 = 768

This isn't necessary if you know the shift behavior of the cpu your working on, but some people are paranoid

[ziq8tsi]

It looks like a right justified value to me.

Right justified is the better choice in this case, because then the low order bits are already in the correct place.  With a left justified result, both halves would need to be shifted.

Would I be correct in saying that an OR takes fewer operations than an XOR?

IOR and XOR are both available as native instructions, and all arithmetic instructions are the same speed on a PIC.  In fact, I doubt you could find any architecture where there is a speed difference.

You could also use addition, but that can be slightly slower than OR on 8bit platforms due to the additional complication of handling a (impossible in this case) carry between the bytes.

I think I would argue that inclusive OR is the more common idiom, when dealing with fields that do not overlap.

I don't understand what the purpose of this is:

Code: [Select]

myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

The &768 would only be necessary if the six unused bits in ADRESH might contain junk values.  But the datasheet guarantees that the unimplemented bits read as zeros, so that mask cannot have any effect.

The &255 could only have an effect if ADRESL was declared as a signed char, and had therefore been sign-extended when promoting to 16bit.  But ADRESL is actually declared as unsigned char, so that mask cannot have any effect.  And the correct solution in any case would have been an explicit cast to (unsigned char), since masking back afterwards is not guaranteed to work on all arithmetic systems.

[Psi]

And the correct solution in any case would have been an explicit cast to (unsigned char), since masking back afterwards is not guaranteed to work on all arithmetic systems.

interesting

[Lance]

This is what I came up with:

Code: [Select]

unsigned short int ADCREGU=0; //Holds upper ADRESH value
unsigned short int ADCREGL=0; //Holes lower ADRESL value
unsigned short int ADCRES=0; //Final Result

ADCREGU=ADRESH;
ADCREGL=ADRESL;

ADCREGU=ADCREGU << 8;
ADCRES=ADCREGU^ADCREGL;

return(ADCRES);


That looks like it should work fine but

- You might want to check the PIC datasheet as i'm not sure if this applies to PICs but... With AVRs you should always read the Low ADC byte first because it buffers the result. So reading the low byte locks the high byte from changing until you read it. If you read the High byte first there is a chance the low byte may get updated with new data before you can read it. In which case you end up with the high byte from sample 1 and the low byte from sample 2.  (but it may not apply to PICs)

- Again, i'm going on my AVR experience but.. you don't really need the ADCREGL variable, you can use ADRESL directly and save the memory.

- You don't strictly need to zero those 3 variables, since they all get assigned to, but it's a good habit to get into. If you were tight on space you could safely remove the initialization to zero for those 3 lines.

1. As far as I know the result only changes when you actually tell the module to run. The way I wrote my functions if the ADC is in the middle of a computation it will not allow the read to happen.

2. I could, I'm not super tight on memory. However I do want to try and make the code as clean as possible.

3. I always initialize variable where needed. It guarantees silly things won't happen from what I'm told.

I don't understand what the purpose of this is:

Code: [Select]

myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

Some old cpus can do unusual or undefined things when you do bitwise shift operations. If you are paranoid about checking that the "seemingly" new bits that appear in a shift operation are zeros you can use a mask.
That is what those two AND operations do.
  & 255 masks off all data but the bits your after 0000 0000 1111 1111 = 255
  & 768 does the same 0000 0011 0000 0000 = 768

This isn't necessary if you know the shift behavior of the cpu your working on, but some people are paranoid

I see. I've heard of masks, but I don't actually know what they are, nor have I ever used one before. To wikipedia!

I don't understand what the purpose of this is:

Code: [Select]

myvariable = (myvariable & 255) | ( (ADCH << 8 ) & 768 );

The &768 would only be necessary if the six unused bits in ADRESH might contain junk values.  But the datasheet guarantees that the unimplemented bits read as zeros, so that mask cannot have any effect.

The &255 could only have an effect if ADRESL was declared as a signed char, and had therefore been sign-extended when promoting to 16bit.  But ADRESL is actually declared as unsigned char, so that mask cannot have any effect.  And the correct solution in any case would have been an explicit cast to (unsigned char), since masking back afterwards is not guaranteed to work on all arithmetic systems.

Good to know, thanks!

[DrGeoff]

Code: [Select]

unsigned short n;
unsigned char adl;
unsigned char adh;

n = (unsigned short )(adl) | (((unsigned short )(adh) << 8) & 0xff00);

// if it's a signed value, with the sign in the 10th bit you will need to move the
// sign bit up to bit 15 and remove it from bit 9.
n = (n | ((n << 6) & 0x8000)) & 0xfdff;



[johnmx]

The simplest, cleanest and fastest way:

Code: [Select]

union IntOr2Char
{
unsigned int ir;
unsigned char cr[2];
};

union IntOr2Char ADCResult;

ADCResult.cr[1] = ADRESH;
ADCResult.cr[0] = ADRESL;

ADC result can be accessed like this: ADC_Mean += ADCResult.ir;

[RayJones]

The simplest, cleanest and fastest way:

Code: [Select]

union IntOr2Char
{
unsigned int ir;
unsigned char cr[2];
};

union IntOr2Char ADCResult;

ADCResult.cr[1] = ADRESH;
ADCResult.cr[0] = ADRESL;

ADC result can be accessed like this: ADC_Mean += ADCResult.ir;

Finally commonsense prevails.
You must remember at the end, a 16 bit value is exactly as johnmx has shown, 2 sequential bytes in memory.
When you have a 8 bit bus, you are simply wasting time wanking about with 16 bit shorts bit shifting and oring to simply read the data from the ADC.

Do as John says and use a union and the ADC read opration will take place very efficiently as per the sample code.
Yes it is micro specific, but so is the architecture of the PIC's ADC.

[RayJones]

a variation I am fond of also uses a union

Code: [Select]

union ADCshort {
  unsigned short word;
  struct bytes {
     unsigned char lsb;
     unsigned char msb;
  }
}

union ADCshort ADCval;
short testo;

ADCval.bytes.msb = ADRESH;    // read and store ADC's msb
ADCval.bytes.lsb = ADRESL;    // read and store ADC's lsb

testo = ADCval.word;          // utilise 16bit result


The proof is looking at the assembler listing that the compiler generates.
You will see that the structure de-referencing boils down to simple reads from one address to another address.
All the tom foolery of calculating the addresses is pre-determined by the compiler.

(It would be good if fixed font size appeared in the forum's editor when punching in "code" blocks - hence the edits. :-/)

[Mechatrommer]

i concur. if you know how data is represented in memory location, there should be not much trouble. union is a great tool for bit/byte arrangement.

[ziq8tsi]

You must remember at the end, a 16 bit value is exactly as johnmx has shown, 2 sequential bytes in memory.

But you do need to know what order they are stored.  There is no compelling reason to prefer one endianness or another on the pic14 architecture, so it is really up to the compiler to choose.

When you have a 8 bit bus, you are simply wasting time wanking about with 16 bit shorts bit shifting and oring to simply read the data from the ADC.

When you have an 8bit ALU, there are no instructions that can be used to shift a 16bit value by more than one bit at a time.  So you can pretty much guarantee that even a poorly optimizing compiler will have to eliminate the shift.  Not to mention that it is a common idiom.

The proof is looking at the assembler listing that the compiler generates.

Okay.  First we need to insert your two missing semicolons and complete the declaration of the member "bytes" so that the code will compile.

Hi-tech C lite produces twelve instructions for your code (plus six for the extra assignment) compared to fifteen for the shift and or.

[RayJones]

Okay.  First we need to insert your two missing semicolons and complete the declaration of the member "bytes" so that the code will compile.

Hi-tech C lite produces twelve instructions for your code (plus six for the extra assignment) compared to fifteen for the shift and or.

[/quote]

Oh Jesus, nail me to the wall, this was written straight into the forum's text editor.
Do you code 100% correct every time without the compiler reminding you of your errors?
I spend most of my life coding in C++ these days so probably got the syntax of the struct wrong, I'm sorry you had to work it out a bit

The extra assignment was purely used as an example of what to do beyond the ADC reading operations and is irrelevant, the actual loading of the ADC is the part that matters, so 12 over 15 cycles is still better, what's your point?

The endianess of a compiler will be known, just as you need to read the PIC's data sheet to know how to read the ADC.
We are NOT talking about portable code here, it is very much platform dependent.

[Mechatrommer]

you should gave him the "pseudo-code" Ray.

[Lance]

I actually got this working a while ago...I'm trying to figure out the LCD driver right now.