Not sure if my syntax is correct

Hi,

I have the following code

// If REVERSE and RIGHT are engaged then disable FORWARD and LEFT
If ((PORTB_val & (reverse + right)) == (reverse + right))
{
PORTB_val = PORT_val - (forward + left)
}

I am trying to control a movable device using the direction arrows keys on my keyboard.

If the user presses down and right ( reverse and right direction of device) then i want to disable the forward and left motion in case it fries the devices microcontroller ( not the freeduino board ).

I am using LEDS in the freeduino to simulate the output, but the above code is not working right and i am not getting the proper LEDS to light.

Could someone tell me if the syntax of the code above is correct?

My results are as follows:
Forward = Proper LED lights
Reverse = Proper LED lights
Left = Proper LED Lights
Right = Proper LED Lights
Forward Left = Proper LEDs light
Forward Right = Proper LEDs light
Reverse Left = Reverse and Right LEDs light
Reverse Right = Reverse and Left and Right LEDs light

Does the freeduino have a keyboard?

No, im using a Linksys router running a TCP server, connected to a freeduino board with a serial cable and using a VB6 application running on my laptop to send the commands over WiFi to the router then onto the freeduino

@BBK:

...syntax...

If the syntax were incorrect, the compiler would complain. On the other hand, maybe the semantics are wrong.

What happens if you try the following:

// If REVERSE and RIGHT are engaged then disable FORWARD and LEFT
//
// Assuming "reverse", "right", "forward" and "left" have been defined
// as bytes, each with a single bit set that will be copied to the output
// port:
//
// The "if" condition is satisfied if (and only if) both
// "forward" and "left" bits are set.
//
// The way to turn off the "forward" and "left" bits WITHOUT
// CHANGING OTHER BITS on the port is to perform a
// logic "and" with ~(forward | left)
// 
if ((PORTB_val & (reverse | right)) == (reverse | right))
{
    PORTB_val = PORT_val & ~(forward | left);
}

Bottom line: When setting and resetting bits, use logic operations. I mean, generally speaking, a+b is not necessarily equal to a|b, and a-b is not necessarily equal to a & ~b.

Regards,

Dave

Footnote:
If this doesn't help, how about showing us the complete logic of the sketch? And, by the way, if I dropped the snippet that you posted into a sketch, it wouldn't compile (because of three syntax errors). I think it's kind of important, when asking for help, that you show us exactly what you are working with. Not some abstract that is "pretty much the same as" your code.

Thanks Dave for your reply and your time.

I tried the code you provided and change my code accordingly.

Problem is now with REVERSE RIGHT, when i try this all 4 of my LEDs light.
All other directional inputs seem to work fine

The complete source is:

#define DEBUG 0
#define WAIT_FOR_START 1

unsigned char incomingByte = 0;
unsigned long loop_count = 0;
unsigned char redLED = 64;
unsigned char greenLED = 128;

unsigned char forward = 1;
unsigned char reverse = 2;
unsigned char left = 4;
unsigned char right = 8;

unsigned char PORTB_val;
unsigned char PORTD_val;

unsigned char in_char = 0;

void setup() 
{
  // PORTD - Digital pins 0 to 7
  // Sets the digital pin 6 as output for red LED
  pinMode(6, OUTPUT);
  // Sets the digital pin 7 as output for green LED
  pinMode(7, OUTPUT);      

  // PORTB - Digital pins 8 to 13
  // Sets the digital pin 8 as output for forward 
  pinMode(8, OUTPUT);
  // Sets the digital pin 9 as output for reverse
  pinMode(9, OUTPUT);
  // Sets the digital pin 10 as output for left
  pinMode(10, OUTPUT);
  // Sets the digital pin 11 as output for right
  pinMode(11, OUTPUT);

  Serial.begin(9600);      // set up Serial library at 9600 bps

  // Turn on the red RED
  PORTD = redLED;

#if DEBUG
  flash_led(3,500);
#endif

  //Wait for password from router (SLIGO) and once received continue 
  wait_for_start(); 
}


