Array problems( i think....)

[Andrew Blair]

I have recently been working on a copy cat program, and am just in the development stages, so right now i'm working on placing array values every program cycle from user input, then just replaying them. Right now i'm just putting the array values to ram (we don't have much code right now, so our ram is pretty much empty), then replaying them. Ultimately I plan to save them to eeprom by pressing a save button.

My problem is that the code compiles fine, but when the copycat is executed, I have no control and it blinks code error *almost* immediately. The way my code is set up, I have about 2 seconds of record time, and it dies about 2 seconds in. (I do intend to scale my code appropiately, so i'll have much more record time than that, but for right now it's fine recording a value every program cycle).


I warn you, what you are about to see is very crude and may shock you with blatent programming errors. I am actually expecting my array to be very screwed up, because i'm still learning how to use them.

unsigned char left_side[90];    
unsigned char left_side[90];
unsigned char right_side[90];

unsigned int i;


//record device, takes i variable(increases address every program loop), takes value from joystick input.

if (p2_sw_trig==1)
{
   i=0;
   i++;
while( i<90)
{
left_side[i]=p1_y;
right_side[i]=p2_y;
}

if (i<90)
{
  pwm01,pwm02=127;
}

}


//replay device, takes value from table and puts it to pwms

if ( p1_sw_trig==1)

{
i=0;
i++;
while(i<90)
{
pwm01=left_side[i];
pwm02=right_side[i];
}

if(i>90)
{
  pwm01=127;
  pwm02=127;
}

}

Thanks so much in advance! I hope I put that all down right. I just realized I didn't check the main processor anytime in that while loop. Could that my problem?

[devicenull]

while( i<90)
{
left_side[i]=p1_y;
right_side[i]=p2_y;
}

You forgot i++;, so its just an infinite loop.

[sciguy125]

Originally Posted by Andrew Blair

Thanks so much in advance! I hope I put that all down right. I just realized I didn't check the main processor anytime in that while loop. Could that my problem?

I'm not sure if that would cause your code error, but unless the main processor read function (I can't remember what it's called off the top of my head) is called, your outputs (and inputs) never get updated.

Although, your infinite loop may be the cause of the code error:

if (p2_sw_trig==1)
{
   i=0;
   i++;
while( i<90)
{
left_side[i]=p1_y;
right_side[i]=p2_y;
}

i is never incremented in the while loop, so i<90 is always true. I'm also not sure if this is legal:

  pwm01,pwm02=127;

But, I guess if the compiler didn't complain, it's alright.

[jdiwnab]

Originally Posted by devicenull

while( i<90)
{
left_side[i]=p1_y;
right_side[i]=p2_y;
}

You forgot i++;, so its just an infinite loop.

True, but also, that loop would record the same values about 90 times (+/- 1). Better would be to have the record code in the main program and just do 2 if's (if button and i<90) so that it records what you do. Then in the replay code, use for(i=0, i<90, i++) {...} so that it will loop though it that number of times. This makes changing your code easier and easier to read.

<edit>

Originally Posted by sciguy125

I'm also not sure if this is legal:

   pwm01,pwm02=127;

But, I guess if the compiler didn't complain, it's alright.

A better way would be "pwm01=pwm02=127;" becuase pwm02 then is 127, afterword, pwm01 would be set equal to pwm02, making pwm01 127. You could check the values with a debugger or have some visual output, but I don't think "pwm01,pwm02=127" does what you think it does, but it isn't the cause of the problem.

[TimCraig]

You need to place code like the following somewhere that it gets called every time through the 26 millisecond control loop so your joystick values will be updated.

static unsigned char i = 0;

if ((pw_sw_trig1 == 1) && (i < 90))
{
left_side[i] = p1_y;
right_side[i] = p2_y;
i++;

pwm01 = pwm02 = 127;
} // end if

Note that the counter is a static variable so it holds its value between passes through the loop.

Also, you increment the counter before you use is so aside from your other problems, you'd never fill in the first item in each array. Remember C arrays start at 0. If pwm01 and pwm02 aren't being changed somewhere else in your code, you don't need to set them each pass through the loop. Or if you're, recording actual motor performance, then you don't need to keep setting them to 127.

Hope this helps.

[Ryan M.]

The reason (as someone sort of mentioned) that you're getting the code error is because it never calls the master_up function. (Sorry, don't remember the exact name.) If the master processor doesn't hear from your code (through that function) at least every 26 ms, it stops running the code and alerts you to a problem with the code error light.

pwm01,pwm02=127

Just to clear this up, the comma operator is valid C, however, all it does it does is create a sort of "soft" semicolon that allows you to put in a new command, but not actually end the current one. It's a little confusing, but the context you see it most often in is for loops. IE:

for(i = 0, j = 2; i < someArray[j]; i++, j++)
{
}

Overall, you don't need to use it. As a couple people said,

pwm0 = pwm02 = 127;

is what you actually want.

Good luck on this!

--EDIT--
Sorry, that was the best explaination for the comma operator I could come up with on the fly. Computer science majors, don't shoot me!

[Andrew Blair]

Thanks so much! I'll try it out this morning and see i i can get it to work. And I promise, no more commas!

[Andrew Blair]

Well, i've fixed what has been advised, an it compiles and runs. I now have no code error and can drive in the record/replay mode, but it does not seem to be recording. Reason I know, it does not replay! Any suggestions? Thanks again!

[foobert]

i know this isn't the problem you're currently working on, but it will be soon.

in your replay code at the end of the while loop i will be 90. the if statement that follows will fail because i will not be greater than 90, so your pwm will stay at the last recorded value and you bot will keep running. very bad.

there is no need for a conditional at all, since when the loop stops you want the robot to stop.

[Dave Scheck]

Andrew

You need to remember that, by default, the software in your robot controller will only read the joysticks once per loop (Getdata in Process_Data_From_Master_uP).
Same goes for outputting pwm values (Generate_Pwms in Process_Data_From_Master_up).
<Slightly off topic>For reference, the only thing that you're able to read and be assured that it's the current value is the digital inputs because their software representation is tied directly to the pin</Slightly off topic>

I think is what you really want to do is the following....
<disclaimer>This code purposely has some small syntax errors to force the reader to actually understand the code. None of them will affect the program flow, they will just cause compilation errors.</disclaimer>

// In a header that user_routines.c can see
// It is a good habit to get into to create defines for your
// inputs and outputs.  That allows the programmer to write code
// with arbitrary mappings until the interface is finalized.  Also
// if all of a sudden you want to record with port 3 top, you only
// have to change it in one place in the #define
// Also notice that I assigned meaningful names to the pwms, and constants
#define record_button p2_sw_trig
#define playback_button p1_sw_trig
#define LEFT_DRIVE_STICK  p1_y;
#define RIGHT_DRIVE_STICK  p2_y;
#define LEFT_DRIVE_MOTOR  pwm01;
#define RIGHT_DRIVE_MOTOR  pwm02;
#define N_SAMPLES 90
#define PRESSED 1
#define NOT_PRESSED 0

//In your slow loop (Process_Data_From_Master_uP) 
// Use static variables so that they hold their value
// between loops
static int sample_num = 0
static unsigned char left_side[N_SAMPLES]
static unsigned char right_side[N_SAMPLES]
static unsigned char prev_record_button = NOT_PRESSED
static unsigned char prev_playback_button = NOT_PRESSED

// Temp variable
int temp;

// This takes into consideration the case that both buttons are pressed.  
// You will probably also need some sort of reset signal to clear the recorded values.  I have made the assumption that every time the buttons are pressed
// they will be reset
if(record_button == PRESSED and playback_button == NOT_PRESSED)
{
  if(prev_record_button == NOT_PRESSED)
  {
   //We are just starting to press the button
   // Clear the recorded values
    for(temp = 0; temp < N_SAMPLES)
    {
      left_side[sample_num] = 127
      right_side[sample_num] = 127
    }
    // Reset the sample number
    sample_num = 0;
  }

  if(sample_num < N_SAMPLES)
  {
    // Record the current states
    left_side[sample_num] = LEFT_DRIVE_STICK
    right_side[sample_num] = RIGHT_DRIVE_STICK
  }
  sample_num++
}

if(playback_button == PRESSED and record_button == NOT_PRESSED)
{
  if(prev_playback_button == NOT_PRESSED)
  {
    // We are just starting playback
    sample_num = 0
  }
  if(sample_num < N_SAMPLES)
  {
    // There are more values, use them
    LEFT_DRIVE_MOTOR = left_side[sample_num]
    RIGHT_DRIVE_MOTOR = right_side[sample_num]
  }
  else
  {
    // No more values, hold still
    LEFT_DRIVE_MOTOR = 127
    RIGHT_DRIVE_MOTOR = 127
  }
  sample_num++
}
// Store the previous record state
prev_record_button = record_button;
// Store the previous playback state
prev_playback_button = playback_button;

One other thing that you will want to keep in mind is that 90 samples is equivalent to 90*40ms = 3.6 seconds. You will need to adjust the number of samples to get a good sampling time.

[Andrew Blair]

Wow. Christmas came early this year. Thanks so much Dave! Now all I have to do is fill in the blanks, then on to EEPROM!

By the way, I ran a printf on my increment variable, and for some reason it was not incrementing....I think thats my problem in the original program. I don't seem to have any luck with for loops.....