Optimization issue or timing issue?

Go To Last Post
9 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am running the following code on an ATMEGA128RFR2, compiled with AVR Toolchain, optimization set to -OS

	case ALERTED_SENDING_MSG :
		prevBState = bState;
		if(currentFoo1Action == IDLE) {					//Foo1 state machine finished
			FOO2_sleep();
			foo1Timer = 0;
			bState = ALERTED;
			currentSysStatus.sleepWakeState = 1;
			if(opSettings.uartEnabled) {
				fprintf_P(&comExt_str, PSTR("bState: ALERTED\n"));
			}
		}
		else if(currentSysStatus.msgsRcvd.disarmRcvd) {	//User disarmed the system
			resetCritVals();
			controlFoo1SM(IDLE);						//Halt Foo1 state machine
			foo2Timer = 0;
			bState = DISARMED;
			currentSysStatus.sleepWakeState = 1;
			if(opSettings.uartEnabled) {
				fprintf_P(&comExt_str, PSTR("bState: DISARMED\n"));
			}
		}
		else if(foo1Timer > max_foo1_sm_duration) {
			controlFoo1SM(IDLE);		//Halt Foo1 state machine
			FOO2_sleep();
			foo1Timer = 0;
			bState = ALERTED;		//Give up on transmitting message
			currentSysStatus.sleepWakeState = 1;
			if(opSettings.uartEnabled) {
				fprintf_P(&comExt_str, PSTR("bState: ALERTED (tx failure)\n"));
			}
		}

		if(tock_happened) {
togBit(GLED_REG, GLED);
			foo1Timer++;
		}
		break;

Note, tock_happened is set elsewhere - once a second. It is latched high for one cycle through the main loop, of which this case in a switch statement is a part. There are many other things going on, and the MCU is talking to many different external peripherals. However, this particular block is not operating as expected.

The togBit statement is for debugging. When I place a printf statement at that line:

printf("foo1Timer: %d\n", foo1Timer)

I see the timer value increasing as expected, and eventually the code falls out from the last else if (foo1Timer > max_foo1_sm_duration). However, when I remove the printf statement, the code hangs in this state forever. Note, it is not truly hung (the overall system keeps functioning, including kicking the watchdog). This particular switch never changes state - which MUST mean that either:

1. currentFoo1Action never equals IDLE
2. currentSysStatus.msgsRcvd.disarmRcvd never equals 1
3. foo1Timer never exceeds max_foo1_sm_duration.

I know that 1. and 2. have not occurred (nor should they have). However, 3. definitely should. I have used other means to prove that tock_happened is, in fact, getting set once a second.

I am a little frustrated, and not sure how to proceed at this point. Is there any chance that the optimizer is somehow altering the logic in this case statement? The addition of the printf statement both adds a delay (not sure how that is relevant in this case) and a use for the foo1Timer value. Does this prevent the optimizer from making the same alteration?

Science is not consensus. Science is numbers.

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

Quote:

Is there any chance that the optimizer is somehow altering the logic in this case statement?

No. The optimizer does not break code. It only highlights code that might have a faulty design in the first place.(*)

I think you need to strip things back to a simpler test case that demonstrates the issue. 9 times out of 10, in doing this you stumble upon the issue anyway but if you don't you then have a test case that others can build, test and analyze and help you to identify the problem. It is vaguely possible it's a compiler error but 99 times out of 100 it isn't.

(*) obviously the main issue with optimization is main/ISR shared variables that aren't "volatile" when they should be. But that's not the only thing the optimiser will weed out. Even the simple for(i<100) style delay loop will be "found out" and discarded.

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

As always, excellent advice. I'm leaning towards the picnic (problem in chair not in compiler) option right now.

Interesting point regarding the volatile keyword. tick is declared as a volatile uint8_t.

It is set to 1 in an ISR tied to timer 3 (TIMER3_COMPA_vect):

#define CTR3_PRESCALE			8		
#define CTR3_LENGTH				0.01	//Used to create tick for SW tasks (seconds)

void timerSetup(void) {
	uint32_t N;

	//Counter Settings
	GTCCR |= (1 << TSM);		//Hold PSRSYNC Value (Prevent hw reset)
	GTCCR |= (1 << PSRSYNC);	//Reset timers


	//Counter 3 = 16 bits -- used to create tick
	TCCR3B |=  (1 << WGM32);	//CTC Mode, internal
	if(CTR3_PRESCALE == 1) {
		TCCR3B |= (1 << CS30);
	}
	else if(CTR3_PRESCALE == 8) {
		TCCR3B |= (1 << CS31);
	}
	else if(CTR3_PRESCALE == 64) {
		TCCR3B |= (1 << CS31) | (1 << CS30);
	}
	else if(CTR3_PRESCALE == 256) {
		TCCR3B |= (1 << CS32);
	}
	else {					//Default to 1024
		TCCR3B |= (1 << CS32) | (1 << CS30);
	}
	TCNT3 = 0x0000;			//Set counter value to 0 (sub other values here if required)

	N = (uint32_t)round((CTR3_LENGTH*F_CPU)/CTR3_PRESCALE);
	if(N > 0xFFFF) {
		//Defaulting to: TIMER3_DELAY: 10ms, TIMER3_PRESCALE: 64
		TCCR3B &= 0xE8;	//0b11101000 -> clear (still set to CTC)
		TCCR3B |= (1 << CS31) | (1 << CS30);
		N = 1535;
	}
	OCR3A = (uint16_t)N;		//N = (F_CPU/(2*PRESCALE*Fout)) - 1, where Fout = desired output frequency (i.e., 4Hz).
	TIMSK3 |= (1 << OCIE3A);	//Enable interrupt on OCOA
	GTCCR &= ~(1 << TSM);
}

ISR(TIMER3_COMPA_vect) {
	tick = 1;
}

Do I need to do anything with respect to atomicity when I check to see if tick == 1?

i.e.:

	while(((rxResult != COMPLETE_MSG) && (rxResult != TERM_MSG)) && (tickCtr < MAX_LOOP_TIME)) {
		rxResult = checkRxData();
		if((rxResult == COMPLETE_MSG) && (bufCnt > 2)) {
			rxFlag = 1;
		}
		
		if(tick) {
			tickCtr++;
			tick = 0;	
		}
	}

What I'm trying to do here is prevent that while loop from becoming infinite if the peripheral doesn't ever respond.

Science is not consensus. Science is numbers.

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

Follow up:

I am not sure if I am simply masking the problem, but when I change the code to;

		ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {		
			if(tick) {
				tickCtr++;
				tick = 0;	
			}
		}

it works every time...

From an assembly code perspective, is there a significant difference between:

if(tick)

and

if(tick == 1)

It seems like either could be a single comparison (i.e., if(tick) == if(tick != 0))

Science is not consensus. Science is numbers.

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

Yes!

if (tick)

will be true if the tick is anything but zero.

if (tick==1)

will be true if tick is 1 and only if tick is 1.

Since I can't find where you defined tick, perhaps you forgot to make tick volatile?

volatile char tick = 0 ;

As the service people at Atmel are finding, beware of volatile brats. Tor is expected to recover.

The largest known prime number: 282589933-1

It's easy to stop breaking the 10th commandment! Break the 8th instead. 

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

It is declared as volatile (not included above). However, my question was more to the point of trying to figure out how many actual instructions the comparison takes.

I.e., to == 1 seems like it would be the same number as to != 0.

Science is not consensus. Science is numbers.

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

To see if it's a speed problem try to ++ in interrupt and see if tick ever is more than 1.
I guess I don't need to ask if tick is volatile :)

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

hobbss wrote:
It is declared as volatile (not included above). However, my question was more to the point of trying to figure out how many actual instructions the comparison takes.

I.e., to == 1 seems like it would be the same number as to != 0.

Yes. If your expressions are boolean, you will only have TRUE or FALSE. In C, this is 1 or 0.

However C is not very strict about types. Any expression that is non-zero is regarded as NOT FALSE. i.e. TRUE

Java will be strict about logical expressions. Modern C compilers only 'advise'. Personally, I have always taken advantage of the non-zero anomaly. Probably due to the way that I used 6502 assembly language. Other processors do not affect the Z flag like the 6502 used to. (almost every opcode)

Just get your program structure nice and clean. Everything will fall into place. (i.e. don't copy my habits)

David.

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

There is a zero flag (Z), there is no "one" flag. So there is an implicit compare to zero in many instructions. After one of them you know if a result is zero or not zero (zero flag set or not set). If you want to know if something equals one, you have to add an instruction that compares with one (to set or not to set the zero flag).

So a compare to zero (or not zero) is usually a little cheaper than to compare with something else.