need some hints and props

Comments

• Dave Hein Posts: 6,347

  2012-07-19 13:29 edited 2012-07-19 13:29

  It looks like the units of "given" is in micro-seconds since pausems divides CLKFREQ by one million, and then multiplies times "given".
• amossam Posts: 35

  2012-07-19 13:33 edited 2012-07-19 13:33

  yes, you are correct. i typed nano but I was thinking micro... sorry, my bad.
• ersmith Posts: 6,184

  2012-07-19 13:57 edited 2012-07-19 13:57

  There's one serious bug in your code: the line

           int cog = _start_cog_thread(cog_stack[0] + STACK_SIZE, rotate, 0, &thread_data);
  

  should read:

           int cog = _start_cog_thread(&cog_stack[0] + STACK_SIZE, rotate, 0, &thread_data);
  

  Traditionally one communicates with another COG via a "mailbox", which is just any kind of data area. A pointer to the mailbox is passed to the new COG thread. So your code would look something like:

  struct mbox {
    int direction;
    /* add anything else you like here */
  };
  
  volatile  struct mbox mailbox;  /* volatile is important, it says that multiple threads may change this */
  
  void rotate(volatile struct mbox *arg)
  {
     while(1)
       if (arg->direction == 1) {
        ...
        } else if (arg->direction == -1) {
        ...
        }
     }
  }
  
  int main(int argc, char *argv[]) {
  ...
  int cog = _start_cog_thread(&cog_stack[STACK_SIZE], rotate, &mailbox, &thread_data);
  ...
  }
  

  
  The global variable as you have it does work too, but it's a bit cleaner to isolate everything in the structure.
  
  Eric
• ersmith Posts: 6,184

  2012-07-19 13:59 edited 2012-07-19 13:59

  Oh, and as Dave already answered pausems takes microseconds.
  
  A slightly more general function to use is usleep(n) (which is in the C library, and also takes microseconds). It will work correctly even if you end up using pthreads to run the thread on the same COG as the main code, whereas pausems literally pauses the COG and prevents other threads from running on it.
  
  Eric
• amossam Posts: 35

  2012-07-19 14:18 edited 2012-07-19 14:18

  thx for your reply ersmith...
  
  i did your modifications, in function "rotate" and in "main"

  int main (int argc, char* argv[])
  
   {
  
   
      mailbox.direction = 0;
  
   
      int cog = _start_cog_thread(&cog_stack[0] + STACK_SIZE, rotate, &mailbox, &thread_data);
       mailbox.direction = 0;
   .....
  }
  

  
  
  but now i got this warnings when i compile it...

  propeller-elf-gcc -o a.out -Os -mlmm -I . -Wall -fno-exceptions -std=c99 optoc.c
  
   optoc.c: In function 'main':
  
   optoc.c:97:5: warning: passing argument 2 of '_start_cog_thread' from incompatible pointer type [enabled by default]
  
   /opt/Development/Applications/SimpleIDE/parallax/bin/../lib/gcc/propeller-elf/4.6.1/../../../../propeller-elf/include/sys/thread.h:112:5: note: expected 'void (*)(void *)' but argument is of type 'void (*)(volatile struct mbox *)'
  
   optoc.c:97:5: warning: passing argument 3 of '_start_cog_thread' discards 'volatile' qualifier from pointer target type [enabled by default]
  
   /opt/Development/Applications/SimpleIDE/parallax/bin/../lib/gcc/propeller-elf/4.6.1/../../../../propeller-elf/include/sys/thread.h:112:5: note: expected 'void *' but argument is of type 'volatile struct mbox *'
  
  

  
  and it doesn't work anymore?
  edit: it works, I had one small error in code... but, warnings are still there...
  
  thx...
  
  p.s. probably i'm missing something, but i don't know what... thx again...
• ersmith Posts: 6,184

  2012-07-19 17:36 edited 2012-07-19 17:36

  Ah, my mistake, the definition of _start_cog_thread expects a function declared as "void func(void *);" and not "void func(volatile void *);".
  
  You can fix that by moving the "volatile" into the definition of the structure:

  struct mbox {
    volatile int direction;
  };
  

  and removing the "volatile" from the definitions of struct mbox mailbox and rotate.
  
  The effect should still be the same -- the earlier version declared the entire mailbox structure to be volatile, this version declares just the "direction" field to be volatile and subject to change by other threads, but since that's the only field of the structure the generated code will end up the same.
  
  Eric
