What am doing wrong here with my class?

Yes, but the point of classes is they have objects (instances of that class).

This is purely to avoid naming clutter is it? You might want to consider using a namespace.

I'd never heard of namespaces. That looks like the sort of thing I was looking for, but I gotta say, C++ seems pretty ridiculous to me. You've got structs, classes, and now namespaces, and they all seem to (mostly) duplicate the functionality of one another.

Why do you want to make a class?

Well, to reduce clutter like you said. Though I had also planned to make some of the variables private and only accessible to the functions in the class. Which it seems I can't do with name spaces. But I guess that doesn't really matter, I mean I'm not making a huge application here and I'm not worried about accessing something I shouldn't somewhere I shouldn't. I was just going to do that because it's good practice.

So I guess I'll use a namespace instead and forget the whole private data thing.

So with namespaces I don't have to redeclare those variables in the namespace outside of it eh? Or use static?

I'm getting another error now though:

namespace LED {
   
    static int MODULES;   // Number of LED modules connected.  Defined in config.txt.
  
    static byte * data;    // LED data is stored as a string of 12 bit LED values.  There are 3 bytes for every two LEDs.  We store it this way because the data does not need to be altered when we are shifting it out to the TLC5947s.  Modifying the data with bit shifts would be very expensive and there is a lot of data to move.
    static byte * scale;   // Scaling factor for LED brightness.  Used when say, you want to be able to adjust the brightness of the powercell without changing all the code that assumes the max desired brightness is 1.0.  Value stored here is scaling_factor*255, where scaling factor is 0..1.
 
    void set(int, float, float);  
    float get(int);
    
};

"Namespace LED {} redeclared as different type of symbol."

Eh, nevermind that error, it's because LED is defined in the arduino pin definitions. Not sure why that didn't throw an error before.

Okay, by uncommenting that timing code I put in there when I was trying to optimize my SPI function for outputting the LED data, I have verified that the function is indeed being called, but it is going through the LED loop so fast that the thisLED pointer must surely be being set wrong:

  if (time >= nextLEDFrameTime) { // If it is time for an LED update...  
  
    unsigned int time = micros();
      
    thisLED = &led::data[led::MODULES*36]; // The first operation is to decrement this pointer, so the first time we write thisLED it will be array index LEDMODULES*36-1
    lastLED = &led::data[0]; 
   
    cli(); // Halt interrupts.
        
    do {       
    
      SPDR = *--thisLED; WAIT;            
      //while (!(SPSR & _BV(SPIF))); SPDR = *thisLED--; // Wait for transfer of byte over SPI bus to complete, then transfer *thisLED into SPDR register, and decrement address of *thisLED. 
          
    } while (thisLED != lastLED); // thisLED is decremented one last time before we hit the end of the loop, so after byte 0 transfers, the loop exits.

    WAIT; // Wait for last byte to finish transfer. 
    SPSR = SPSR & ~_BV(SPIF); // Clear transfer flag.
    
    //while (!(SPSR & _BV(SPIF))); // Wait for last byte to finish transfer.
    
    // 15 nops and:
    // 6x unroll = 276us
    // 12x unroll = 256us
    // 24x unroll = 252us
    
    // This may be leaving the SPIF flag set.  
    
    // Clear SPIF flag:
    //while (!(SPSR & _BV(SPIF))); // Wait for last byte to finish transfer.

    // 340 microseconds
    // The trick which makes the above so fast is that we decrement the pointer and do the conditional jump while we are waiting for the previous byte transfer to complete.

    // Move data from shift register into latch register:
    
      LATPORT &= ~(1 << LATPIN); // Set LATCH pin low.  It is important to do this first, just before transitioning high!  I don't know why, but the LED modules won't work right otherwise!
      LATPORT |= (1 << LATPIN); // Set LATCH pin high.  Transitioning from low to high moves all data from the shift register into the latch registers. 

      ENAPORT |= (1 << ENAPIN); // Set BLANK pin high. Turn all outputs off, and reset grayscale timer.  Blanking reduces flicker, but it does not seem to matter if it is before, after, or in the middle of latching.
      ENAPORT &= ~(1 << ENAPIN); // Set BLANK pin low. Turn all outputs on.
      
    sei(); // Reenable interrupts.
    
    time = micros() - time; 
    Serial1.println(time,DEC); 
    
    nextLEDFrameTime = time + LEDFrameTime; // Set the time of the next update.
    
  }
}

I have also just confirmed that led::MODULES contains the right value.

