Confusion with For/Next Loop

DougP

New Member
The following code is supposed to extinguish a light connected to c.4 and turn on a light connected to b.5 for the time duration of the loop.
The program works as expected when running the simulator. When the program is loaded into the 14M2 PicAxe and run, the light connected to c.4 goes out and the light connected to b.5 blinks instead of staying lit. The routine does complete as expected.


greenflash: let b0=0
for b0=0 to 50
low c.4
high b.5
if pinc.0=1 then whiteflash
if pinc.1=1 then blueflash
next b0

Thanks for any assistance.

Doug
 

lbenson

Senior Member
OK, first, obvious questions......
And remainder of program.

How fast do you think the loop will complete if pinc.0 and pinc.1 are low? Then what happens? Remember that on the chip, you will run far faster than with the simulator.
 

AllyCat

Senior Member
Hi,

The most likley cause is that you don't have any pauses, so it will all happen literally in the blink of an eye.

Also, do you have pulldown resistors on c.0 and c.1? If either of these pins goes high the code will exit the FOR loop, with apparently no way back (no label to jump to). Nor do you appear to restore c.4 to high or c.5 to low.

Cheers, Alan.
 

DougP

New Member
Sorry for the delay in the response. The PicAxe Forum will not allow me to send an attachment which is over 976kb. Both of the pictures I want to send exceed that limit. If you have no objection to supplying your email address, I could forward the pictures. My email address is [B][B][I]removed by moderator to avoid spam[/I][/B][/B].

Thanks,
Doug
 
Last edited by a moderator:

geoff07

Senior Member
You could crop the images or shrink your .jpg files using e.g. The GIMP (free software for Linux, MAC, and, if you must, Windows.)
 

lbenson

Senior Member
It would be best to post your code directly by copying it and pasting it within the tags "[ code]" and '[ /code]" (without the spaces).

Sorry, but your code is classic "spaghetti code", with "goto"s within "for .. next" loops, so it is very hard to follow, especially since no symbols are used to provide meaningful names to pins and variables. Can you provide a description of what is supposed to happen and which input and output pins signify what?

But for instance, with regard to the lack of pauses, in your "greenflash" routine, if the inputs are both low, the code will be executed 50 times very quickly (at around 2,000 to 4,000 basic statements per second), and will fall into "blueflash", which will also execute in less than a second if the inputs are low.
 

oracacle

Senior Member
to me it look as though it running its 50 loop that will take half a second maybe and is then overflowing into blueflash that instantly switched of b.5, there does also seem to be a lot of repeated code too which cold be done with a sub procedure, perhaps the blue and green flash should be done the same way

just ran a simulation, and holding c.2 creates something similar as far as I can tell to your description, similar to holding c.1 high

Code:
[color=Black]whiteflash:
      [/color][color=Blue]low b.5[/color][color=Black],[/color][color=Blue]c.4
      high b.0
      pause [/color][color=Navy]200
      [/color][color=Blue]low b.0
      if [/color][color=Purple]pinc.1 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]greenflash
      [/color][color=Blue]if [/color][color=Purple]pinc.2 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]blueflash
      [/color][color=Blue]high b.1
      pause [/color][color=Navy]200
      [/color][color=Blue]low b.1
      if [/color][color=Purple]pinc.1 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]greenflash
      [/color][color=Blue]if [/color][color=Purple]pinc.2 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]blueflash
      [/color][color=Blue]high b.2
      pause [/color][color=Navy]200
      [/color][color=Blue]low b.2
      if [/color][color=Purple]pinc.1 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]greenflash
      [/color][color=Blue]if [/color][color=Purple]pinc.2 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]blueflash
      [/color][color=Blue]high b.3
      pause [/color][color=Navy]200
      [/color][color=Blue]low b.3
      if [/color][color=Purple]pinc.1 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]greenflash
      [/color][color=Blue]if [/color][color=Purple]pinc.2 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]blueflash
      [/color][color=Blue]high b.4
      pause [/color][color=Navy]200
      [/color][color=Blue]low b.4
      if [/color][color=Purple]pinc.1 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]greenflash
      [/color][color=Blue]if [/color][color=Purple]pinc.2 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]blueflash
      [/color][color=Blue]goto [/color][color=Black]whiteflash

      [/color][color=Green]'these create a loop within them selves until c.0 goes high[/color]
[color=Black]greenflash:
      [/color][color=Blue]let [/color][color=Purple]b0 [/color][color=DarkCyan]= [/color][color=Navy]0
      [/color][color=Blue]for [/color][color=Purple]b0 [/color][color=DarkCyan]= [/color][color=Navy]0 [/color][color=Blue]to [/color][color=Navy]50
            [/color][color=Blue]low c.4
            high b.5
            if [/color][color=Purple]pinc.0 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]whiteflash
            [/color][color=Blue]if [/color][color=Purple]pinc.2 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]blueflash
      [/color][color=Blue]next [/color][color=Purple]b0
      [/color][color=Green]'b.5 high for just 50 loops until switched off in blueflash[/color]