void flash_led(unsigned int count, unsigned int rate)
{
  // Flashes an LED
  int n_count = 0;

  while (n_count < count)
  {
    n_count++;
    // Turn the LED on
    digitalWrite(13, HIGH);
    // Wait for a bit
    delay(rate);
    // Turn the LED off
    digitalWrite(13, LOW);
    // Wait for a bit
    delay(rate);
  }
}

char get_char()
{
  // Waits for a character from the serial port if none are received, return 0.
  // The timeout is so that if the router stops sending data to the microcontroller,the micrcontroller will stop driving the car.
  // Otherwise the car will continue with the last command received............
  // Timeout is about 250mS.
  while (loop_count < 30000)
  {
    loop_count++;

    if (Serial.available() > 0)
    {
      incomingByte = Serial.read();
      loop_count = 0;
      return incomingByte; 
    }
  }  

  loop_count = 0;

#if DEBUG
  Serial.print('X', BYTE);
#endif

  return 0; 
}

unsigned char wait_for_start()
{
  //Waits for startup message from router serial port
#if WAIT_FOR_START

#if DEBUG
  Serial.println("Waiting...Just anticipating...........");
#endif

  while(1)
  {
    if (get_char() == 'S' && get_char() == 'L' && get_char() == 'I' && get_char() == 'G' && get_char() == 'O') 
    { 

#if DEBUG
      Serial.print("Thank you, Passcode Verified");
#endif

      return 0; 
    }
  }
#endif
}

void loop()                       
{  
  // Process input from serial port and drives the car based on that input.

  in_char = get_char();

  //Split byte received in to upper and lower halves.
  PORTB_val = in_char & 0x0F;
  PORTD_val = in_char & 0xF0;

  //Make sure the greenLED is turned on now.
  if ((PORTD_val & greenLED) == 0)
  {
    PORTD_val = PORTD_val + greenLED;          
  }

  // Make sure controls can't override each other 
  // Make sure LEFT and RIGHT can't be engaged at the same time
  if ((PORTB_val & (left + right)) == (left + right))
  {    
    PORTB_val = PORTB_val - right;    
  }

  // Make sure FORWARD and REVERSE can't be engaged at the same time
  if ((PORTB_val & (forward + reverse)) == (forward + reverse))
  {
    PORTB_val = PORTB_val - reverse; 
  }

  // If REVERSE and RIGHT are engaged disable FORWARD and LEFT
  if ((PORTB_val & (reverse | right)) == (reverse | right))
{
    PORTB_val = PORTB_val & ~(forward | left);
}

  // If REVERSE and LEFT are engaged disable FORWARD and RIGHT
if ((PORTB_val & (reverse | left)) == (reverse | left))
{
    PORTB_val = PORTB_val & ~(forward | right);
}

  // If FORWARD and RIGHT are engaged disable REVERSE and LEFT
  if ((PORTB_val & (forward | right)) == (forward | right))
{
    PORTB_val = PORTB_val & ~(reverse | left);
}

  // If FORWARD and LEFT are engaged disable REVERSE and RIGHT
  if ((PORTB_val & (forward | left)) == (forward | left))
{
    PORTB_val = PORTB_val & ~(reverse | right);
}

  //Write the processed values to the ports.
  PORTD = PORTD_val;
  PORTB = PORTB_val;

#if DEBUG
  Serial.print(PORTD, HEX);
  Serial.print(PORTB, HEX);
#endif

}

Here's my recommendation for developing stuff like this: Test the logic in a separate program before you try to use it to move something!

Here's a sketch where I copied the logic of your program and printed out the results for all possible movement inputs:

//    davekw7x
// My recommendation:
//
// Test the logic before you try to use it to move something
//
const unsigned char forward = 1;
const unsigned char reverse = 2;
const unsigned char left    = 4;
const unsigned char right   = 8;

void setup()
{
    Serial.begin(9600);
}

