HELP!! SPIN coding problem with LEDs lighting on down count.

AIman

I feel embarrassed to say this, but for the life of me I can't figure out what it is thats screwed up in this code. All its supposed to do is make a series of 8 LED lights turn on one at a time and go back and forth.

Var
Byte Pin ' I/O pin.

PUB Toggle {Made to toggle 8 LEDs on and off in sequence from low to high and then high to low}

Pin := 16 ' Set value to first pin

dira[Pin]~~

Repeat

Repeat until !outa[Pin]++ == 23 ' Go up one pin at a time to pin 23
waitcnt(clkfreq/4 + cnt) ' Wait for delay cycles

Repeat until !outa[Pin]-- == 16 ' Go down one pin at a time to pin 16
waitcnt(clkfreq/4 + cnt) ' Wait for delay cycles

AIman

Of course the space indentations didn't show up in the post ...

msrobots

use the C to make a code block, then the indentions show up.

You are doing it slightly wrong, here a better version

Var
Byte Pin ' I/O pin.

PUB Toggle {Made to toggle 8 LEDs on and off in sequence from low to high and then high to low}

Pin := 16 ' Set value to first pin


Repeat

  Repeat
    dira[pin] := 1 
    outa[pin] := 1 
    waitcnt(clkfreq/4 + cnt) ' Wait for delay cycles 
  until Pin++ == 23 ' Go up one pin at a time to pin 23
  Repeat
    dira[pin] := 0
    outa[pin] := 0
    waitcnt(clkfreq/4 + cnt) ' Wait for delay cycles 
  until Pin-- == 16 ' Go down one pin at a time to pin 16

Enjoy!

Mike

AIman

Thanks Mike.

Just one question - if I alter the code slightly to shut off the led once they are turned on but putting !outa[Pin] after waitcnt, why doesn't it work going from 23 to 16? So it would read as

waitcnt(clkfreq/4 + cnt)
!outa[pin]

If I do !dira[Pin] it will light up on the way back to 16 but not shut off the others.

JonnyMac

Here's another approach that may be a bit easier to digest. Look for opportunities to put redundant code into methods, and when you keep using the same methods over and over group them into a library.

pub main | pin

  repeat { forever }
    repeat pin from 16 to 23
      high(pin)
      waitcnt(cnt + (clkfreq >> 2))
    repeat pin from 16 to 23
      low(pin)
      waitcnt(cnt + (clkfreq >> 2))  
      

pub high(pin)

  outa[pin] := 1
  dira[pin] := 1

  
pub low(pin)

  outa[pin] := 0
  dira[pin] := 1

For my normal work I have a library called jm_io.spin which handles essential pin IO functions.

Phil Pilgrim (PhiPi)

Just for future reference, AIman, you'll probably get more extensive help if your title includes some information about your issue. "Help!!" is much too vague to elicit the best this forum has to offer.

-Phil

AIman

Code still isn't working. I will attach a video of a few seconds so you can see the original code.

The LEDs will light up for 1/4 second and then shut off for 1/4 second. The LEDs move from pin 16 to 17 to 18 etc. However, on the way down from 23 the LEDs will not light up - or not light properly. There should be one light on at a time and the light should move back and forth across string of lights.

For ease of coding I am using the Propeller Demo Board Rev G since it already has LEDs on it.

This code works halfway, but it is bulky and crude. At this point I will take bulky and crude if it will run like its supposed to.

Var

 Byte Pin                {I/O pin to toggle on/off. Good from 0 - 255}
 
PUB Toggle

  Pin := 16                {Set value of pin to starting point}

  Repeat 
    
    repeat while pin < 24   ' Repeat the following
      dira[Pin]~~           ' Set I/O pin to output direction 
      !outa[Pin]            ' Toggle I/O Pin
      waitcnt(clkfreq/4 + cnt)  ' Wait for delay cycles
      dira[pin]~
      waitcnt(clkfreq/4 + cnt)  ' Wait for delay cycles    
      Pin += 1              ' Go to next pin. Short form of Pin := Pin + 1

    repeat until pin < 16   
      dira[Pin]~~           ' Set I/O pin to output direction 
      !outa[Pin]            ' Toggle I/O Pin
      waitcnt(clkfreq/4 + cnt)  ' Wait for delay cycles    
      dira[pin]~                                     
      waitcnt(clkfreq/4 + cnt)  ' Wait for delay cycles    
      Pin -= 1              ' Go to next pin. Short form of Pin := Pin - 1 
      

AIman

So the video I was going to attach apparently isn't allowed. This will require a bit of thought to work around.

jmg

AIman wrote: »

The LEDs will light up for 1/4 second and then shut off for 1/4 second. The LEDs move from pin 16 to 17 to 18 etc. However, on the way down from 23 the LEDs will not light up - or not light properly. There should be one light on at a time and the light should move back and forth across string of lights.

In fault finding, you should note exactly what does happen, and compare that with what you expect to happen.
'not light properly' is too vague to be useful.
Do the leds (all/some/partial?) fail on down-slope only, and work ok on every up slope time window ?

AIman

The video I was hoping to upload would have shown and clarified most questions but in absence of that -

The LEDs are one to each pin on pins 16 - 23. Pin 16 goes high turning on that LED and after 1/4 second the pin goes low shutting off the LED. This repeats up to pin 23.

BUT

When the pins go from 23 - 16 it doesn't work and only a very faint LED light is visible as the pins count down from 23 to 16.

jmg

AIman wrote: »

When the pins go from 23 - 16 it doesn't work and only a very faint LED light is visible as the pins count down from 23 to 16.

.. and then what happens.. ? Does the next up ramp work ok ?

"doesn't work and only a very faint LED light is visible" is still vague. Does that mean the timing is as-expected, but each single on LEDs is faint, or all leds light, but faint, or something else ?
The small details matter a lot, and help you figure out possible causes.

twm47099

Here's the solution. You need to delay before you toggle the pin when going from P23 to p16.

repeat until pin < 16   
      dira[Pin]~~           ' Set I/O pin to output direction 
      waitcnt(clkfreq/4 + cnt)  ' Wait for delay cycles  - need to delay before turning off the led 
      !outa[Pin]            ' Toggle I/O Pin
      ' waitcnt(clkfreq/4 + cnt)  '   This line needs to be before the !outa[pin]
      dira[pin]~                                     
      waitcnt(clkfreq/4 + cnt)  ' Wait for delay cycles    
      Pin -= 1              ' Go to next pin. Short form of Pin :=

AIman

The next ramp up works fine, but for whatever reason going down doesn't. The LEDs are very dim, almost not noticeable, but have proper timing.

In other words, it works with timing as it should, but the LEDs won't fully light up when subtracting a pin. I will try to post pictures since the video clip can't post.

AIman

twm47099 - Thanks!!!

JonnyMac

The next ramp up works fine, but for whatever reason going down doesn't. The LEDs are very dim, almost not noticeable, but have proper timing.

Because you're not manipulating the IO pins correctly.

I don't mean to be blunt, but you're writing code in a bit of an obtuse manner. In my opinion, it's always best to write code that is obvious.

It seems that you now want to create a Larson Scanner (Cylons, Kit car) -- I didn't understand that from your initial post. As I live and work in Hollywood, I've written lots of Larson scanner code. Here's one way that I think is easy to follow.

pub main | pin

  repeat { forever }
    repeat pin from 16 to 22
      high(pin)
      waitcnt(cnt + (clkfreq >> 3))                             ' delay 1/8 second 
      low(pin)
    repeat pin from 23 to 17
      high(pin)
      waitcnt(cnt + (clkfreq >> 3))
      low(pin)  
      

pub high(pin)

  outa[pin] := 1
  dira[pin] := 1

  
pub low(pin)

  outa[pin] := 0
  dira[pin] := 1

There are many roads that lead to Rome, and many ways to solve the same programming problem. Here's one that's close to your style -- if a bit more obvious:

pub main | pin

  outa[23..16] := %00000000                                     ' set LEDs to outputs
  dira[23..16] := %11111111

  pin := 16
  
  repeat { forever }
    repeat
      outa[pin] := 1
      waitcnt(cnt + (clkfreq >> 3))                             ' delay 1/8 second
      outa[pin] := 0
    while (++pin < 23)
    repeat
      outa[pin] := 1 
      waitcnt(cnt + (clkfreq >> 3))
      outa[pin] := 0
    while (--pin > 16)

AIman

Ironic you should say that about KITT. My kiddo pointed out to me just a short while ago that my lights looked like the light from KITT.

Here is what I got to work.

Var

 Byte Pin     {I/O pin to toggle on/off. Good from 0 - 255}
 Byte Rate    {Number value that controls the speed of the LED Light}
  
