PIC Period Measurement Program

Thread Starter

The_Fleertz

Joined Apr 23, 2012
8
Hey, I'm trying to write code to measure an input and determine if it is within five cents of the period of a guitar note. The input from a function generator is 110 Hz and the buttons are set up correctly. What doesn't work is the comparison if(tau>period_low & tau<period_high) which should cause an LED to blink. I noticed that the code also never enters the if statement if(capreg_new>capreg_old) either. I don't know if there is a timing error somewhere? Thanks for the help.

Rich (BB code):
//Tuner Program/\/\/\

#define  _LEGACY_HEADERS
#include <htc.h>

__CONFIG(INTIO & WDTDIS & PWRTDIS & BORDIS & BORDIS & LVPDIS & DEBUGEN & DUNPROTECT & UNPROTECT);

#define stop 0
#define forward 1
#define backward 2
//void motor_forward(void);
//void motor_backward(void);
unsigned long long get_period(void);

volatile unsigned int notes[4][6]=	{12134, 9091, 6811, 5102, 4050, 3034,				//E Standard
									13620, 9091, 6811, 5102, 4050, 3034,				//Drop D
									13620, 9091, 6811, 5102, 4545, 3405,				//DADGAD
									12857, 9631, 7216, 5405, 4290, 3214 };				//Eb

volatile unsigned long long capreg_new, capreg_old;
volatile char overflow;
volatile unsigned long long tau,period, period_high, period_low;
unsigned int portb=0;
char num_times=0;



void main(void){
	ANSEL=0x00;
	ANSELH=0x00;
	PORTD=0;
	TRISB=0b11111111;
	TRISC=0b00000010;			//CCP2 pin as input
	TRISD=0b00000000;
	T1CON=0x01;
	PEIE=1;
	GIE=1;
	CCP2IF=0;
	CCP2IE=0;
	TMR1IF=0;
	TMR1H=0;
	TMR1L=0;
	TMR1IE=0;
	CCP2CON=0b00000111;
	
	while(1){
		period=get_period();

		period_high=(period*4108)>>8;  //calculating 5 cent off period (4108=2^12*1.00289)
		period_low=(period*4084)>>8;   //(4084=2^12*(1/1.00289)

		TMR1IE=1;
		CCP2IE=1;

		while(num_times==0);
		capreg_old=(CCPR2H<<8)+CCPR2L;
		while(num_times==1);
		capreg_new=(CCPR2H<<8)+CCPR2L;
		CCP2IE=0;
		TMR1IE=0;
		num_times=0;

		if(capreg_new>capreg_old){    //Code never enters here 
			tau=(capreg_new-capreg_old)+65536*overflow;  //calculate period
		}
		else{
			tau=65536-(capreg_old-capreg_new)+65536*overflow;
		}
		if(tau>period_low & tau<period_high){
			PORTD=PORTD^0b11111111;
		}
	}			
}

void interrupt isr(void){

	if(TMR1IF=1){				//If Timer 1 overflows and nothing has been captured do not increase overflow
		TMR1IE=0;
		if(num_times!=0){
			overflow++;
		}
		TMR1IF=0;
		TMR1IE=1;
	}

	if(CCP2IF=1){
		CCP2IE=0;
		CCP2IF=0;
		num_times++;
	}
}

unsigned long long get_period(void){
	char portb, string, i;
	unsigned long long period=0;    //Function to get the period associated with
	portb=PORTB;			  //the string being tuned

	for(i=0;i<4;i++){
		if((portb&0b11000000)==(i<<6)){
			for(string=0;string<6;string++){
				if(portb&(0b00000001<<string)){
					period=notes[string];
				}
			}
		}
	}
	return period;
}
 

ErnieM

Joined Apr 24, 2011
8,377
Heh... give yourself a smack on the back of the head. :D

Inside your ISR you have these two statements:

if(TMR1IF=1)...

if(CCP2IF=1)...

I do believe you intended comparisons instead of assignments.

I would freely donate body parts for a C compiler that would issue a warning if it ever sees this situation. It's my most common error.
 

spinnaker

Joined Oct 29, 2009
7,830
I would freely donate body parts for a C compiler that would issue a warning if it ever sees this situation. It's my most common error.

I wouldn't need it because I NEVER make this mistake. :eek:

BTW OP You also have

