the

Code:

#define RESET SS

at the top of the sketch refers to a definition in pins_arduino.h of

Code:

const static uint8_t SS   = 10;

If you change the SS in that define to whatever pin you want it should work .

I guess that when u say your motors are connected to the pins, you mean that a high-current switch is connected to the output pins... either a relay, or a transister or perhaps a motor controller circuit.
Because a motor needs a LOT more power than the arduino can output.

==
Assuming you have a switch that can be turned on by the small current that the arduino outputs..
==

Your code always only read one character per call of loop() - as the Arduino is so fast compared to the the serial line.

But you are tring to read twice... and checking the first character against 'R' or 'r' and the second character against 'L' or 'l'.

You should really only read one character at a time - and check it against R, r, L and 'l'.

The HardwareSerial implementation (.cpp) does use the RX interrupt, and puts the received data in a ring buffer for the sketch to read later...

Even if you modified the HardwareSerial implementation to call one of your functions when it copied '>' into the ring buffer (which is not hard to do, but does create a mess that is hard to look after as new versions of the Arduino software are released) there is still the problem that you are using humungous delay()s in your code.

If you delay(10000) in your code, then the sketch can not respond sensibly to the command for 10 seconds!

If you really want to keep the structure of the code relatively unchanged you need to do something like

Code:

static char cmd_i = 0 ;
define CMD_L 10
static char cmd[CMD_L] ;

void
checkSerial(void)
{
    if (Serial.available())
    {
        char ch ;
        if (cmd_i >= CMD_L)
            cmd_i = 0 ;
        cmd[cmd_i++] = ch = Serial.read() ;
        if (ch == '>')
            checkCommand() ;
    }
}

static bool abortDelay = false ;
void
my_delay(int n)
{
   abortDelay = false ;
   long end = millis() + n ;
   while (end > millis())
   {
       checkSerial() ;
       if (abortDelay)
           return ;
   }
}

then call my_delay() rather than the normal one.

electronicstudent

if you search for "avr interrupt" in google, the top page of links look quite good.

Hi electronicsstudent..

Your code will compile in your simulator environment if you added an include as the first line.

Code:

#include "WProgram.h"

This is added by the arduino environment before the compiler is called.

The WProgram.h file is in the arduino/cores/arduino directory of the distribution.

It in turn includes the <avr/interrupt.h> header that defines the ISR() macro.

It also defines the functions from the arduino core.

good luck

Then there is VIM.  Which has many good features .

www.vim.org

This thread talks about where the compilation directory is..

http://arduino.cc/forum/index.php/topic,46733.0.html

Good to hear the code is running faster .

The hardware serial device has two bytes of buffer inside it for writing..

That means you can write 2 characters with no blocking (if the serial device is idle).

At 115200 baud, each character is taking about 100 microseconds to send, so sending 4 will block for 200 micro-seconds before returning.

Or you can send 2 characters each 200 microseconds without blocking.

To call newPinData you need to write

Code:

newPinData()

.

Code:

newPinData

refers to the address of the function.

[easier]

One new simple change...

Code:

     static double sectionDivider = 1;                                                   // Section length randomly varies between /1 /2 and /4. This value signifies the number of bits to shift the values by to accomplish that. [0..2]
      static word sectionIndex = 0;                                                       // Incremented as we step through each section.
      static byte section;                                                                // The section currently being rendered.