[color=Black]blueflash:
      [/color][color=Blue]let [/color][color=Purple]b1 [/color][color=DarkCyan]= [/color][color=Navy]0
      [/color][color=Blue]for [/color][color=Purple]b1 [/color][color=DarkCyan]= [/color][color=Navy]0 [/color][color=Blue]to [/color][color=Navy]50
            [/color][color=Blue]low b.5
            high c.4
            if [/color][color=Purple]pinc.0 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]whiteflash
            [/color][color=Blue]if [/color][color=Purple]pinc.1 [/color][color=DarkCyan]= [/color][color=Navy]1 [/color][color=Blue]then [/color][color=Black]greenflash
      [/color][color=Blue]next [/color][color=Purple]b1
      [/color][color=Blue]goto [/color][color=Black]greenflash[/color]
double check your circuit
 
Last edited:

jims

Senior Member
"spaghetti code" is a new term for this non-programmer. Thanks to ibensen I searched on Google & learned something new. Does anyone have an example of how this "spaghetti code" can be converted to a more "classic" style of program and maintain the same logic as oracacle's code in #9 above? Jims
 

hippy

Technical Support
Staff member
Though it looks like spaghetti code I wouldn't necessarily say it is - assuming it's meant to do what I think it does - which is to choose an appropriate LED flashing sequence depending on which input is activated.

Wanting to be inside one of a number of loops ( LED sequences ) and able to move to a different sequence loop when an input is activated requires some sort of jump out of each loop, and there have to be numerous tests for input changes if it is to be responsive. Even interrupts don't help because then one simply ends up with numerous jumps on an 'interrupt has been called' flag, which isn't much better.

It is possible to make it look more aesthetically pleasing but I wouldn't say it was worth the effort, as there's nothing fundamentally wrong with what there is. Real spaghetti code goes all over the place and is impossible to follow, this is fairly easy to understand once one figures out what it is doing.

I think the only problem is that there aren't any pauses in the green and blue flashing routines as mentioned earlier, and green flash may not be intended to fall through into blue flash.
 

AllyCat

Senior Member
Hi,

Does anyone have an example of how this "spaghetti code" can be converted to a more "classic" style of program and maintain the same logic as oracacle's code in #9 above? Jims
As hippy says it's not really a good (or should it be bad) example of spaghetti code. However, there may be some "spaghetti thinking" because these two statements come from the first 5 lines of the OP:

"turn on a light connected to b.5 for the time duration of the loop."
"the light connected to b.5 blinks instead of staying lit." :confused:

The real issue is that there are absolutely no comments or symbolic names, so it's difficult to determine what the code is doing, or what the Programmer thinks it is doing. There are 3 inputs and 7 outputs, but all we know (from the OP text) is that there are two "lights", which might be anything from a LED to a 1000 watt relay-controlled flloodlight. Is this the full program (as there are no init: or main: labels)? Are the inputs and outputs "Active High" or "Active Low" and might more than one input become active at a time (if so does it matter what happens)?

The actual code structure also makes one "suspicious" that it might not accurately represent what was intended. There is no need to set b0 = 0 immediately before it's set in a FOR loop, the green/blue loops are executed 51 times, and why are the High / Low commands repeated with every pass through the FOR loop? However, the fall-through from greenflash into blueflash perhaps is intended, as there is a goto from the end of blueflash back to greenflash.

Therefeore, I had to make a number of wild assumptions to derive this allternative code. It might not be "correct", but is shorter and hopefully clearer in its functioning. Note the use of constants (in place of "magic numbers") allows the delay times, and even the Active High / Low inputs, to be changed without delving deep into the code (although the PICaxe Basic "switch" command does assume Active High).

Code:
#picaxe 14m2
#no_data					; Avoid unnecessary downloading time

symbol GBDELAY = 10				; Green/Blue alternation period (1 unit = 50ms)					
symbol WHITEDELAY = 20000			; White pulse time (1 unit = 10 us)
symbol FIRSTPIN = b.0				; First "white" pin
symbol LASTPIN  = b.4				; Last "white" pin
symbol GREENLED = b.5				; Pin Active High
symbol BLUELED  = c.4				; Pin Active High
symbol outpin = b0
symbol whitebutton = pinc.0
symbol greenbutton = pinc.1
symbol bluebutton  = pinc.2
symbol PUSHED = 1				; Buttons Active High

whiteflash:
	switchoff GREENLED,BLUELED
main:
	for outpin = FIRSTPIN to LASTPIN
		pulsout outpin,WHITEDELAY
		if greenbutton = PUSHED then greenflash
		if bluebutton = PUSHED then blueflash
	next outpin
	goto main				; Repeat the white sequence
greenflash:
	switchoff BLUELED
	switchon GREENLED
	for b0 = 0 to 50
		pause GBDELAY   
		if whitebutton = PUSHED then whiteflash
		if bluebutton = PUSHED then blueflash
	next b0							; Then fall through into blueflash
blueflash:
	switchoff GREENLED
	switchon BLUELED
	for b0 = 0 to 50
	   	pause GBDELAY
		if whitebutton = PUSHED then whiteflash
		if greenbutton = PUSHED then greenflash
   	next b0
	goto greenflash
Cheers, Alan.
 
Top