if(tau>period_low & tau<period_high){

//I suspect it should be

if(tau>period_low && tau<period_high){

a single & is a bit operation.
 

t06afre

Joined May 11, 2009
5,934
Heh... give yourself a smack on the back of the head. :D

Inside your ISR you have these two statements:

if(TMR1IF=1)...

if(CCP2IF=1)...

I do believe you intended comparisons instead of assignments.

I would freely donate body parts for a C compiler that would issue a warning if it ever sees this situation. It's my most common error.
Just a note to the OP. It is perfectly OK to use
if(TMR1IF)... as if(TMR1IF==1)

if(!CCP2IF)... as if(!CCP2IF==0)...
I like this style much better. You can also use this for a zero or non zero evaluation on any integer variable
 

ErnieM

Joined Apr 24, 2011
8,377
Another note: I love whitespace (extra spaces tabs and linefeeds) to parse elements on a line. It makes the mental task of catching simple things so much easier.

Which group of code lines would be easier to read?

These:
Rich (BB code):
  if(TMR1IF=1);
  Sum=Rat_A+Rate_B+Rate_C;
  if(!CCP2IF==0);
Or these:
Rich (BB code):
  if( TMR1IF = 1 );
  Sum = Rat_A + Rate_B + Rate_C;
  if( !CCP2IF == 0 );
(two of these lines have typos)
 
Last edited:

Thread Starter

The_Fleertz

Joined Apr 23, 2012
8
You guys are life savers. I gotta remember to check that kind of stuff. :) Anyone have any tips to speed up my code a bit? for 82.4 Hz, low E note, it is really slow at capturing. Also if someone could double-check my tau calculation in the else statement. else{ tau = 65536 - (capreg_old-capreg_new)+65536*overflow;} I just want to be sure that that math will give the correct answer.
Thanks
 

Markd77

Joined Sep 7, 2009
2,806
If your PIC doesn't have a hardware multiplier, you could use bit bashing to get much faster multiply times, eg in this line:
Rich (BB code):
period_high=(period*4108)>>8;  //calculating 5 cent off period (4108=2^12*1.00289)
you could replace the *4108)>>8 with this or the C equivalent, link at bottom to calculate more:

Rich (BB code):
; ACC = ACC * 16.0469
; Temp = TEMP
; ACC size = 16 bits
; Error = 0.01 %
; Bytes order = little endian
; Round = no

; ALGORITHM:
; Clear accumulator
; Add input * 16 to accumulator
; Add input / 32 to accumulator
; Add input / 64 to accumulator
; Move accumulator to result
;
; Approximated constant: 16.0469, Error: 0 %

;     Input: ACC0 .. ACC1, 16 bits
;    Output: ACC0 .. ACC2, 21 bits
; Code size: 46 instructions

    cblock
    ACC0
    ACC1
    ACC2
    TEMP0
    TEMP1
    TEMP2
    endc

;copy accumulator to temporary
    movf    ACC1, w
    movwf    TEMP1
    movf    ACC0, w
    movwf    TEMP0


;shift accumulator right 1 times
    clrc
    rrf    ACC1, f
    rrf    ACC0, f

;add temporary to accumulator
    addwf    ACC0, f
    movf    TEMP1, w
    skpnc
    incfsz    TEMP1, w
    addwf    ACC1, f

;shift accumulator right 5 times
    swapf    ACC0, w
    andlw    0x0F
    movwf    ACC0
    swapf    ACC1, w
    movwf    ACC1
    andlw    0xF0
    xorwf    ACC1, f
    iorwf    ACC0, f
    skpnc
    bsf    ACC1, 4
    clrc
    rrf    ACC1, f
    rrf    ACC0, f

;shift temporary left 4 times
    swapf    TEMP1, f
    movf    TEMP1, w
    andlw    0x0F
    xorwf    TEMP1, f
    movwf    TEMP2
    swapf    TEMP0, f
    movf    TEMP0, w
    andlw    0x0F
    xorwf    TEMP0, f
    iorwf    TEMP1, f

;add temporary to accumulator
    clrf    ACC2
    movf    TEMP0, w
    addwf    ACC0, f
    movf    TEMP1, w
    skpnc
    incfsz    TEMP1, w
    addwf    ACC1, f
    movf    TEMP2, w
    skpnc
    incfsz    TEMP2, w
    addwf    ACC2, f


; Generated by www.piclist.com/cgi-bin/constdivmul.exe (1-May-2002 version)
; Sat Oct 20 18:12:13 2012 GMT
 

ErnieM

Joined Apr 24, 2011
8,377
Anyone have any tips to speed up my code a bit?
First off, don't do anything twice you don't have to do.

Next, don't calculate anything you can put into a look up table.

In your main while(1) loop, you check the buttons and return the "unsigned long long period," yet most times you do this the same button is down and the present answer is the same as the last answer. So save the last state of the buttons and don't calculate is present == last.

Next, can the high and low ranges be per-calculated? (I'd tell you but I didn't read your code that close) (has too many wide tabs for me to see it).
 

THE_RB

Joined Feb 11, 2008
5,438
...
if(CCP2IF=1)...

I do believe you intended comparisons instead of assignments.

I would freely donate body parts for a C compiler that would issue a warning if it ever sees this situation. It's my most common error.
Agreed! In the "good old days" classrooms taught C programmers to write it like this;
if(1==CCP2IF)

as the comparison operator gives the same result but if you erroneously use an assignment operator it always signals an error in code.

Now if only I had a dollar for every time I should have coded like that but never bothered...

(to the OP); Re measuring the period, I think your measurement procedure is not a good way to do it. A better way is to use the capture compare module CCP1 and a 16bit timer, to automatically give you a capture value representing the period.

Another way is to clear the 16bit TMR1 on the pulse / edge, then when you detect the next pulse / edge grab the timer value manually. That can be an easier way for long period where you can also count the times that TMR1 has rolled over to give long period captures of periods > 65536 counts.
 
Last edited:
Top