Code:

       sectionIndex++; // Increment the number of samples we have rendered in this section.
        if (sectionIndex >= (sectionLength[section]*sectionDivider)) { // If we have rendered all the samples needed for this section, move on to the next section.
          sectionIndex = 0;
          section++;

Is doing a float (double is not implemented on AVR-GCC) multiply each time through the loop. That is a couple of hundred cycles.

You could count down from  sectionLength [ section ] *sectionDivider to 0.

=======
The comment about restructuring is a bit of a guess. I thought the outer function might look something like this

Code:

     const word sectionLength[4] = {256, 128, 256, 384};                                 // The number of samples in each of the four sections which make up the sound effect.  (5 sines, 5 sines w/ noise, 5 sines, 2 sines)
      static double sectionDivider = 1;                                                   // Section length randomly varies between /1 /2 and /4. This value signifies the number of bits to shift the values by to accomplish that. [0..2]
      static byte pulseCount;                                                             // A pulse is a group of 5 sections.  We count how many have been rendered so we can change the timing between pulses every second or so.


      calcSection02(sectionLength[0]*sectionDivider) ;
      calcSection1(sectionLength[1]*sectionDivider) ;
      calcSection02(sectionLength[2]*sectionDivider) ;
      calcSection3(sectionLength[3]*sectionDivider) ;

      pulseCount++; // Keep track of the number of pulses (chord pairs) we have rendered at this speed.
            
      if (pulseCount >= 32) {
          pulseCount = 0;        
          // At a 31250hz sample rate, and standard pulses being 1024 samples long, there are around 30 pulses per second.
          // If we want to change the pulse rate every second or so, (as defined by sectionLength[section]>>sectionDivider) then changing the rate every 32 pulses will do the job.
          // Since sectionDivider allows us to speed up the pulses, if we want to wait for the same period of time before changing speeds, we must remember to multiply the desired pulseCount by sectionDivider,
          // or divide pulseCount by sectionDivider before comparing it.
              
          // Random() is a slow function but since we're only using it once in a while, it might be okay.  
          switch ((int)(pkeSpeed*3)) { // With an (int) conversion, fractional portions are discarded, so everything except pkeSpeed = 1.0 is mapped to 0, 1, or 2.
              
             case 0: // Select from any of the three speeds at the "slowest" (quietest, lowest pitch) setting.
                  // sectionDivider = 1;
                sectionDivider = 0.75 + random(0, 10)*0.04;
                break;
                
             case 1: // At "medium" setting, select from the two fastest speeds at random.
                sectionDivider = 0.75 + random(0, 10)*0.02;
                break;
              
             case 2: // At the "highest" setting, use only the fastest speed.
             case 3: // Only when pkeSpeed = 1.0 will the result be 3.
                sectionDivider = 0.75 + random(0, 10)*0.01;
                break;
          }
      }

and the calcSection02() etc would then count down and contain only the code for their part.

I am not sure if this works out as I can not see the surrounds of tour code.

===

The output of avr-objdump is your code intermingled with the assembler.
I agree the address and stack maniplulations are a pain to follow but the code as comments makes it easier to follow.

So my comments in 2nd reply (written b4 i read your reply) seem to be ok:)

For tools, you can see an assembler listing of the code.

If you search the forums for avr-objdump i think you will find instructions (hopefiully for your OS and arduino version / environment).

Then each byte of op-code is a clock, and a few instructions (like the 8 bit multiply variants) are 2, and the obscure flash reading ones etc.. you need the databook for.

It is easy to count up where the time goes.

-- i am off for the day.

Good luck again.

You calculate the phase[0...9] as 16 bit numbers but only use one byte in the code in the inner loop ..

Code:

phase[x]>>8

You would make the code easier to execute (so faster) if you stored the bit of it you need to use.
ie calculate

Code:

highPhase[i] = phase[i]>>8

outside the loop.

I cannot see why sample should be a long
It contains the sum of upto 6 byte values, divided and shifted to be 0 centered, then multiplied by a byte volume.
It should be a signed 16 bit int...
This code would be 2 to 4x quicker to execute

Code:

      // Adjust volume of sample.
        sample = (sample * volume) >> 6; // Divide by lower value to maximize volume, which on average is around 64, not 256.

Code:

           sample = sample + sintable[phase[3]>>8];
 

might be 2x faster

Similarly
   

Code:

volume = ((long) pkeVolumeFixed * sine) >> 8; // [0..255]

is doing a 32bit multiply, when a 16 bit result would do I think.
If the compiler generates a call to the 32 bit multiply this is costly > 32 clocks.
A 32 bit multiply is 4x slower than a sixteen bit multiply which is 4x slower than a 8 bit multiply which is 2 clocks.

-

The only other oportunity in the code for improvement that I can see is the logic around the sectionIndex, section number and the switch(section).

If you could pull this logic out of the sample loop, and have three functions one for section02(), section1() and section2() you might get a speed up.

This does depend on the rest of the codes structure.

-

Is there a simulator environment in which you can profile the execution of the code?

Professionally, i refuse to optimise without measurement.

in this play environment it is fun almost because the measurement tools seem to be missing, but this is a lot of code to get running fast and correctly - and profile tools would help a lot.

good luck.

A lot of code...

How are you compiling it?

Frome what I read - to improve by 4x requires an algorithm change,
or a simplicifation in the inner loop.

It would help you think about the time spent in the code if you calculated the number of times each line of code is executed..

