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

AImanAIman Posts: 525
edited 2019-02-04 - 22:02:44 in Propeller 1
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

Comments

  • Of course the space indentations didn't show up in the post ...
  • 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
    I am just another Code Monkey.
    A determined coder can write COBOL programs in any language. -- Author unknown.
    Press any key to continue, any other key to quit

    The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this post are to be interpreted as described in RFC 2119.
  • AImanAIman Posts: 525
    edited 2019-02-04 - 14:37:46
    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.
  • JonnyMacJonnyMac Posts: 6,212
    edited 2019-02-04 - 19:35:58
    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.
    Jon McPhalen
    Hollywood, CA
    It's Jon or JonnyMac -- please do not call me Jonny.
  • 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
    “Perfection is achieved not when there is nothing more to add, but when there is nothing left to take away. -Antoine de Saint-Exupery
  • 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 
          
    
  • So the video I was going to attach apparently isn't allowed. This will require a bit of thought to work around.
  • jmgjmg Posts: 13,561
    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 ?

  • 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.
  • jmgjmg Posts: 13,561
    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.
  • 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 :=
    
  • 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.
  • twm47099 - Thanks!!!
  • JonnyMacJonnyMac Posts: 6,212
    edited 2019-02-05 - 15:12:41
    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)
    
    Jon McPhalen
    Hollywood, CA
    It's Jon or JonnyMac -- please do not call me Jonny.
  • AImanAIman Posts: 525
    edited 2019-03-03 - 20:50:58
    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
    
    
    


  • 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.



  • JonnyMacJonnyMac Posts: 6,212
    edited 2019-03-01 - 17:24:58
    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.
    Jon McPhalen
    Hollywood, CA
    It's Jon or JonnyMac -- please do not call me Jonny.
Sign In or Register to comment.