• amossam Posts: 35

  2012-07-20 14:57 edited 2012-07-20 14:57

  minor setback today... I connected 31V, 1.5A PS to driver board, with separate 5V supply (for electronic on driver board) from QuickStart board powered over laptop, and after few second something poped from driver board, familiar smell appeared and stepper stopped... It seems that L298N bridge died... i'm still unsure why... will take driver board tomorrow to a friend that will try to examine what was fried...
  
  @ersmith: i changed struct and removed volatile from function parameters, and i got rid of one warning message... but still i have this one:

    optoc.c:92:5: warning: passing argument 2 of '_start_cog_thread' from incompatible pointer type [enabled by default]
   /opt/Development/Applications/SimpleIDE/parallax/bin/../lib/gcc/propeller-elf/4.6.1/../../../../propeller-elf/include/sys/thread.h:112:5: note: expected 'void (*)(void *)' but argument is of type 'void (*)(struct mbox *)'
  

  
  it seems that compiles doesn't like '&message' variable in _start_cog_thread function...
  is it safe to ignore it?
  
  and another question to anyone willing to help... can following section be written better?
  

   int stepArray[] = {1, 0, 0, 0,
                     1, 0, 1, 0,
                     0, 0, 1, 0,
                     0, 1, 1, 0,
                     0, 1, 0, 0,
                     0, 1, 0, 1,
                     0, 0, 0, 1,
                      1, 0, 0, 1};  
  
  void setPinState(int pin, int state)
   {
       unsigned int mask = 1 << pin;
       DIRA |= mask;
       switch (state) {
           case 0 : OUTA &= ~mask; //pin set to low
                    break;
           case 1 : OUTA |= mask; //pin set to high
                    break;
       }
   }
   
  
   void setState(int p1, int p2, int p3, int p4, long int delay)
   {
       setPinState(pin[0], p1);
       setPinState(pin[1], p2);
       setPinState(pin[2], p3);
       setPinState(pin[3], p4);
       usleep(delay);
   }
   
  
   void singleHalfStep(int dir)
   { 
       setState(stepArray[ 0+0],stepArray[ 0+1],stepArray[ 0+2],stepArray[ 0+3], 400);
       setState(stepArray[ 4+0],stepArray[ 4+1],stepArray[ 4+2],stepArray[ 4+3], 400);
       setState(stepArray[ 8+0],stepArray[ 8+1],stepArray[ 8+2],stepArray[ 8+3], 400);
       setState(stepArray[12+0],stepArray[12+1],stepArray[12+2],stepArray[12+3], 400);
       setState(stepArray[16+0],stepArray[16+1],stepArray[16+2],stepArray[16+3], 400);
       setState(stepArray[20+0],stepArray[20+1],stepArray[20+2],stepArray[20+3], 400);
       setState(stepArray[24+0],stepArray[24+1],stepArray[24+2],stepArray[24+3], 400);
       setState(stepArray[28+0],stepArray[28+1],stepArray[28+2],stepArray[28+3], 400);
   }
  

  
  by better I mean with less data and code? and i still need to figure out how to reverse statements in sigleHalfStep function regarding 'dir' variable value.
  and would there be any speed improvements if i write functions for sending impulses to driver in assembler? or that would be too complicated compared to (possible) gain?
  
  thx