There has to be something wrong here:

 byte *thisLED;
  byte *lastLED;

  if (time >= nextLEDFrameTime) { // If it is time for an LED update...  
  
    unsigned int time = micros();
      
    thisLED = &led::data[led::MODULES*36]; // The first operation is to decrement this pointer, so the first time we write thisLED it will be array index LEDMODULES*36-1
    lastLED = &led::data[0];

Something like this:

namespace myLeds {
   
    int modules;   // Number of LED modules connected.  Defined in config.txt.
  
    byte * data;    
    byte * scale;   
 
    void set(const int which, const float a, const float b)
      {
      // whatever
      }
      
    float get(const int which)
      {
      return 42.0;
      }
    
};  // end of namespace myLeds

void setup ()
  {
  myLeds::modules = 2;

  myLeds::data =  (byte *) malloc (myLeds::modules * 36 * sizeof (byte));  
  myLeds::scale = (byte *) malloc (myLeds::modules * 24 * sizeof (byte));
  }  // end of setup
  
void loop ()
  {
  myLeds::set (1, 2, 3);
  }  // end of loop

This has reduced namespace clutter because the only "global" name now is myLeds, and the other functions and data need to be qualified to get at them.

That's all it has done, but it does mean that if you happen to use "data" or "scale" elsewhere (not qualified) then they don't get confused with the ones in the namespace.

Strictly speaking I would have MODULES as modules, because all caps is for constants (another reason not to use the name LED).

A thing, like modules, that you are planning to change, is clearly not a constant. Unless perhaps it is just something you change at compile time.

It was a constant at one point, and it still is, but eventually it will be loaded from a file, so yeah I will change that.

namespaces don't have semicolons at the end; might want to fix that (although it still compiles)

This compiles. It appears to work:

#include <SPI.h>

namespace myLeds 
    {
   
    const int LEDBYTES = 36;
    const int SCALEBYTES = 24;
    
    int modules;   // Number of LED modules connected.  Defined in config.txt.
  
    byte * data;    // LED data is stored as a string of 12 bit LED values.  There are 3 bytes for every two LEDs.  We store it this way because the data does not need to be altered when we are shifting it out to the TLC5947s.  Modifying the data with bit shifts would be very expensive and there is a lot of data to move.
    byte * scale;   // Scaling factor for LED brightness.  Used when say, you want to be able to adjust the brightness of the powercell without changing all the code that assumes the max desired brightness is 1.0.  Value stored here is scaling_factor*255, where scaling factor is 0..1.
 
    void set(const int which, const float a, const float b)
      {
      // whatever
      }
      
    float get(const int which)
      {
      return 42.0;
      }
    
    
}  // end of namespace myLeds

void setup ()
  {
  myLeds::modules = 2;

  myLeds::data =  (byte *) malloc (myLeds::modules * myLeds::LEDBYTES * sizeof(byte));  
  myLeds::scale = (byte *) malloc (myLeds::modules * myLeds::SCALEBYTES * sizeof(byte));
  
  SPI.begin ();
  Serial.begin (115200);
  
  }  // end of setup
  
void loop ()
  {
  myLeds::set (1, 2, 3);
 
  unsigned long startTime = micros();
      
  byte *thisLED = &myLeds::data [myLeds::modules * myLeds::LEDBYTES]; 
  byte *lastLED = &myLeds::data [0]; 
     
  noInterrupts ();
  
  do
    {       
    SPI.transfer (*--thisLED);
    } while (thisLED != lastLED);

  interrupts ();

  unsigned long endTime = micros();
  Serial.println (endTime - startTime);
  delay (1000);
  }  // end of loop

The serial monitor displays:

216
216
216
220
220
220
220
220
220

That was the predicted time (216 uS) for the fastest SPI loop. So not only does it work, it works nice and fast without mucking around with the SPI registers. Throw in one NOP if necessary if the LEDs require it.

(edit) Changed loop to post-test.

I just discovered that it was the NOP's which were the problem. Somehow switching to the use of a namespace has changed the timing so 10 NOP's is no longer sufficient. I threw 20 in there just to see if that was the issue (WAIT; WAIT;) , and sure enough the LEDs light up once more.

As for your suggestion Nick, I'm afraid there's one problem with your benchmark. The benchmarks I was doing before were for 6 LED modules, not 2. So that SPI function you're calling there is in fact 3x as slow as the theoretical max speed.

I guess I'll have to add or remove some NOPs and see what happens. I probably only need to add one. I had noticed with my earlier testing that removing just one would result in no LEDs lighting, and I could not figure out why that would be, when removing 2 resulted in garbage.

Well thanks for all your help. I guess I must have had the class stuff right in the first place. Ugh. :slight_smile:

Oh well at least I learned about namespaces.

Perhaps if you post all your code? All that namespaces do is change the compiler symbol table management.

All the code wouldn't fit into one post and it uses a modified wavehc and sd card reader lib and 1284 pin definitions and serial output on uart1 with a special bootloader... I don't think you'd want to go to all the trouble to compile it. :slight_smile: Anyway I'm gonna post something in the optimization thread for this spi stuff, I just came across something interesting.

You can make attachments you know. Including .zip files. Click on the Additional Options button below.

I don't think you'd want to go to all the trouble to compile it. :slight_smile:

I've gone to the trouble of writing, compiling and testing the code above with a view to helping you.

Okay, if you really want to take a look at it, here's the code:
http://mightymicrocontroller.com/backups/Mightycode.zip

The board is a 1284p with an SD card on usart0, DAC and TLC5947s on SPI, and serial upload is done via USART1 using a special bootloader. I use the avr-developers variant.
(Crappy setup I know... too late to fix it though.)

The wavehc lib has been modified to use hardware SPI, and the sd lib that comes with it has been modfied to use usart0.

I've got a video of the thing in action too if you want to see what the code is doing. This was with the old slow version of the LED SPI function though:

scswift:
I just discovered that it was the NOP's which were the problem. Somehow switching to the use of a namespace has changed the timing so 10 NOP's is no longer sufficient.

I just tested the code with the NOPs. You are right, it's faster (84 uS). I was wrong about the SPI.transfer.

However looking at the generated code you still got 11 NOPs. The namespace didn't change that at all.

However looking at the generated code you still got 11 NOPs. The namespace didn't change that at all.

I don't understand. The generated code for which version?

With the code just using standard global arrays, 10 NOPs was sufficient. When I added the namespace, 10 stopped working (no leds lit) but 11 works.

I tried adding an extra WAIT before and after the loop as well, but 10 still does not work. Nor does 9.