What am doing wrong here with my class?

Can you define "no longer working"? Do you mean it crashes? What does it do compared to what you expect?

Your usage of the class is, ah, different to how classes are normally used.

I don't understand why some, if not all, of the procedural code for the class is done outside it, eg. here:

Inside setup():

  // Configure LEDs:

LED::MODULES = 2; // Eventually this will be defined in config.txt.

LED::data = (byte ) malloc(LED::MODULES36 * sizeof(byte));
     LED::scale = (byte ) malloc(LED::MODULES24 * sizeof(byte));
   
     for (int x = 0; x < (LED::MODULES * 36); x++) { LED::data[x] = 0; }
     for (int x = 0; x < (LED::MODULES * 24); x++) { LED::scale[x] = 255; }  // Set LED brightness scale factor to default, then adjust only those which are defined in config.txt.

Since the class only has static members there doesn't seem to be any point in having a class.

And I'm still wondering why new doesn't seem to work. I see you had to recreate it yourself to get the code you wrote to compile.

They omitted new and delete from the earlier distributions. It is in the current one but not new[] and delete[] so I had to create them, yes. I think I have a bug report in about that.

It may not matter too much in your case, but when working with class objects you should use new and not malloc. In your case the only thing you are doing new on is a byte* so it won't make a lot of difference.

Can you define "no longer working"? Do you mean it crashes? What does it do compared to what you expect.

The code seems to still run (My sound is working, and I can push a button and trigger something on a port) but my LEDs no longer light up. Since I know the function to update the LEDs is being called and I'm not getting flickering, that means the modules are being sent a stream of 0's. If they were being sent garbage they would flicker. And if they were being sent nothing, they would flicker as well because the sound update sends data down that bus and if I hook up more led modules than data is sent each update then on the remaining modules I get garbage.

So for some reason this code is not working:

    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.

Your usage of the class is, ah, different to how classes are normally used.
Since the class only has static members there doesn't seem to be any point in having a class.

I figured using a class to have:
LED::set()
LED::get()
SERVO::set()
SERVO::get()

And having those encapsulated in their own class with their own private static data (the data array is only public at the moment because I haven't finished moving the other LED functions that actually set that up into the class), was better than having a bunch of global variables and just naming my functions:

LEDset()
SERVOget()

Also I figured it was about time that I learned C++ class syntax since for years I avoided OOP and stuck with C, but I started using it quite heavily once I learned Blitzmax which I understand has a lot of similarities to C++ (and a lot of differences).

It wouldn't make sense to say create an array of LED objects, when all an LED is a brightness and a scaling factor and I need to be able to blast the data out to the led modules as fast as possible so the data has to be kept in a packed array of 12 bit numbers.

I don't understand why some, if not all, of the procedural code for the class is done outside it, eg. here:

I put that code outside the class because the tutorials I found online said that is what you need to do if you want to set static variables in a class to a default value. Since I am not creating any LED objects, how else would that code get called? I understand there is a function that gets called when you create the first object but I'm not creating any.

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.