removed bug comment as not sure anymore...

Two more comments..
Code like the your setupTimers() requires me to read the datasheet to understand.
For some reason I can not be bothered.
Perhaps, because when I write code that contains obscure special values in combinations, I add comments about what the values are intended to achieve, and on what page of the datasheet I can confirm the correctness of my assumptions.
(ok - no I dont record the references to the datasheet page but I should)

So - I have no idea what the timer is set up to do...
I guess you do, but can not validate it.  Neither can anyone else.

Code:

void setupTimers()
{
  TCCR1A = 0;
  TCCR1B = 0;

  TCCR1B = _BV(CS10);
  TIMSK1 = _BV(OCIE1A) | _BV(TOIE1);
 
  OCR1AH = 255;
  OCR1AL = 255;
 
  TIFR1 = 1;
}

And the second comment is, I missed the indirection by pwmOrder in all the code the ISR().

You could reorder the data outside the ISR() so that the access order is the native order.
The ISR() code would be about 2 x faster for that, especially as the array pwmOrder is declared volatile.

Actually, I think that the code has WAY too many 'volatile' variables for reasonable speed.

You only refer to the non temp arrays in the ISR I see.
So they do not need to be volatile.

Also, the temp arrays are read by the ISR and are built in main line with interrupts disabled.
Except that they are not.. the tempCompValL[pin] and tempCompValH[pin] are changed while interrupts are disabled.. so the ISR() could be copying them before the sort order is updated. This is a bug that should trigger relatively rarely and be hard to find when you are looking for it.  It would NOT cause the errors you report.
In any case, them being volatile does not help.

Then beginIndex, tempOCH and tempOCL are only used in the ISR() - so should be local variables to the ISR and not volatile. (beginIndex should be a static local to the ISR()).

These changes are all simple multiples of the code execution time...
So the code will be perhaps 6 x quicker - 2x for removing the indirection by pwmOrder and 3x for removing the surplus volatiles.

The change to the search from the entire array to only incremental will make a difference based on the number of pins (9), and even more if you add more pins.


again - good luck.

I have an idea...  but since your code is quite complex and I am not a counter guru I might be wrong..

However, The code in the ISR is quite slow I think, as it searches for matches deeper in the arrays it will get slower and slower.

Until the time in the ISR exceeds the difference in the counts perhaps?

Code:

ISR(TIMER1_COMPA_vect) {

tempOCH = OCR1AH;
tempOCL = OCR1AL;

lo = TCNT1L;
hi = TCNT1H;

  for(byte i = beginIndex; i<aantalPwmPins; i++)
  {
    if(tempOCL == compValL[pwmOrder[i]] && tempOCH == compValH[pwmOrder[i]])
    {
      writeOffToPin(pwmPins[pwmOrder[i]]);

Could be faster done like this..

Code:

ISR(TIMER1_COMPA_vect) {

tempOCH = OCR1AH;
tempOCL = OCR1AL;

lo = TCNT1L;
hi = TCNT1H;

// use a static loop counter...
static byte i = 255 ;
if (i==255) i = beginIndex ;

  for(; i<aantalPwmPins; i++) // and start looking at the last match in the array
// most times should pass through the loop only once.

// the first time it will search on average half the table to find the point where it should be
// some times the first search will take so long that the next matching count entry is
// missed.. and hence the search will look through two table entries to get the right entry
// Should be ok after that.
  {
     if (tempOCH > compValH[pwmOrder[i]]) // current table value is in the past
         continue ; // look at next table entry
     if (tempOCH < compValH[pwmOrder[i]])
         return ; // can not match yet
     // not < or > so ==
     if(tempOCL > compValL[pwmOrder[i]]) // current table value in past
         continue; // look at next table entry
     if(tempOCL < compValL[pwmOrder[i]])
         return ; // can not match yet.
    // not < or > so ==
      writeOffToPin(pwmPins[pwmOrder[i]]);
      // more work

      return ; // leaving i set to the last matched location.
    }
    // reset i to 255 to restart search, get to here when searched full range of i in for loop.
    i = 255 ;
}

Actually what is intended at the end of the loop is not clear to me.. so I have probably broken the code.

But the intent, to only search incrementally through the sorted array is clear I hope.

Also, I split the comparison of H and L parts into two parts.  I have used this logic structure before, but am not sure of the definition of these two bytes, it is really a guess based on the names.

good luck.