A CASE of confusion

JonnyMac · 2010-09-20 05:09

I decided to toss a last-minute section into my column and am now kicking myself and seriously confused.

What I decided to do is develop a simple implementation of the Renard protocol which is very popular amongst DIY Christmas lighting enthusiasts. The protocol is very simple

sync ($7E)
addr ($8x)
channel levels

$7F is used as an escape character for levels that require two bytes encoding. Easy-peezy. Right.

Well, yes -- and this works with my little 12-channel board:

  repeat                                 
    c := cmd.rx                          
    if (c == SYNC)                       
      c := cmd.rx                        
      if (c == ADDR)                     
        repeat ch from 0 to CHANNELS-1           
          c := cmd.rx                    
          if (c <> ESC)                  
            leds.set(ch, c)              
          else                           
            c := cmd.rx                  
            if (c => $30) and (c =< $32) 
              leds.set(ch, $AF-c)        
            else                         
              quit

I thought, however, that a state-machine would be more elegant and be better suited to trap an error in the stream. So I recoded to....

  state := 0
  repeat
    c := cmd.rx

    case state
      0 : if (c == SYNC)
            state := 1
            
      1 : if (c == ADDR)
            state := 2
            ch := 0
          else
            state := 0
            
      2 : if (c <> ESC)
            leds.set(ch, c)
            ch += 1
          else
            state := 3
            
      3 : if (c => $30) and (c =< $32)
            leds.set(ch, $AF-c)
            ch += 1
            state := 2
          else
            state := 0

    if (ch == CHANNELS)
      state := 0

I'm stumped and a tad frustrated because I'm sure it's something ridiculously simple that I'm looking right past. Your input is appreciated

JonnyMac · 2010-09-20 06:00

I moved the last channel check from outside the CASE structure to inside and it works now. Seem odd....

Mike Green · 2010-09-20 07:04

Might it be something to do with "ch" not being initialized until after state 1?

LoopyByteloose · 2010-09-20 07:23

Compiler bug?

JonnyMac · 2010-09-20 07:43

Mike Green wrote: »

Might it be something to do with "ch" not being initialized until after state 1?

I thought that might be the case, too, but it's not (what's not shown is that ch is pre-initialized to 0 before the repeat loop).

This is the dolled-up working version; for now I'm going to live with the redundant channel limit check.

  state := WAIT_SYNC
  chan := 0                          
                                              
  repeat                                      
    rxbyte := cmd.rx                          
    case state                                
      WAIT_SYNC : if (rxbyte == SYNC)         
                    state := WAIT_ADDR        
                                              
      WAIT_ADDR : if (rxbyte == ADDR)         
                    state := WAIT_LVL1        
                    chan := 0                 
                  else                        
                    state := 0                
                                              
      WAIT_LVL1 : if (rxbyte <> ESC)          
                    leds.set(chan, rxbyte)    
                    if (++chan == CHANNELS)   
                      state := WAIT_SYNC      
                  else                        
                    state := WAIT_LVL2        
                                              
      WAIT_LVL2 : if (rxbyte => $30) and (rxbyte =< $32)
                    leds.set(chan, $AF-rxbyte)     
                    if (++chan == CHANNELS)   
                      state := WAIT_SYNC      
                    else                      
                      state := WAIT_LVL1      
                  else                        
                    state := WAIT_SYNC

Mike Green · 2010-09-20 07:48

Check your indenting again

I would be surprised if there were a compiler bug at this late date. I've used state machines like this one including a statement after the end of the CASE statement, but inside the REPEAT and it's worked for me.

JonnyMac · 2010-09-20 09:59

Like you, Mike, I've used this structure before without problems. And, yes, I checked the indenting very carefully (graphic display of indent is enabled) -- even moved the channel limit check to the top of the repeat loop (before rx input) just to make sure.

I agree that it's probably not a compiler bug, but there does seem to be an unknown (to me, that is). For now I have working code and I'm pressing forward. Thanks for looking.

Phil Pilgrim (PhiPi) · 2010-09-20 10:58

Jon,

I'm not sure about the CASE issue, but I do notice that if you get two SYNCs (or any even number) in a row, starting from WAIT_SYNC, and followed by an address, your interpreter will not do what you probably want it to:

WAIT_SYNC: SYNC -> WAIT_ADDR
WAIT_ADDR: SYNC -> WAIT_SYNC
WAIT_SYNC: ADDR -> [color=red]WAIT_SYNC[/color]

I think that SYNCs arriving at WAIT_ADDR should not change state, but remain at WAIT_ADDR. In fact, if I understand the protocol correctly, a SYNC arriving at any state should force the next state to WAIT_ADDR.

-Phil

JonnyMac · 2010-09-20 12:02

Thanks, Phil, that makes perfect sense -- and I thought I had covered that situation (obviously not). Here's where I am at present, which, hopefully, which catch a "broken" stream. It seems to be okay with me "breaking" the connection and then putting it back.

  repeat
    cmd := console.rx
    case state
      W_SYNC : if (cmd == SYNC)
                 state := W_ADDR
            
      W_ADDR : if (cmd == ADDR)
                 state := W_LVL1
                 chan := 0                         
               elseif (cmd <> SYNC)
                 state := W_SYNC
                                                   
      W_LVL1 : if (cmd <> ESC)                     
                 leds.set(chan, cmd)               
                 if (++chan == CHANNELS)              
                   state := W_SYNC                 
               else
                 state := W_LVL2                   
            
      W_LVL2 : if (cmd => $30) and (cmd =< $32)    
                 leds.set(chan, $AF-cmd)           
                 if (++chan == CHANNELS)           
                   state := W_SYNC                 
                 else
                   state := W_LVL1              
               elseif (cmd == SYNC)
                 state := W_ADDR
               else
                 state := W_SYNC

A CASE of confusion

Comments