• ersmith Posts: 6,184

  2012-07-20 19:04 edited 2012-07-20 19:04

  Sorry to hear of your hardware troubles! I hope you can get going again soon.
  

  amossam wrote: »

  @ersmith: i changed struct and removed volatile from function parameters, and i got rid of one warning message... but still i have this one:

    optoc.c:92:5: warning: passing argument 2 of '_start_cog_thread' from incompatible pointer type [enabled by default]
   /opt/Development/Applications/SimpleIDE/parallax/bin/../lib/gcc/propeller-elf/4.6.1/../../../../propeller-elf/include/sys/thread.h:112:5: note: expected 'void (*)(void *)' but argument is of type 'void (*)(struct mbox *)'
  

  
  it seems that compiles doesn't like '&message' variable in _start_cog_thread function...
  is it safe to ignore it?

  Yes, it is safe to ignore it. void * is compatible with all pointers. If you want to get rid of the warning you can declare repeat as something like:

  void repeat(void *varg) {
    struct mbox *arg = varg;
    // now use arg; never bother with varg
  }
  

  and another question to anyone willing to help... can following section be written better?

  
  How about something like:

  inline void setPinState(int pin, int state)
   {
       unsigned int mask = 1 << pin;
       DIRA |= mask;
       switch (state) {
           case 0 : OUTA &= ~mask; //pin set to low
                    break;
           case 1 : OUTA |= mask; //pin set to high
                    break;
       }
   }
   
  
   void setState(int *p1, long int delay)
   {
       int i;
       for (i = 0; i < 4; i++)
           setPinState(pin[i], p1[i]);
       usleep(delay);
   }
   
  
   void singleHalfStep(int dir)
   { 
       int i;
  
       for (i = 0; i < 8; i++) {
           setState(&stepArray[i*4], 400);
       }
  }
  

  
  The compiler will decide if it's better to unroll the loops or not -- you can optimize for space with -Os and for speed with -O3.
  

  And would there be any speed improvements if i write functions for sending impulses to driver in assembler? or that would be too complicated compared to (possible) gain?

  
  It's unlikely that there would be many speed improvements. Are you finding that speed is an issue? Premature optimization is usually a bad idea -- if it ain't broke, don't fix it!
  
  Oh, and the other thing you may want to look at is how sensitive your loop is to timing. If you really need exact microsecond delays, the best bet is probably to use __builtin_propeller_waitcnt and keep track of the current cycle counter across calls to setState. usleep() and the pauseMs() function you were using earlier will both wait for N milliseconds, but that doesn't count the time spent in overhead and the time spent actually doing the step. If you want the impulses to actually arrive at exact intervals you have to take that time into account. For most applications it probably doesn't matter, but I thought I would mention it in case you do have timing requirements.
  
  Eric
• amossam Posts: 35

  2012-07-21 04:53 edited 2012-07-21 04:53

  so, if i understand correctly, you are passing pointer to array at specified index, and function takes that address as starting point of array?
  and why have you put inline in front "void setPinState"? thx
  
  speed is not an issue, but this is my first venture to microcontrollers, so I'm total newbie in this field. Also, if I put less than 330 ms in delay, stepper motor starts to vibrate but no spinning, so I'm not sure is it because of QuickStart board (some racing conditions in pins?) or maybe driver board...
  
  and timings in this part are not important.... because i'll use number of steps for rotation (in my case, 400 half steps for one revolution), not time of rotation...
  
  when i come home, i'll try these changes...
  
  thx
  
  btw., it turns out that i have another driver board at home! :-D
• ersmith Posts: 6,184

  2012-07-21 06:09 edited 2012-07-21 06:09

  amossam wrote: »

  so, if i understand correctly, you are passing pointer to array at specified index, and function takes that address as starting point of array?

  Right.

  and why have you put inline in front "void setPinState"? thx

  You can ignore that. It's a case of me forgetting my own advice about premature optimization -- the "inline" tells the compiler to expand calls to the function in-line for faster speed at the cost of more space. It actually makes sense to wait on that until you know which (speed or space) your application actually needs.
  
  Other things to improve the code:
  (1) The "switch" statement in setPinState is slightly inefficient, because there are actually three cases generated by the compiler: pin == 0, pin == 1, and pin == anything else (for which the switch does nothing).
  (2) There's no need to use up a whole "int" (32 bits) to hold the stepArray values. "char" (8 bits) would save some data space. Or, you could compress the pin states, for example putting 4 in each char by manipulating the bits, although that gets a bit more complicated (unless the pins are contiguous, see below)
  (3) If the pins are all together you could easily store the 4 bits together and set all of them at once via something like:

  void setFourPins(int lowestpin, int state)
  {
      unsigned int mask = 0xf << lowestpin;
      state = state << lowestpin;
      DIRA |= mask;
      OUTA &= ~mask;
      OUTA |= state;
  }