Pages: 1 [2] 3   Go Down
Author Topic: What am doing wrong here with my class?  (Read 1293 times)
0 Members and 1 Guest are viewing this topic.
Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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. 
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Why do you want to make a class?
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:

Code:
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."
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:

Code:
  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.
   
  }
}
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

There has to be something wrong here:
Code:
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];
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Something like this:

Code:
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.
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Offline Offline
Edison Member
*
Karma: 17
Posts: 1041
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

This compiles. It appears to work:

Code:
#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:

Code:
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.
« Last Edit: November 04, 2012, 05:11:23 pm by Nick Gammon » Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Oh well at least I learned about namespaces.
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 452
Posts: 18694
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Manchester, New Hampshire
Offline Offline
Edison Member
*
Karma: 1
Posts: 1283
Propmaker
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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. :-)  Anyway I'm gonna post something in the optimization thread for this spi stuff, I just came across something interesting.
Logged

Pages: 1 [2] 3   Go Up
Jump to: