CCS/EK-TM4C123GXL: It does not follow the code correctly

Your code is hard on the eyes.

Maybe it is easier for you to state which kind of behavior you were expecting and which you are getting.

twelve12pm said:

Conditions like "if (timer == 1000)" may be missed because timer is incremented in an interrupt asynchronously to the operation of main context.

May we call, "Excellent?"      Especially as you've made the time/effort to describe, "How & Why" such (limited) a condition may be "missed."

Another means to "Add Robustness" would be to employ,   "if (timer >= 1000)"  as this eliminates the (overly strict) "singularity of match!"           (such singularity - for what critical reason?)

cb1_mobile said:

twelve12pm

Conditions like "if (timer == 1000)" may be missed because timer is incremented in an interrupt asynchronously to the operation of main context.

May we call, "Excellent?"      Especially as you've made the time/effort to describe, "How & Why" such (limited) a condition may be "missed."

Another means to "Add Robustness" would be to employ,   "if (timer >= 1000)"  as this eliminates the (overly strict) "singularity of match!"           (such singularity - for what critical reason?)

My initial thought was to use "if (timer >= 1000)" but there is a problem with that. From looking at the OP's code it appears the intended behavior is "edge-triggered" not "level-triggered." By asking "if (timer >= 1000)" one would service that condition up to 1000 times, until timer is reset back to 0.

Timer is incremented only in the interrupt handler. It is not modified anywhere else. Therefore the interrupt handler cannot "miss" timer passing through the state of 1000 with a comparison of "==" -- UNLESS:

Unless the programmer forgets that the correct operation of the program depends on timer never being modified anywhere else, and at some future time introduces code which makes such modification. Then, all bets are off. Therefore, to be much more certain that no other code will modify timer due to such a mistake, it can be made a static variable inside the interrupt handler function, so that no other code would have visibility to it. A comment to that effect would be helpful, too :-)

You've made a key/critical observation - I did not read that far into poster's code.

That said - did he "really" require such "edge detection?" That in itself may signal, "less than thoughtful design - and as you neatly noted - opens "current & future code reviewers" - to disaster... (unless that code segment is properly - and notably - commented!)

cb1_mobile said:

You've made a key/critical observation - I did not read that far into poster's code.

That said - did he "really" require such "edge detection?" That in itself may signal, "less than thoughtful design - and as you neatly noted - opens "current & future code reviewers" - to disaster... (unless that code segmented is properly - and notably - commented!)

I can't tell you how many times I've inadvertently caused "level triggered" behavior when I wanted to react to a transition just once.

twelve12pm said:

...many times I've inadvertently caused "level triggered" behavior when I wanted ..."edge triggered"

You are correct, Sir.      That said - the use of the "forgiving test" (i.e. >=) may immediately, "Set some flag" - and thus accomplish the "twin goals" of (both) Code Robustness and "One Time (i.e. Edge) Execution."

Many is the time we've encountered (and resolved) client issues - especially in "Noise Rich" environments - thru use of such "forgiving test."

In addition - when such "forgiving test" is "hit" - cannot that event - "Clear and/or Re-Load" - the value being tested - insuring an "Edge Response?"

Should one - for ANY reason - "miss" that "One and ONLY One" (overly critical) Test - you have "Booked First Class Passage - on the Titanic!" (water's STILL ... (very) cold/choppy - (ice infested) - we are told...)

cb1_mobile said:

twelve12pm

...many times I've inadvertently caused "level triggered" behavior when I wanted ..."edge triggered"

You are correct, Sir.      That said - the use of the "forgiving test" (i.e. >=) may immediately, "Set some flag" - and thus accomplish the "twin goals" of (both) Code Robustness and "One Time (i.e. Edge) Execution."

Agreed that that is more robust and I think worth the addition. That said twelve you deserve a like for your post. Quite agree with cb1 again on that aspect.

Robert

as your code is poorly formatted, here is a reformatted while() loop - hopefully it will make your analysis a little bit easier:

    while(1) {
    	timer+=1;
        if (timer == 1000) {
            flag1 = -flag1;
            if (flag1 == 1) {
                GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, VALV_6);
            } else {
                GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, 0x00);
            }
        }
        if (timer == 2000) {
            flag2 = -flag2;
            timer = 0;
            if (flag2 == 1) {
                GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, VALV_19);
            } else {
                GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, 0x00);
            }
        }
    }

an analysis of it is fairly easy.

let's assume flag1=flag2=-1 at time 0/timer=0:

time0/timer=0: flag1=-1/pin6=0, flag2=-1/pin19=0;

time1000/timer=1000: flag1=1/pin6=1, flag2=-1/pin19=0;

time2000/timer=0: flag1=1/pin6=1, flag2=1/pin19=1;

time3000/timer=1000: flag1=-1/pin6=0, flag2=1/pin19=1;

time4000/timer=0: flag1=-1/pin6=0, flag2=-1/pin19=0;

...

so basically every 2000 counts on timer, pin6/19 are flipped (period = 4000 counts), except that pin6/19 are 90 degrees out (off by 1000 counts).

a quick simulation on an avr showed the same results.

now, I have no idea if that's what behavior you wanted - since you haven't told us. but if that's not what you observed, your problem is somewhere else.

Matheus Nogueira said:

Thanks guys for the replies!
My interest is just that the pins have a period of 4000 counts and a delay one another of 1000 counts (only the state are of my interest)

In that case, forget about the main loop and marshalling "events" between the interrupt and main loop with volatile flags. Simplify the whole thing and do it all in the interrupt... Something like the following:

void Timer0Isr()
{
	static uint32_t timer = 0;
	static uint32_t state = 0;

	TimerIntClear(TIMER0_BASE, TIMER_TIMA_TIMEOUT);

	timer++;
	if (timer >= 1000) {
		timer = 0;

		state++;
		state &= 0x3;

		switch (state) {
			case 0:
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, 0x00);
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, 0x00);
				break;

			case 1:
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, VALV_6);
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, 0x00);
				break;

			case 2:
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, VALV_6);
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, VALV_19);
				break;

			case 3:
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, 0x00);
				GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, VALV_19);
				break;
		}
	}
}


int main(void)
{
	SysCtlClockSet(SYSCTL_SYSDIV_1 | SYSCTL_USE_OSC | SYSCTL_OSC_MAIN | SYSCTL_XTAL_16MHZ);

	GPIOInit();
	TimerInit();

	TimerEnable (TIMER0_BASE, TIMER_A);
	TimerLoadSet(TIMER0_BASE, TIMER_A, SysCtlClockGet() / 100000);

	// Start with these pins in state 0
	GPIOPinWrite(GPIO_PORTF_BASE, VALV_6, 0x00);
	GPIOPinWrite(GPIO_PORTF_BASE, VALV_19, 0x00);

	IntMasterEnable();

	while(1) {
		// VALV_6 and VALV_19 are handled in Timer0 interrupt
		// Possibly do other things here
		// ...
	}
}

Caveat: This is just an example to show what I'm talking about. I haven't actually tested this on hardware, but it will probably give something approaching the desired result. :-)

This code incorporates the suggestions of cb1_mobile and Robert Adsett with respect to the "forgiving" ">=" test. It also makes the intent much more specific: that is, that we want to proceed through these four distinct states one after another in a circular fashion.

Now if one wants to nitpick:

* Strictly speaking, switch statements should have a "default" clause that performs some recovery from whatever inexplicable situation that causes the "state" variable to contain garbage. Since we are performing a bitwise AND with 0x3 in the line just before the switch, it is extremely unlikely that the default would ever be executed, because if "state" did contain garbage, it would now be garbage in the range of 0 to 3.

* Someone will surely point out that my "timer" and "state" variables are not declared volatile. I declare them static inside the Timer0Isr() function, which means: (1) that their contents are preserved between calls and (2) that there is no visibility to these variables from outside Timer0Isr(). Because of said lack of visibility, no code should access these variables asynchronously to the interrupt. You must use volatile when there is asynchronous access from different contexts (main context, thread context, interrupt context). It tells the compiler optimizer not to get smart with that variable.

If you really want extra credit, consider getting rid of the timer variable and test against 1000 entirely and set the timer's period to match the waveform you want. Then your interrupt would only increment "state," AND it with 0x3, and change pin states according to "state."

Nice job - hope our poster appreciates.    As to "extra credit" - crack staff note that such "credit" (even hallowed "extra") fails to satisfy (either) Canada's or Chicago's  "Power & Light."   (staff "expects" the pleasing, (incandescent) "over-head glow" - w/in our "back-room" - to continue...)

cb1_mobile said:

Nice job - hope our poster appreciates.    As to "extra credit" - crack staff note that such "credit" (even hallowed "extra") fails to satisfy (either) Canada's or Chicago's  "Power & Light."   (staff "expects" the pleasing, (incandescent) "over-head glow" - w/in our "back-room" - to continue...)

That's okay. In the submarine business, you get zero credit for diving. You only get points for coming back to the surface alive.

"Something like the following:"

you don't need that kind of complexity: you can do it in the main loop, and with timer count directly, like this:

  switch (timer) {
    case 1000: //do something with gpio
    case 2000: //do something with gpio
    case 3000: //do something with gpio
    case 4000: //do something with gpio
               //reset timer
               timer=0;
  }

you can also put this in the isr but in my view much cleaner and a lot more readable to have it in the main loop.

Danny F said:

"Something like the following:"

you don't need that kind of complexity: you can do it in the main loop, and with timer count directly, like this:

  switch (timer) {
    case 1000: //do something with gpio
    case 2000: //do something with gpio
    case 3000: //do something with gpio
    case 4000: //do something with gpio
               //reset timer
               timer=0;
  }

you can also put this in the isr but in my view much cleaner and a lot more readable to have it in the main loop.

No.

If you do that, then you will run into the problem where you might "miss" the timer equalling 1000, 2000, 3000, and 4000.

It might not happen today. But tomorrow, additional code will be added to do other things, and that code will take longer to execute. You might execute your "switch (timer)" statement when timer is 999, then again when timer is 1001. The resulting waveform will be incorrect. This will all happen because timer changes asynchronously to execution of the main loop. If you do it in the interrupt, then it will work because timer increments synchronously to the "switch (timer)" statement.

I agree with your earlier statement though:

Danny F said:

generating quadrature outputs can be done with timers / compare match. you can specify the phase difference and once set-up, it can be done without the cpu involvement, freeing it up to doing other things.

here is an example: dannyelectronics.wordpress.com/.../

the chip is different but the basic concept is the same.

Waveforms should be generated by hardware peripherals if possible, because that results in a far more accurate waveform (better timing, less jitter) than one generated by software.

Mes Amis,      Did we not  ALL - just hear this poor (and very) Dead Horse - "Cry for Quiet?"
You've both done a great job - let's move onto some REAL issues.      (but not in "this" horse graveyard...)

Note that this thread's "questionable" Subject/Title - almost guarantees that your continued efforts - will be "Little Reviewed."      And they deserve BETTER!

As the thread's originator/author - you "are" likely to review it.

Yet others - when "Searching for posts (somewhat) related to their (specific) needs" - are unlikely to "keyword in" anything near, "Does not follow the code correctly."     There's much of value here - yet the "Subject/Title" (as stated) dooms this thread to, "Low odds of Discovery."

Over-Loading a thread - which is unlikely to be - "dialed up again" - seems not the best means to showcase "poster talent" - much in evidence here...

Nicely done twelve, the state machine is a good idea.

Only one caveat, and I don't think we know enough here to know how to follow it in this case. Structure the state chart to follow the problem, not to follow coincidental timing. IE if you have two outputs that are notionally independent implement separate state machine for them even if the output changes coincide over some portion of the timing (even if that portion is all of it). This makes maintenance substantially easier.

Something like Yakindu makes this kind of maintainence and documentation easier as well.

twelve12pm said:

Now if one wants to nitpick

If you insist

twelve12pm said:

Strictly speaking, switch statements should have a "default" clause that performs some recovery from whatever inexplicable situation that causes the "state" variable to contain garbage.

Yes and certain quality standards (such as MISRA and a number of security standards) make that mandatory. It's a good practice to get into and it costs you little. A good static analyzer will support this as well.

twelve12pm said:

Someone will surely point out that my "timer" and "state" variables are not declared volatile.

Nope, there are two ways of using volatile incorrectly

1. Not using it when it is needed
2. Using it when it's not needed

You don't appear to have committed either fault. Using volatile here would be a mistake, needlessly constraining the compiler.

twelve12pm said:

Because of said lack of visibility, no code should access these variables asynchronously to the interrupt

That's really not the function of volatile. Its use in that situation is a byproduct of its guarantees, not a guarantee in and of itself.

 Robert