Is this code ugly?

Jay Kickliter

I wrote what I feel is my first piece of real code, and even though I stole ideas from other objects, it took my almost 2 hours to get this short method to work:

PUB signalStrength
  bytefill(@commandBuffer, 0, 20) 
  byteCounter := 0
  _attention
  phone.str(@phone_signal_strength_command)
  repeat
    rxByte := phone.rx
  until rxByte == ":"
  repeat 
    rxByte := phone.rx
  while rxByte == SPACE
  repeat
    if rxByte == ","
       commandBuffer[noparse][[/noparse]byteCounter++] := 0
    else
       commandBuffer[noparse][[/noparse]byteCounter++] := rxByte
    rxByte := phone.rx
  until rxByte == ","                   
  return @commandBuffer

My problem is that although it works fine, that last repat loop looks funny to me. I've been staring at it for so long, I think I need someone else's opinion. Any suggestions are welcome.

Here the data I'm parsing:

I send "AT+CSQ" to the cell phone

It responds with:

"



+CSQ: 24,99



OK
"

What I'm interested in is the 24 in this example, as that corresponds to the signal strength.

Mike Green

Maybe this will give you some ideas. It starts after phone.str:

repeat until phone.rx == ":"   ' Skip over everything until ":"
repeat while (rxByte := phone.rx) == " "   ' Skip any blanks and leave rxByte set to the 1st non-blank
repeat until rxByte == ","   ' Copy the input to the commandBuffer until the 1st comma (not including the comma)
   commandBuffer[noparse][[/noparse]byteCounter++] := rxByte
   rxByte := phone.rx
commandBuffer[noparse][[/noparse]byteCounter]~   ' Replace the comma with an end-of-string zero

Mike Green

You could even dispense with commandBuffer by converting the decimal number on the fly:

repeat until rxByte == ","
   result := result * 10 + (rxByte - "0")
   rxByte := phone.rx

Note that this doesn't check for invalid information, but then none of this code checks for correct format.

Jay Kickliter

Thanks Mike! That worked perfectly. And looks a whole lot nicer.

Jay Kickliter

Any ideas on how to put a timeout in here in case the battery dies or connector gets unplugged?

I was also wonder on weather I should rewrite these loops for all the phone commands, or make one central method to handle all commands.

Mike Green

You can do a timeout like this:

At the beginning of your routine (when you want to start the timeout) put "startTime := cnt"

In your various loops, put:

if (cnt - startTime) > clkfreq/10
   abort

When you call this routine, use "\" in front of the call (to "catch" the abort). Read the manual about the abort statement.

clkfreq/10 gives you a timeout of 100ms. Change this for some other time that would work better for you.

Jay Kickliter

Thanks mike for all the help.

I tried two things:

if not \debug.dec(phone.signalStrength)
    debug.str(string("Error", 13))

and

if not debug.dec(\phone.signalStrength)
    debug.str(string("Error", 13))

The first one didn't work when I created an abort condition the loop, I never got the error message. The second one did, but since the '\' was inside the debug.dec(), debug.dec() got passed 0, which is not what I was looking for. I was looking for a way to kill the stack up to and including debug.str() (Which will be replaced with something more useful later).

I also ran into another problem. When I unplugged the phone, the abort never terminated the loop, I imaging because phone.rx was just sitting there waiting for a receive and never looped again to catch a byte. Is there no way to have an "external" abort?

Thanks again, Jay

Mike Green

1) You'll need to test phone.signalStrength separately with:

temp := \phone.signalStrength
if temp < 0
   debug.str(string("Error",13))
else
   debug.dec(temp)

You should also change the "abort" to "abort -1" so you have a unique return value on a timeout.

2) You can move your I/O to a separate cog and do a timeout on that. The easiest way to do this would be with FullDuplexSerial which already has a receive with timeout routine. Remember that the transmit routine returns when the character is put into the transmit buffer, not when the character is actually sent.

Rayman

I hope Mike is getting paid by Parallax [noparse]:)[/noparse]