void loop()
{
    unsigned char PORTB_val;
    Serial.println("  Input Character         Port Bits");
    Serial.println("------------------------------------------");
    // Process input from serial port and drives the car based on that input.
    for (unsigned char in_char = 0; in_char < 16; in_char++) {

        PORTB_val = in_char;
        // Make sure controls can't override each other
        // Make sure LEFT and RIGHT can't be engaged at the same time
        if ((PORTB_val & (left + right)) == (left + right)) {
            PORTB_val = PORTB_val - right;
        }

        // Make sure FORWARD and REVERSE can't be engaged at the same time
        if ((PORTB_val & (forward + reverse)) == (forward + reverse)) {
            PORTB_val = PORTB_val - reverse;
        }

        // If REVERSE and RIGHT are engaged disable FORWARD and LEFT
        if ((PORTB_val & (reverse | right)) == (reverse | right)) {
            PORTB_val = PORTB_val & ~(forward | left);
        }

        // If REVERSE and LEFT are engaged disable FORWARD and RIGHT
        if ((PORTB_val & (reverse | left)) == (reverse | left)) {
            PORTB_val = PORTB_val & ~(forward | right);
        }

        // If FORWARD and RIGHT are engaged disable REVERSE and LEFT
        if ((PORTB_val & (forward | right)) == (forward | right)) {
            PORTB_val = PORTB_val & ~(reverse | left);
        }

        // If FORWARD and LEFT are engaged disable REVERSE and RIGHT
        if ((PORTB_val & (forward | left)) == (forward | left)) {
            PORTB_val = PORTB_val & ~(reverse | right);
        }
        printNibbleBits(in_char); Serial.print(":  ");
        printDirections(in_char);
        
        Serial.print("  |  ");

        printNibbleBits(PORTB_val); Serial.print(": ");
        printDirections(PORTB_val);
        Serial.println();
        delay(1000);
    }
    delay(10000);
}

void printNibbleBits(unsigned char x)
{
    for (unsigned char mask = 0x08; mask; mask >>= 1) {
        Serial.print((mask & x)?'1':'0');
    }
}
    

void printDirections(unsigned char x)
{
    if (x & right) {
        Serial.print("ri ");
    }
    else {
        Serial.print("   ");
    }
    if (x & left) {
        Serial.print("le ");
    }
    else {
        Serial.print("   ");
    }
    if (x & reverse) {
        Serial.print("re ");
    }
    else {
        Serial.print("   ");
    }
    if (x & forward) {
        Serial.print("fo ");
    }
    else {
        Serial.print("   ");
    }
}

This prints the input bits and direction mnemonics and output bits and direction mnemonics for the 16 possible lower four bits values of in_char.

(I used "ri" for "right," "le" for "left," "re" for "reverse," and "fo" for "forward.")

Here's the output I got:


[color=#0000ff] Input Character         Port Bits
------------------------------------------
0000:                |  0000:             
0001:           fo   |  0001:          fo 
0010:        re      |  0010:       re    
0011:        re fo   |  0001:          fo 
0100:     le         |  0100:    le       
0101:     le    fo   |  0101:    le    fo 
0110:     le re      |  0110:    le re    
0111:     le re fo   |  0101:    le    fo 
1000:  ri            |  1000: ri          
1001:  ri       fo   |  1001: ri       fo 
1010:  ri    re      |  1010: ri    re    
1011:  ri    re fo   |  1001: ri       fo 
1100:  ri le         |  0100:    le       
1101:  ri le    fo   |  0101:    le    fo 
1110:  ri le re      |  0110:    le re    
1111:  ri le re fo   |  0101:    le    fo 
[/color]

Now, if these don't do what you want, then trace through your logic to see what happened. If these give the values that you expect, then look at other parts of your original sketch to see where things went wrong.

Regards,

Dave

Dave,

thank you again for your patience with my stupidity.

I ran your code on my freeduino and all the outputs appear to match what i expected them to be.

As i a beginner at this, i dont know what could be causing the problem.
Am i right in thinking it must be hardware based now and not software ?

Thanks again for your help

Still haven't managed to get it working :-[

I have tried everything i can think of and i'm still having a issue with pin 8.

There is like a small volt leak (0.7v) onto pin 8 ( which represents forward) when i press (reverse right)

Any advice or opinions would be greatly appreciated