PUB LED_Light

  Pin := 16     {Set value of pin to starting point}
                ' Final Value to start zero
  
  Rate := 11   ' TEST VALUE ONLY. REAL VALUE WILL BE SENT FROM OTHER SOURCES.

  Repeat 
    
    Repeat while Pin =< 23        ' Repeat the following
      dira[Pin]~~                ' Set I/O pin to output direction 
      !outa[Pin]                 ' Toggle I/O Pin
      if Pin <> 16
        waitcnt(clkfreq/Rate + cnt)  ' Wait for delay cycles
      dira[Pin]~                 ' Set I/O pin to input direction
      if Pin <> 23
        waitcnt(clkfreq/Rate + cnt)' Wait for delay cycles    
      Pin += 1                   ' Go to next pin. Short form of Pin := Pin + 1

    Repeat while Pin => 16   
      dira[Pin]~~                  ' Set I/O pin to output direction
      if Pin <> 23              
        waitcnt(clkfreq/Rate + cnt)  ' Wait for delay cycles  - need to delay before turning off the led 
      !outa[Pin]                 ' Toggle I/O Pin
      dira[Pin]~                 ' Set I/O pin to input direction
      if Pin <> 16                                 
        waitcnt(clkfreq/Rate  + cnt)' Wait for delay cycles    
      Pin -= 1                   ' Go to next pin. Short form of Pin := Pin - 1

twm47099

Glad it worked. When you post code you should post it between code blocks to preserve the indentation. Put the cursor where you ant to start the code; click on the "C" in the icons at the top of your edit window. That will enter [ code][ /code] (without the spaces between the [ and the first character.) Position the cursor between the ][ and paste your code there.

JonnyMac

In my day job I do a bit of Python coding. There is an Easter Egg built into Python that every programmer can benefit from. If you enter "import this" into a Python REPL, you get...

Beautiful is better than ugly.
Explicit is better than implicit.
Simple is better than complex.
Complex is better than complicated.
Flat is better than nested.
Sparse is better than dense.
Readability counts.
Special cases aren't special enough to break the rules.
Although practicality beats purity.
Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, refuse the temptation to guess.
There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.
Now is better than never.
Although never is often better than *right* now.
If the implementation is hard to explain, it's a bad idea.
If the implementation is easy to explain, it may be a good idea.
Namespaces are one honking great idea -- let's do more of those!

In point of fact, you spent more time adding comments than had you encapsulated confusing code into named methods (like I did with high() and low()).
Also, I suggest you avoid arcane operators when possible. This:

  dira[pin]~~

...uses an arcane operator and requires a comment. This version, however...

  dira[pin] := 1

...would be understood by a fellow embedded programmer and actually runs more than half a microsecond faster. Yes, microseconds count. Not in this program, but if you do enough coding you will come to a point where you are fighting for them. I wrote a very complex (about 6000 total lines) laser-tag control program and I was scrambling for every bit of time savings I could muster. The great thing about the Propeller is that you can use standard code to do timing tests -- here's what I used:

  pin := 16
  
  elapsed := -cnt                                               ' start timing
  dira[pin]~~                                                   ' code under test
  elapsed += cnt - 544                                          ' stop timing
  term.dec(elapsed)
  term.txn(13, 2)

  elapsed := -cnt
  dira[pin] := 1
  elapsed += cnt - 544
  term.dec(elapsed)
  term.txn(13, 2)

In my program, term is an instance of FullDuplexSerial (my variation). The -544 in the final calculation accounts for the Spin overhead. If you remove code from between the updates of elapsed you should get 0.
Finally, if I had been commissioned by on of the special effects clients to write this code, I would have given them this (archive is attached).

pub main

  setup

  ' Larson scanner
  ' -- avoids "sticking" on end boundaries
  
  repeat { forever }
    zip(LSB, MSB-1, ON_MS)
    zip(MSB, LSB+1, ON_MS)


pub zip(first, last, ms) | idx

'' Sequence outputs from first to last
'' -- output delay in milliseconds

  repeat idx from first to last
    io.high(idx)
    time.pause(ms)
    io.low(idx)

This follows the rule by one of my colleagues: If you find yourself writing the same block of code more than once, it's time for a function/method. I frequently remind the new programmers in work to break their code into small, atomic functions/methods so that the code the write has more clarity and the ability to be re-used.
You'll get there. I just takes time and practice.