 |
 |
| View previous topic :: View next topic |
| Author |
Message |
Sid2286
Joined: 12 Aug 2010
Posts: 119
|
| MCP3201 problem |
 Posted: Thu Dec 05, 2013 3:42 am |
 |
|
I'm trying to write a SPI code to read MCP3201 data, but I'm not sure what is wrong, the code is as follows:
| Code: |
void Init_ADC()
{
output_high(ADC_CS);
}
int16 Read_ADC(void)//ADC MCP3201 single channel
{
unsigned long int AVERAGE = 0;
char count,cpy=3,k=3;
unsigned int16 ADC_RDG = 0;//clear
while(cpy)
{
output_low(ADC_CS); //start ADC
delay_ms(1);
output_high(ADC_CLK); //1st clk
delay_ms(1);
output_low(ADC_CLK);
delay_ms(1);
output_high(ADC_CLK); //2nd clk
delay_ms(1);
output_low(ADC_CLK);
delay_ms(1);
output_high(ADC_CLK); //3rd clk
delay_ms(1);
output_low(ADC_CLK);
delay_ms(1);
for(count=0;count<11;count++)
{
output_high(ADC_CLK); //on L-H pulse data is on pin Dout
if(ADC_OUT == 1)
ADC_RDG += 1;
ADC_RDG <<= 1;
delay_ms(1);
output_low(ADC_CLK);
delay_ms(1);
}
output_high(ADC_CS); //stop ADC
ADC_RDG &= 0x0FFF; //data is max 12bit
AVERAGE += ADC_RDG;
cpy--;
}
ADC_RDG = AVERAGE/k;
return(ADC_RDG);
} |
pl. let me know what is wrong.
Sid |
|
 |
Ttelmah
Joined: 11 Mar 2010
Posts: 19504
|
|
| Posted: Thu Dec 05, 2013 5:17 am |
|
|
First, your speed. 1mSec between each pulse edge. 500Hz effective clock rate. The data sheet says:
"Because the sample cap will eventually lose charge, effective clock rates below 10 kHz can affect linearity performance,
especially at elevated temperatures. See Section 6.2 for more information"
500Hz is so low, that it may well not work at all!...
You need to clock a lot faster. 1uSec, rather than 1mSec...
Assuming ADC_OUT, is #defined as a pin number. To access this, you need to perform the input instruction. Probably main problem...
Then tidy your thinking:
| Code: |
#inline //for speed
void pulse(void)
{
delay_us(1);
output_low(ADC_CLOCK);
delay_us(1);
output_high(ADC_CLOCK)
}
int16 Read_ADC(void)//ADC MCP3201 single channel
{
unsigned int16 Average = 0;
char count, cpy=3;
unsigned int16 Adc_Rdg;
//C standard, is to reserve all capitals for 'defines' etc..
while(cpy)
{
Adc_Rdg=0; //ensure starts each loop clear
count=0; //count needs to initialise here for this version
output_low(ADC_CS); //start ADC
pulse();
pulse();
pulse(); //three clocks
do
{
Adc_Rdg*=2; //compiler automatically optimises this to rotation
pulse();
Adc_Rdg != input(ADC_OUT);
}
while (++count<12); //12 bits
output_high(ADC_CS); //stop ADC
Average += Adc_rdg;
cpy--;
}
//Why fiddle around copying the number twice?.
return(Average/3);
}
|
Now, realise you are rotating the value _after_ each bit is read. Problem then is that the last bit will also be rotated. The rotation has to occur before the next read (means one extra at the start, but not worth the overhead of not doing it...).
As one further added comment, why use three samples, not four?.
/4, will be done by rotation, and will be vastly quicker than /3. Just worth considering....
Best Wishes |
|
|
Sid2286
Joined: 12 Aug 2010
Posts: 119
|
|
| Posted: Thu Dec 05, 2013 11:49 pm |
|
|
I did not understand what you did here.,
| Code: |
do
{
Adc_Rdg*=2; //compiler automatically optimises this to rotation
pulse();
Adc_Rdg != input(ADC_OUT);
}
|
1. How do I know *=2 automatically optimizes to rotation.
2. Not sure what is happening here Adc_Rdg != input(ADC_OUT);
3. Will the follwing code work
| Code: |
ADC_RDG <<= 1;
if(input(ADC_OUT) == 1)
ADC_RDG += 1;
|
4. Also most of the codes have do while instead of for or while., is there a specific reason for that??
Thanks
Sid |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19504
|
|
| Posted: Fri Dec 06, 2013 2:27 am |
|
|
2) Teach me to read what I have typed. Should be |=, not != The former is bitwise or the second not.....
The input function returns only a 0, or 1. Val or 0 does nothing. Val or 1, sets the bottom bit in Val. Just what is wanted.
There is also semi-colon missing on the last line of pulse.
1) Read the assembler. It is defined in C itself, that for a LSB to right chip, a rotation by one, is equivalent to a multiplication by 2. For 90% of compilers and chips this is optimised to basically the same operation (it is for CCS), but it is fractionally better 'defined'. Key is that << in C, does _not_ define what happens when a value shifts out the top, or what happens with a signed value. *2 does. A matter of taste really, especially given in this case, no bit should get this far, and the value is not signed. Generates exactly the same code in this case.
3) Yes. However it takes more instructions.
| Code: |
.................... if(input(ADC_OUT) == 1)
*
004D: BSF 03.5
004E: BSF 06.2
004F: BCF 03.5
0050: BTFSS 06.2
0051: GOTO 056
.................... ADC_RDG += 1;
0052: MOVLW 01
0053: ADDWF 27,F
0054: BTFSC 03.0
0055: INCF 28,F
//versus
.................... Adc_Rdg |= input(ADC_OUT);
*
004D: BSF 03.5
004E: BSF 06.2
004F: MOVLW 00
0050: BCF 03.5
0051: BTFSC 06.2
0052: MOVLW 01
0053: IORWF 27,F
|
Since it knows Adc_Rdg is a 16bit value, 'adding one', might carry, so it performs 16bit maths. With the 'or', it knows that it can't carry and just does the single instruction. Two extra instructions..... 
4) They are different. "While (test) code", tests at the start of the loop before executing. "Do code while", tests at the end of the loop after executing once. Has the visual advantage here that the test value 'becomes' the number of bits involved (12), and it doesn't waste a test before starting. You should find both equally commonly used, depending on circumstances. If the code never wants to execute if the condition is false at the start, then while (test) is the way to go. If you want to ensure the code always executes at least once, then do while is the way to go.
Comment still applies about your variable names.
It is a 'good practice' standard in C, to reserve all capitals for #defines and similar substitutions. It means that things like 'PIN_B3' stand out as being defines, not variables. As code gets longer, you will find that little 'tricks' like this become more important. Same applies about using things like 'long'. Perfectly good, _but_ means different things on different chips/compilers. 'int16' tells you exactly what you are dealing with (actually better to include "stdint.h", and then use 'uint16_t', and similar names, which say now long the variable is, and that it is unsigned). The types defined in this even work if you switch to a PC!....
Then similar things like calling variables that contain pointers xxx_ptr etc..
They are all things that when you come back to a piece of code in a couple of months time, give 'extra clues' to help remember what you did, and why.
As one other comment change the function name to something like 'Read3201', since ReadADC, is the internal function to access the internal ADC in the PIC.
Best Wishes |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
