How heavily optamized are the ATMega's?

Go To Last Post
54 posts / 0 new

Pages

Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Does anyone have a general idea of how heavily optimized the ATMega series are? I'm talking in regards to the compile and how dependent the hardware is on the compiler optimizations for performance. As per several of my previous posts I'm trying to develop a gearbox controller, I have 40ms of time per cycle, running my ATMega88PA at 20mhz gives me 800,000 clock cycles per a gearbox revolution, which should be extreme overkill considering I'm doing simple control with only 5 sensory inputs, most of which are optical. However I seem to be having significant speed issues, and issues just getting what I think is basic code to work. I'm wondering if the optimizations are causing some of my woes. I will turn them off and try it to see what happens, but I'm just curious as to how much performance I would loose in doing so.

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

I'm not sure I understand your question, but whatever problems you're having are almost certainly not due to optimizations (but you don't say what language or toolchain you're using).

Are you using floating point? That eats up plenty of cycles.

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

Hardware is ALWAYS dependent on the compiler generating optimized code. The hardware simply executes a stream of instructions. This is not unique to the AVR.

The instructions themselves are quite efficient on the AVR. However, if the compiler generates sub-optimal code, your application will also be sub-optimal.

As for your performance problems, you need to do a code analysis. Do you have hard-coded delays? Are you using floating point, or doing a lot of multiplies or divides? Are you performing large integer calculations? What features of the C library are you using? Any of the above could have a severe impact on your performance. I also suspect some of your problems will lie around the need for 'volatile' for some of your global variables.

Without seeing any code I cannot say for sure.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

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

Quote:

However I seem to be having significant speed issues,

How are you measuring / analysing that?

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

Quote:
However I seem to be having significant speed issues,

To find out if this is true, set/clear a spare I/O pin at different parts of your code.
Connect that pin to a scope and you will KNOW if there is some part of your code that steal all those cycles.

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

Ok, thanks for the ideas, I may try the spare pin to see what is actually going on. Also in a previous post I had asked if I use cli(); execute code.. sei(); if the interrupt will execute after sei(); re-enables the ISR, I tested that and it did not work, the code was not counting up(which is what the code did in the ISR each time a pin changed from high to low). This was the code I was running:

while(1)		//main loop
	{//Main loop-----------------------------------
		if( (!PINC&_BV(PINC2)) && (position <= 3) )
			PORTD |= (1<<6);
		else if( PINC&_BV(PINC2)) && PINC&_BV(PINC0) )
			PORTD &= ~(1<<6);
                cli();
		if( f1!=TS )
			if(TS){
			   position = position + 1;
			   f1 = TS;}
                sei();
	}//Main loop-----------------------------------
		
}/*Main Program======================================*/

ISR(PCINT1_vect)
{		
	PCDF0 = PINC&_BV(PINC0);
//	PCDF3 = PINC&_BV(PINC3);
//	PCDF1 = PINC&_BV(PINC1);
//	PCDF4 = PINC&_BV(PINC4);	

	if(	PCDF0 < Temp0 )
		TS = 1;
	else
		TS = 0;
		
//	if( PCDF0 != Temp0 ){
//}
//	else if( PCDF3 < Temp3 ){
//		}
//	else if( PCDF1 < Temp1 ){
//		}
//	else if( PCDF4 < Temp4 ){
//		}
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}	

position was not counting up what so ever, so either something I am missing needed to be enabled or that information is incorrect. I want to essentially run through the code in the while loop once without interruption as the counting needs to be exact, check and do what needs to be done if an interrupt occurred, then repeat.

In terms of speed issues, that was actually due to the cli() and sei(), what I thought was happening was it was not fast enough to run through the code and then trigger the interrupt in time, but I think the cause to be what I mentioned above, I removed cli and sei form the start and finish of the code inside the main loop and it was able to count. However I would still have the potential problem of the interrupt occurring during the middle of execution of the code in the main loop and giving me a false value, so I need a way to run the code, check to see if the an interrupt has occurred, execut the code in the interrupt, then start another pass of the main loop and repeat.

Last Edited: Thu. Nov 19, 2009 - 02:01 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

position was not counting up what so ever

Is it volatile? Is it global? And is it actually used? If not the compiler probably didn't bother generating any code.

(which, ironically, is an indication of how optimised the code is! ;-))

Check your .lss for the "position = position + 1" line

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

position is declaired as char in the main program before we start the main loop. I was not aware I needed to declare it volatile since its not being used in the interrupt as TS is, but I suppose it should because its value will change as randomly as TS, is that the reason for declaring it volatile? I shouldn't need to make it global at this point should I as its only used in the main while loop? Also if you look into my interrup, where PCDF0 = PINC&_BV(PINC0);, since the port is digital, if its digital high it would set PCDF0 = 1 and if its low = 0 correct? I wasn't sure if I was alloud to read a digital value directly in, I'm essentially using it as a flag, but I'm thinking now there might be an even quicker way if I can read a pin flag for that pin.

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

Well it's difficult to analyze your code when you only post snippets but consider this whole program:

#include 

int main(void) {
  int position;
  while(1) {
   position = position + 1;
  }
}

How surprised would you be to find that this generates:

0000006c 
: #include int main(){ 6c: ff cf rjmp .-2 ; 0x6c

Oh dear God - who stole 'position'?

Welcome to the world of optimised code.

(if you use pointless variables in pointless code the optimiser will find you out! This is why it's often difficult to post short code samples here as pointless test programs will not generate any code. Two solutions are switching the optimiser off globally (-O0) or locally for the variable (volatile))

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

Here is the whole program:

//purpose: this program controls an AEG gearbox
#include 
#include 

//TS=tappet sensor, PCDFx=pin change deterministic flag
volatile char TS,PCDF0=1,Temp0=1,PCDF3,PCDF1,PCDF4,Temp3,Temp1,Temp4;

int main()
{/*Main Program==============================================*/
volatile char f1,position;

DDRD  |= 0x40;		//set PD6 as output (motor drive)
DDRB  |= 0x02;		//set PB1 as output (motor brake)

PORTD &= ~(1<<6);		//turns off PD6(motor drive)
PORTC &= ~(1<<6);		//turns off PC6(motor brake)
PORTC |= 0x05;		//pullup PC0(Tappet Sensor Input),1(BBDetect),2(Trigger),3(Tcharge),4(MagDetection),5
PORTD |= 0xBF;		//pullup PD0,1,2,3,4,5,7
PORTB |= 0x05;		//pullup PB0,2

sei();			//enable global interrupts 
PCICR  |= 0x02;		//enable PCIE1 for PCINT 14-8
PCMSK1 |= 0x05;		//enable PCINT8 on PC0 (Tappet Sensor), PCINT10 on PC2 (Trigger), PCMSKx enables individual pin interrupts

	while(1)		//main loop
	{//Main loop-----------------------------------
	if( (!PINC&_BV(PINC2)) && (position <= 3) )
			PORTD |= (1<<6);
	else if( PINC&_BV(PINC2) && PINC&_BV(PINC0) )
			PORTD &= ~(1<<6);
		cli();
		if( f1!=TS )
			if(TS){
			position = position + 1;
			f1 = TS;}
		sei();
	}//Main loop-----------------------------------
		
}/*Main Program==============================================*/

ISR(PCINT1_vect)
{		
	PCDF0 = PINC&_BV(PINC0);
//	PCDF3 = PINC&_BV(PINC3);
//	PCDF1 = PINC&_BV(PINC1);
//	PCDF4 = PINC&_BV(PINC4);	

	if(	PCDF0 < Temp0 )
		TS = 1;
	else
		TS = 0;
		
//	if( PCDF0 != Temp0 ){
//}
//	else if( PCDF3 < Temp3 ){
//		}
//	else if( PCDF1 < Temp1 ){
//		}
//	else if( PCDF4 < Temp4 ){
//		}
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}

Some of the code is commented out because its not implemented yet, but I wanted to keep it there as a reminder when I start to write it. I still don't understand why I would need to declare position volatile though, from what I have understood its used in the following:
1. Memory-mapped peripheral registers
2. Global variables modified by an interrupt service routine
3. Global variables accessed by multiple tasks within a multi-threaded application
Position falls within none of these categories. As far as assembly listed above I don't quite understand all of what's being shown there as I've only had one class on assembly and understood about 50% of it, so are you saying position is essentially being discarded due to optimization in my main loop because its not declared as volatile? Also note that when I ran this code position was NOT declared volatile, nor was f1.

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

What use does "position" have in this program? It is never assigned to an external destination so why would anything outside this AVR care whether it existed or not? The optimiser knows this so it doesn't generate any code for it. By using 'volatile' you are saying "this may look completely pointless but, believe me, it isn't". Or by making it global you are inferring that "you may not see it while you compile this C file but for all you know something outside of this file may also be making access to this variable - so it must exist"

Now that we have your whole program I compiled it without that volatile in main and the while(1) is:

   while(1)      //main loop 
   {//Main loop----------------------------------- 
   if( (!PINC&_BV(PINC2)) && (position <= 3) ) 
  d6:	86 b1       	in	r24, 0x06	; 6
         PORTD |= (1<<6); 
   else if( PINC&_BV(PINC2) && PINC&_BV(PINC0) ) 
  d8:	32 9b       	sbis	0x06, 2	; 6
  da:	02 c0       	rjmp	.+4      	; 0xe0 
  dc:	30 99       	sbic	0x06, 0	; 6
         PORTD &= ~(1<<6); 
  de:	5e 98       	cbi	0x0b, 6	; 11
      cli(); 
  e0:	f8 94       	cli
      if( f1!=TS ) 
  e2:	80 91 07 01 	lds	r24, 0x0107
  e6:	98 17       	cp	r25, r24
  e8:	31 f0       	breq	.+12     	; 0xf6 
         if(TS){ 
  ea:	80 91 07 01 	lds	r24, 0x0107
  ee:	88 23       	and	r24, r24
  f0:	11 f0       	breq	.+4      	; 0xf6 
         position = position + 1; 
         f1 = TS;} 
  f2:	90 91 07 01 	lds	r25, 0x0107
      sei(); 
  f6:	78 94       	sei
  f8:	ee cf       	rjmp	.-36     	; 0xd6 

Adding in the "volatile" yields:

   while(1)      //main loop 
   {//Main loop----------------------------------- 
   if( (!PINC&_BV(PINC2)) && (position <= 3) ) 
  e0:	86 b1       	in	r24, 0x06	; 6
         PORTD |= (1<<6); 
   else if( PINC&_BV(PINC2) && PINC&_BV(PINC0) ) 
  e2:	32 9b       	sbis	0x06, 2	; 6
  e4:	02 c0       	rjmp	.+4      	; 0xea 
  e6:	30 99       	sbic	0x06, 0	; 6
         PORTD &= ~(1<<6); 
  e8:	5e 98       	cbi	0x0b, 6	; 11
      cli(); 
  ea:	f8 94       	cli
      if( f1!=TS ) 
  ec:	99 81       	ldd	r25, Y+1	; 0x01
  ee:	80 91 07 01 	lds	r24, 0x0107
  f2:	98 17       	cp	r25, r24
  f4:	51 f0       	breq	.+20     	; 0x10a 
         if(TS){ 
  f6:	80 91 07 01 	lds	r24, 0x0107
  fa:	88 23       	and	r24, r24
  fc:	31 f0       	breq	.+12     	; 0x10a 
         position = position + 1; 
  fe:	8a 81       	ldd	r24, Y+2	; 0x02
 100:	8f 5f       	subi	r24, 0xFF	; 255
 102:	8a 83       	std	Y+2, r24	; 0x02
         f1 = TS;} 
 104:	80 91 07 01 	lds	r24, 0x0107
 108:	89 83       	std	Y+1, r24	; 0x01
      sei(); 
 10a:	78 94       	sei
 10c:	e9 cf       	rjmp	.-46     	; 0xe0 

Only in the latter does 'position' actually exist as Y+2 on the stack.

By the way your indent style suggests the cli()..sei() block might be intended to only execute in the else{} condition - but this is not the case as you didn't use {} to make it a statement block. If that wasn't the intention the indentation is just plain confusing.

Cliff

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

Then would f1 also need to be declared volatile? I am only using f1 in that single code segment as a flag value, outside of that code it does not matter.

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

clawson wrote:

Only in the latter does 'position' actually exist as Y+2 on the stack.

That compiler behavior surprises me. position is used in a test which affects PORTD, so I would expect code to be generated for it.

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

This program could use some more descriptive variable names. Anyway, I see a few possible pitfalls...
1. Your interrupt routine sets TS only if C0 was low and Temp0 was set. This is at the time the ISR is run, not at the time of change - be certain it changes slow enough the ISR gets to test every change. If it's the only pin the PCint is called for, you might not even need Temp0 to track prior state as the pin change interrupt indicates a change did occur. Most importantly, however, don't alter TS if it wasn't a C0 event (which probably means change < to !=).
2. In the cli region, you access TS to see if the ISR has detected the correct edge. That's okay, but the only reason you need cli there is that you're accessing TS thrice. Load it into another variable once, and the critical region can be much shorter.
3. You appear to be trying to pass the event "PINC0 went low". The simple way to pass such an event is a test-and-clear technique; i.e. something like:

volatile unsigned char intvalue=0;
void main() {
  unsigned char value;
  ...
  while(1) {
    cli();
    value=intvalue;
    intvalue=0;
    sei();
    if(value) {
      ...
    }
  }
}
void ISRcode() {
  intvalue++;
}

This method uses a rather small region of code with interrupts disabled, and can cope with the event occurring several times before being processed.
The current code will miss events, particularly if the pin change interrupt is called again as it then clears TS. It appears it's even set to be called for PC2 changes, so this will happen.

Oh, and this program shouldn't need anything beyond the ISR used global variables declared volatile. sei() and cli() themselves should count as barriers to the compiler, so in my sample even that shouldn't strictly be necessary.

With the code technique you're using, main might as well have been testing PINC0 directly. It's doing edge detection anyhow and has no other long-running tasks to be interrupted.

All that said, issues like these are near certain to be logic errors, not performance issues. For instance you didn't initialize position; when do you expect it to be <=3? You can get 0 for global or static variables, but automatic variables are not implicitly initialized.

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

kk6gm wrote:
That compiler behavior surprises me. position is used in a test which affects PORTD, so I would expect code to be generated for it.

But that condition is always false, regardless of the value of position.

if( (!PINC&_BV(PINC2)) && (position <= 3) )

!PINC evaluates to either 0 or 1 depending on the value of PINC. _BV(PINC2) evaluates to 4. 0 & 4, and 1 & 4 both evaluate to 0, therefore the value of the entire expression is 0 and (position <= 3) is not evaluated.

Perhaps ThEThInG meant:

 if( !(PINC & _BV(PINC2)) && (position <= 3) )
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The purpose for if( PCDF0 < Temp0 ) is to detect when that pin has changed on the falling edge, aka a high to a low since the pullup resistors are enabled and that sensor pulls to ground when tripped. Basically I have a photo optic detector in which a plate, initially not blocking the beam, will pass and remain in front of the beam momentarily then return to its original position. this process happens once each gearbox revolution. I wanted to increment position by 1 each time that beam is broken initially, so that would be the falling edge. Now the total gearbox cycle takes 40ms, the plate will block the beam around 50% of the entire cycle, so 15~25 ms, that would mean around 300,000 clock cycles minimum (50ns per clock cycle at 20mhz) will be finished in that time, so that's why I didn't worry about it not scanning in time and the pin changing before the ISR is even run. I also will be adding multiple other sensory devices, this is just the start of the code, but most of the other sensors function very similarly so once I get this working that will be a huge part of it all. Now looking at my code, if I changed this:

	while(1)		//main loop
	{//Main loop--------------------------------------------------
		if( (!PINC&_BV(PINC2)) && (position <= 3) ){
			PORTD |= (1<<6);}
		else if( PINC&_BV(PINC2) && PINC&_BV(PINC0) ){
			PORTD &= ~(1<<6);}

		cli();
		if( f1!=TS )
			if(TS){
				position = position + 1;
				f1 = TS;}
		sei();

	}//Main loop--------------------------------------------------
		
}/*Main Program===================================================*/

ISR(PCINT1_vect)
{		
	PCDF0 = PINC&_BV(PINC0);
//	PCDF3 = PINC&_BV(PINC3);
//	PCDF1 = PINC&_BV(PINC1);
//	PCDF4 = PINC&_BV(PINC4);	

	if(	PCDF0 < Temp0 )
		TS = 1;
	else
		TS = 0;
		
//	if( PCDF0 != Temp0 ){
//}
//	else if( PCDF3 < Temp3 ){
//		}
//	else if( PCDF1 < Temp1 ){
//		}
//	else if( PCDF4 < Temp4 ){
//		}
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}

to this:

	while(1)		//main loop
	{//Main loop-----------------------------------
	if( (!PINC&_BV(PINC2)) && (position <= 3) ){
			PORTD |= (1<<6);}
	else if( PINC&_BV(PINC2) && PINC&_BV(PINC0) ){
			PORTD &= ~(1<<6);}

	}//Main loop-----------------------------------
		
}/*Main Program======================================*/

ISR(PCINT1_vect)
{		
	PCDF0 = PINC&_BV(PINC0);
//	PCDF3 = PINC&_BV(PINC3);
//	PCDF1 = PINC&_BV(PINC1);
//	PCDF4 = PINC&_BV(PINC4);	

	if(	PCDF0 < Temp0 ){
              position = position + 1;}

//	if( PCDF0 != Temp0 ){
//}
//	else if( PCDF3 < Temp3 ){
//		}
//	else if( PCDF1 < Temp1 ){
//		}
//	else if( PCDF4 < Temp4 ){
//		}
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}	

wouldn't that do the same thing since that if statement would only allow position to be incremented on the falling edge? I would then declare position as a global volatile char correct? And no, I didn't see (!PINC&_BV(PINC2)) was syntactically incorrect for what I intended. What I wanted was to compare PC2 and psotion, so PC2 would have to be low and position would have to be less than or = x in order to turn on PD6.

Last Edited: Fri. Nov 20, 2009 - 01:58 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You still haven't got the point Timothy was making. You are using logical NOT (!) when you meant to use binary NOT (~) to invert the bits read from PINC.

! will convert any non zero number to 0 and zero to 1.

~ will convert a bit pattern to its binary inverse (0's become 1 and 1's become 0)

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

Timothy said syntactically it should have been !(PINC & _BV(PINC2)) instead of (!PINC & _BV(PINC2)), the location of ! outside all brackits indicates the inverse of whats inside, so would that not mean the opposite of PINC & _BV(PINC2)? I guess in this case ! or ~ will both work since we are reading digital values on that pin only. What I got wrong I think is where I placed the inversion.

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

No, (~PINC & _BV(PINC2)) would act as intended, but ~(PINC & _BV(PINC2)) would always be non-zero, ie true.

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

Functionally then, in order to see if PC2 is low I would either use !(PINC & _BV(PINC2)) or (~PINC & _BV(PINC2))

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

The convention is:

((PINC & (1<<2)) == 0)   // switch is low
((PINC & (1<<2)) != 0)   // switch is high

Note that you test for non-zero and not 1.

Of course you can add the extra abstraction if you want. But the compiler will not worry.

Another way of doing the same thing is to use exclusive-or with a bit-mask.

I lost the will to live while reading your expression. If you want to use ~ then you need to trace the expression with paper and pencil.

David.

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

TimothyEBaldwin wrote:
kk6gm wrote:
That compiler behavior surprises me. position is used in a test which affects PORTD, so I would expect code to be generated for it.

But that condition is always false, regardless of the value of position.

if( (!PINC&_BV(PINC2)) && (position <= 3) )

!PINC evaluates to either 0 or 1 depending on the value of PINC. _BV(PINC2) evaluates to 4. 0 & 4, and 1 & 4 both evaluate to 0, therefore the value of the entire expression is 0 and (position <= 3) is not evaluated.

Perhaps ThEThInG meant:

 if( !(PINC & _BV(PINC2)) && (position <= 3) )


Yes, you're right. I always use lots of parentheses to make the intended order of evaluation explicit, and I think I just mentally inserted them in the OPs code. :oops: Let this be a lesson to the unwary, parentheses are your friend. Thanks for catching that.

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

Ok, then what would be conventionally the best way to read a value from a pin into a char? This is what I currently have:

ISR(PCINT1_vect)
{		
	PCDF0 = PINC&_BV(PINC0);
//	PCDF3 = PINC&_BV(PINC3);
//	PCDF1 = PINC&_BV(PINC1);
//	PCDF4 = PINC&_BV(PINC4);	

	if(	PCDF0 < Temp0 )
		TS = 1;
	else
		TS = 0;
		
//	if( PCDF0 != Temp0 ){
//}
//	else if( PCDF3 < Temp3 ){
//		}
//	else if( PCDF1 < Temp1 ){
//		}
//	else if( PCDF4 < Temp4 ){
//		}
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}	

Would PCDF0 = PINC&_BV(PINC0); not copy what ever value is in the register for PINC0 into PCDF0? We are dealing with digital so it would only be logic 1 or 0.

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

ThEThInG wrote:
Ok, then what would be conventionally the best way to read a value from a pin into a char? This is what I currently have:

ISR(PCINT1_vect)
{		
	PCDF0 = PINC&_BV(PINC0);
//	PCDF3 = PINC&_BV(PINC3);
//	PCDF1 = PINC&_BV(PINC1);
//	PCDF4 = PINC&_BV(PINC4);	

	if(	PCDF0 < Temp0 )
		TS = 1;
	else
		TS = 0;
		
//	if( PCDF0 != Temp0 ){
//}
//	else if( PCDF3 < Temp3 ){
//		}
//	else if( PCDF1 < Temp1 ){
//		}
//	else if( PCDF4 < Temp4 ){
//		}
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}	

Would PCDF0 = PINC&_BV(PINC0); not copy what ever value is in the register for PINC0 into PCDF0? We are dealing with digital so it would only be logic 1 or 0.


It makes no sense to read a single bit into a char. Perhaps you mean an 8-bit boolean value, zero or not zero? While in practice the two may be the same, you should only use chars when dealing with actual characters. Otherwise you should use 8-bit signed or unsigned values. It just makes your intentions more clear.

Also, doing a < on such 1-bit-set values doesn't make sense. The only comparison that makes sense is equal or not equal. Is the current bit state equal to the saved bit state, or is it different. You do that (in one place) for Temp0 but not for the others. What do you think < gains you in the other cases?

Note also that

if(	PCDF0 != Temp0 )
		TS = 1;
	else
		TS = 0;

(I replaced < with !=)

can be written as

TS = (PCDF0 != Temp0) ? 1 : 0;

or even just

TS = (PCDF0 != Temp0);

It's just a matter of preference which one you choose, although the last one is not obvious enough in its intent for my tastes.

Mike

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

Forget PINC0 as you might think of it as a special case as it only affects bit 0. Instead take PINC3 as an example.

The header files define:

#define PINC3   3

and therefore:

_BV(PINC3) == (1<<3) = 0x08

So when you use:

PCDF3 = PINC&_BV(PINC3);

either bit 3 of PINC is set in which case this is saying:

PCDF3 = 0b????1??? & 0x08
PCDF3 = 0x08;

or the bit is clear in which case it's saying

PCDF3 = 0b????0??? & 0x08
PCDF3 = 0x00;

So your "bit" variables will be set to the bit mask if the bit is set. If you want them to be just 0 or 1 maybe consider:

PCDF3 = (PINC&_BV(PINC3) == _BV(PINC3));

relying on that fact that "true" is represented as 1 and "false" as 0.

Cliff

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

kk6gm: the purpose for looking to see if PCDF0 < Temp0 will tell me if the interrupt for that pin was triggered on the rising or falling edge. I only want to increment position when PINC0 goes from high to low. I realise that using the TS method to increment position requires it to read both high to low and low to high, but if I increment position directly using < should increment it only when the pin goes low and not when the pin goes low to high. So your correct that code wouldn't work. Man this stuff gets confusing some times for new people.

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

Thank you for the explanation clawson, I had thought that by reading the _BV into PCDF0 it was reading if the pin was high or low(logic 1 or 0), not whether the bitmask itself, but that makes sense as to why nothing has been working.

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

I do have a question though, I don't fully understand TS = (PCDF0 != Temp0) ? 1 : 0; as I've never seen this before, I am not familiar with ? 1:0;. Can you please explain? I'm not finding anything on google.

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

I presume that you are using a Pin-change interrupt. You tell which edge by the bit value.

if ((PINC & (1<<2)) != 0) {  // +ve edge
    ...
}
if ((PINC & (1<<2)) == 0) {  // -ve edge
    ...
}

You can never really tell what you are doing with a < operator. Unless you are testing a bit value against zero. But even then you can fall foul if char is signed.

If you want to put the state into a char, you just set flag = 1 or whatever in the if clause.

David.

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

Quote:

I am not familiar with ? 1:0;. Can you please explain? I'm not finding anything on google.

Look in your C references for the "?" or "?:" operator. It is often called the "conditional operator".

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

ThEThInG wrote:
I do have a question though, I don't fully understand TS = (PCDF0 != Temp0) ? 1 : 0; as I've never seen this before, I am not familiar with ? 1:0;. Can you please explain? I'm not finding anything on google.

That's the C ternary operator, sometimes also called the conditional operator, I think.

A = B ? C : D;

means

if ( B )
  A = C;
else
  A = D:
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
 ?  : 

is just shorthand for:

if () {
 
}
else {
 
}

but has the advantage that you can assign the result. A common use is something like:

printf("Condition is % s", (x>3) ? "true" : "false");

Which prints one of

Condition is true
Condition is false

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

using the < is a good trick, if you only ever want to look at one bit, but based on your commented out sections, you are ultimately going to look at multiple bits. In which case I suggest the following modification:

// these could be static insid the ISR if not accessed externally
// if accessed externally they need to be tagged as volatile
   uint8_t PCDF; // flag bits set to 1 when there is a valid change
   uint8_t last; // last reading 

ISR(PCINT1_vect)
{      
  uint8_t tmp;

  tmp = (~PINC) & _BV(PINC0) | _BV(PINC3) | _BV(PINC1) | _BV(PINC4); // inverse logic, as we want a 1 when it goes low
  PCDF = tmp ^ last;  // mark the changed bits
  BCDF &= tmp;        // keep only the bits that went to 0  [1 in tmp, as it is inverse]
  last = tmp;

  if(   PCDF & _BV(PINC0) )
  {
    TS = 1;
  }
  else
  {
    TS = 0;
  }

  if(   PCDF & _BV(PINC3) )
  {
    // do something
  }
  else
  {
    // do something else
  }

  if(   PCDF & _BV(PINC1) )
  {
    // do something
  }
  else
  {
    // do something else
  }

  if(   PCDF & _BV(PINC4) )
  {
    // do something
  }
  else
  {
    // do something else
  }
}

The above will process the edges in parallel, reducing the number of calculations required overall. It also reduces the number of variables you need to keep to maintain state.

[edit] I see there was some additional discussion while I was replying. IF all you are doing is setting a flag, then yes the ternary operator is a good option. But you can bypass that as well, and simply store the boolean result if the value is only ever 1 or 0.

 TS = (PCDF & _BV(PINC0))==0;

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

Last Edited: Fri. Nov 20, 2009 - 06:40 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ThEThInG wrote:
kk6gm: the purpose for looking to see if PCDF0 < Temp0 will tell me if the interrupt for that pin was triggered on the rising or falling edge. I only want to increment position when PINC0 goes from high to low.

Still the < comparison is misleading, and may not do what you want (what if char is signed?). Better to do something like this:

if ( cur state && !last_state ) // true if cur_state is true and last_state is false

or

if ( !cur_state && last_state )  // true if cur_state is false and last_state is true
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

Still the < comparison is misleading, and may not do what you want (what if char is signed?). Better to do something like this:

if ( cur state && !last_state ) // true if cur_state is true and last_state is false

or

if ( !cur_state && last_state )  // true if cur_state is false and last_state is true

Yes the variable would need to be declared as unsigned, if the MSbit is ever used. Otherwise it will always work, and does what you propose in a single operation. It's simply an optimization 'trick' to generate fewer instructions, but is less clear of the intention at the source level.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

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

glitch wrote:

Yes the variable would need to be declared as unsigned, if the MSbit is ever used. Otherwise it will always work, and does what you propose in a single operation. It's simply an optimization 'trick' to generate fewer instructions, but is less clear of the intention at the source level.

Right. I'd be OK with it as long as each use were commented indicating the intended logic. It is clever, I'll admit, and it may save a fair number of instructions, especially since the boolean tests might be promoted to 16 bit operations.

Mike

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

I need to include stdint.h in my header files correct? I'm just asking because when I write in #include and then set uint8_t TS;, uint8_t is not in blue, generally when I use char, int etc. its highlighted in blue which I'm assuming means its written correctly?

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

yes, you need to include stdint.h. syntax highlighting only highlights the [old] standard types.

you can also use "unsigned char" instead of uint8_t if you like.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

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

Thanks to everyone for their help! I greatly appreciate all of it, its certainly quite different being and I'm an EE student so programming is not my forte, however I feel its very important to know at least the basics very well, it seems I have a great ways to go yet. I want to step through the program ISR so I understand what is actually going on, please correct what is incorrect. The condition is, PC0 goes from a logic high to a logic low(we are just looking as the ISR as the the stuff in the main loop I was able to write and understand on my own for the most part, I'll do the comparrison step through in the post below because for some reason my browser is acting up.

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

PC0 goes low, first time ISR is called:

ISR(PCINT1_vect) 
{      
  uint8_t tmp, PCDF, BCDF, last; 

  tmp = (~PINC) & (_BV(PINC0) | _BV(PINC3) | _BV(PINC1) | _BV(PINC4)) ; 
//tmp = 0x01 since (~PINC) & _BC(PINC0) = 1 if PC0 is low, all else is 0

  PCDF = tmp ^ last; 
//PCDF = 0x01 since 0x01(tmp) ^ 0x00(last) = 0x01 

  BCDF &= tmp;  
//BCDF = BCDF & tmp, BCDF = 0x00 & 0x01 = 0x00

  last = tmp; 
//last = 0x01

  if(   PCDF & _BV(PINC0) ) //true
  { 
    TS = 1; //TS is set to 1
  } 
  else 
  { 
    TS = 0; 
  } 

  if(   PCDF & _BV(PINC3) ) 
  { 
    // do something 
  } 
  else 
  { 
    // do something else 
  } 

  if(   PCDF & _BV(PINC1) ) 
  { 
    // do something 
  } 
  else 
  { 
    // do something else 
  } 

  if(   PCDF & _BV(PINC4) ) 
  { 
    // do something 
  } 
  else 
  { 
    // do something else 
  } 
} 

Now the compiler gives me warning about BCDF and last being unitialized, but they cannot be initialized to 0 because that would occur every time the ISR is run thus negating the whole purpose of the code correct? Also because they are not initialized, how do I know what value they have, I'm assuming 0?

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

Your code is extremely convoluted. And will just not work.

Why not explain in English what you want to do? e.g.

If +ve edge on PC.0 then ...
If -ve edge on PC.0 then ...

I can only guess that you want to do different things according to the state of other pins.

Once you have this written, the coding is trivial. But it is very confusing if you try to code directly. Especially if you do multiple inversions.

David.

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

That's the whole point of this post, I don't know how to do everything in a higher level language, I can write pseudo code easily enough, but how to actually translate that into a functioning pice of code to get the micro to do what I want is another story, at least for me. The code is also what clawson suggested, in the comments I was trying to understand what was actually going on inside each operation. I'm the type that needs to know all the way to the core of how something works in order to fully understand it, so that's why I was trying to follow through step wise what was happening to each bit.

In English what I want to do is the following:

1. If any change of state on PORTC pins run the ISR (got that part)
2. Inside ISR determine which pin caused the change of state (this is what I'm trying to set up right now)
3. Run a segment of code to perform an operation for the pin that caused the ISR to be called (only have the code segment with TS in it currently)
4. Return to the main while loop

The most important part for me right now is to determine which pin changed and caused the ISR to be called. I believe that's what everyone has been poking at here and there, there are different ways of doing it, I would preferrably like to do it as efficiently as possible. The code 2 posts previous is what clawson gave me and a way to do that, however I didn't understand the reason for all of its parts, so that's why I posted a question on it and tried to step through it.

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

Good thread on best practices for bit testing and boolean variables and expressions. While we're at it, I might as well point out my observation that if you put the opening brace on the end of the line with the keyword, it saves a line of vertical whitespace for every keyword, and the whole program fits on the screen you are looking at instead of only half. This doubles the 'visual bandwidth' and improves readability. In My Opinion. At least my buddies Brian Kernigan and Dennis Ritchie agree.

Imagecraft compiler user

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

Next order of business is to figure out how to run a segment of code in the main loop without the interrupt interrupting, and then call the ISR after the segment has finished running if an interrupt occured while running that segment. So the following:

main{
while(1){
do stuff 1 (can be interrupted)
do stuff 2 (cannot be interrupted)
}
}

ISR
{
do stuff
}

Now I tried cli(); do stuff sei(); and that does not work, the interrupts never occur. I know there is an interrupt flag, but will that flag still be set even if cli(); and a pin changes state? If it is I can perhaps check the status of that flag after each time I run the uninterruptable code segment then call the ISR after. How I would do that in code I don't know, so any help would be appreciated.

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

ThEThInG wrote:

Now I tried cli(); do stuff sei(); and that does not work, the interrupts never occur. I know there is an interrupt flag, but will that flag still be set even if cli(); and a pin changes state? If it is I can perhaps check the status of that flag after each time I run the uninterruptable code segment then call the ISR after. How I would do that in code I don't know, so any help would be appreciated.

That should work, that is the way microcontrollers are designed. Most interrupts are latched, so they can be recognized when the uC is accepting interrupts. That can either be when the global interrupt enable is set, or when a currently executing ISR finishes. It is also possible to write your code so that a new interrupt can interrupt an executing ISR.

So, you should look elsewhere for your problem, since what you are trying to do should work, and is standard procedure. Are your individual interrupt enable bits set for the interrupts you want to accept?

I should add that you should not block interrupts for a long time, or your system becomes unresponsive. If you are blocking them for a long time you should rethink your approach.

Mike

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

You are still being a little secretive about what you want to do.

You can instigate an ISR when a pin changes. And determine which pin is the one that has changed since the last interrupt.

You need to describe your logic in English. e.g. if run out of fuel then jump out. Or if door has slammed shut then start engine.

Get your algorithm clearly described (and explain what sensors are connected to which pins) and the coding is simple. The most important thing being to describe an event just as logical event. Do not try to worry about a sensor going high or low. That comes with the implementation.

Secondly do not get concerned about enabling or disabling interrupts. Just say English constraints like "do not open door while train is moving".

"Determine which sensor has changed" is a perfectly valid English statement. As is "temperature has risen" or "stop button pressed".

David.

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

This is the entire program as things stand currently:

//purpose: this program controls an AEG gearbox
#include 
#include 
#include 

//TS=tappet sensor, PCDFx=pin change deterministic flag
volatile uint8_t TS,TF=0,position=0;

int main()
{/*Main Program==============================================*/
const uint8_t Cycles=3;

DDRD  |= 0x40;		//set PD6 as output (motor drive)
DDRB  |= 0x02;		//set PB1 as output (motor brake)

PORTD &= ~(1<<6);		//turns off PD6(motor drive)
PORTC &= ~(1<<6);		//turns off PC6(motor brake)
PORTC |= 0x05;		//pullup PC0(Tappet Sensor Input),1(BBDetect),2(Trigger),3(Tcharge),4(MagDetection),5
PORTD |= 0xBF;		//pullup PD0,1,2,3,4,5,7
PORTB |= 0x05;		//pullup PB0,2

//sei();				//enable global interrupts 
//PCICR  |= 0x02;		//enable PCIE1 for PCINT 14-8
//PCMSK1 |= 0x05;		//enable PCINT8 on PC0 (Tappet Sensor), PCINT10 on PC2 (Trigger), PCMSKx enables individual pin interrupts

	while(1)		//main loop
	{//Main loop---------------------------------------------
		if( ((PINC&(1<<2)) == 0) && (position < Cycles ){
			PORTD |= (1<<6);}
		else{
			PORTD &= ~(1<<6);}
		
			if(TF!=TS){
				if(TS){
					position += 1;
					TF=TS;}	}

	}//Main loop---------------------------------------------
		
}/*Main Program==============================================*/

ISR(PCINT1_vect)
{	
	uint8_t PCDF0,Temp0;
		
	PCDF0 = ((PINC&(1<<0)) == 0) ? 1 : 0;
//	PCDF1 = ((PINC&_BV(PINC1)) == _BV(PINC1));
//	PCDF3 = ((PINC&_BV(PINC3)) == _BV(PINC3));
//	PCDF4 = ((PINC&_BV(PINC0)) == _BV(PINC3));		

	TS = (PCDF0 == Temp0) ? 1 : 0;


				
/*	if( PCDF3 < Temp3 ){
		}
	if( PCDF1 < Temp1 ){
		}
	if( PCDF4 < Temp4 ){
		}*/
			
	Temp0 = PCDF0;	
//	Temp3 = PCDF3;
//	Temp1 = PCDF1;
//	Temp4 = PCDF4;	
}

What happens often is if I hold down the button to run the gearbox continuously it will not turn it off when I know for a fact it has completed more than 3 cycles, if I press the button then stop and push again reapeatedly it will count the cycles relatively accuracy. Its almost as if when I hold the button its ignoring some part of the code. The button is on PC2 and the photo sensor which detect a reciprocating plate(that's cammed) is PC0 for reference. The processor is a ATMega88PA runing on an external ceramic resonator at 20MHz. I'm wondering if the ceramic resonator is not giving it a stable enough clock and that could be the cause. My scope is very old and only goes to 15mhz :-( so i can't actually see how much it varies.

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

You're missing a closing parenthesis here. Is that a typo? Does this compile?

if( ((PINC&(1<<2)) == 0) && (position < Cycles ){
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

No, that was a typo when copying the code. The program compiles just dandy. Running dandy is another thing all together :-(. I'm wondering if there is an issue with the Oscillator load caps possibly, the oscillator I am using has 9pF caps built in, the data sheet(atmel's) suggests 12-22pF for a crystal oscillator and manufacturer suggested for ceramic. There could be some sort of clocking issue stemming from that possibly? It programs fine, and always seems to respond fine with anything in the main loop, but that interrupt does not want to work like I want it to. No errata I could find either. I'm wondering if this is an issue with the ATMega88PA vs the older ATMega88P, but from what I understood the PA was just a process change, not functional. I'm stumped as to what is causing this. Any ideas? For reference here is the code one more time, shouldn't be any typos in this.

//purpose: this program controls an AEG gearbox
#include 
#include 
#include 

//TS=tappet sensor, PCDFx=pin change deterministic flag
volatile uint8_t TS;

int main()
{/*Main Program======================================*/
const uint8_t Cycles=3;
uint8_t position=0,TF=0;

DDRD  |= 0x40;	//set PD6 as output (motor drive)
DDRB  |= 0x02;	//set PB1 as output (motor brake)

PORTD &= ~(1<<6);	//turns off PD6(motor drive)
PORTC &= ~(1<<6);	//turns off PC6(motor brake)
PORTC |= 0x05;		//pullup PC0,2
PORTD |= 0xBF;		//pullup PD0,1,2,3,4,5,7
PORTB |= 0x05;		//pullup PB0,2

sei();			//enable global interrupts 
PCICR  |= 0x02;		//enable PCIE1 for PCINT 14-8
PCMSK1 |= 0x05;		//enable PCINT8,10

	while(1)		//main loop
	{//Main loop-----------------------------------
if( ((PINC&(1<<2)) == 0) && (position < Cycles ) ){
			PORTD |= (1<<6);}
		else{
			PORTD &= ~(1<<6);}
		
		if(TF!=TS){
			if(TS){
				position += 1;
				TF=TS;}	}
	}//Main loop----------------------------------
		
}/*Main Program======================================*/

ISR(PCINT1_vect)
{	
	uint8_t PCDF0,Temp0;
		
	PCDF0 = ((PINC&(1<<0)) == 0) ? 1 : 0;		

	TS = (PCDF0 == Temp0) ? 1 : 0;
			
	Temp0 = PCDF0;		
}

Under Project -> configuration options, Optamization is -0s, frequency I designated at 20,000,000hz, device is set to ATMega88p(if I use 88PA i get compile errors with the device not being a recognized part, winavr still hasn't added it to the compiler yet).

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

for reference I did forget one thing, I had cli(); above the if(TF!=TS) statement, and sei(); below TF=TS;. But that's when I kept missing all together, position never counted up.

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

temp0 does not retain value between executions of the ISR, you need to either make it a global, or a local static for it to retain its value.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

Pages