SPI doesn't work in my library

Hello, new here.

I’m trying to develop a library for driving the MAX7219/MAX7221 7-segment LED driver IC’s. My code works flawlessly when I place everything within the main sketch file, but as soon as I try and turn that code into its own class and put it in a library, it doesn’t work. (Interestingly, though, it does work when the code is in an external library as long as the code is entirely procedural, no classes involved.) It seems as soon as I create a class, the SPI transfers no longer execute. The debug Serial statements in my code lets me know everything else is working correctly. It’s as if it’s just skipping over the SPI.transfer() statements, refusing to perform them, because the MAX7221 IC isn’t doing anything.

It doesn’t matter whether I put the SPI initialization commands in the main program or the library class. Either way, it doesn’t work.

I’ve posted my code below. I want to use the SPI library if at all possible, since it uses the Atmega’s hardware SPI capabilities. I’ve seen other libraries just bit-banging the SPI transfers, but I want my code to run as fast as possible. As far as I can tell, the only reason my code doesn’t work is because the SPI.transfer() calls are inside a class. They work when they’re not inside a class. What have I done wrong?

led_test.ino:

#include <SPI.h>
#include "MaxLed.h"

#define DEBUG

int state = 0;
MaxLed led(10);

void setup() {
  #ifdef DEBUG
  Serial.begin(9600);
  #endif
  
  SPI.begin();
  SPI.setBitOrder(MSBFIRST);
  SPI.setDataMode(SPI_MODE3);
  delay(1);
  
  led.initialize();
  
  #ifdef DEBUG
  Serial.println("Setup complete");
  #endif
}

void loop() {
  
  if (state == 0) {
    led.print(" HELLO  ");
    #ifdef DEBUG
    Serial.println("Hello");
    #endif
    state = 1;
  } else {
    led.print(" Wworld ");
    #ifdef DEBUG
    Serial.println("World");
    #endif
    state = 0;
  }
  
  delay(1000);
  
}

MaxLed.h:

#ifndef MAXLED_H
#define MAXLED_H

#include "Arduino.h"

#define MAXLED_CHARS_PER_CHIP 8

// Register name definitions
#define MAXLED_RNO 0x00
#define MAXLED_RD0 0x01
#define MAXLED_RD1 0x02
#define MAXLED_RD2 0x03
#define MAXLED_RD3 0x04
#define MAXLED_RD4 0x05
#define MAXLED_RD5 0x06
#define MAXLED_RD6 0x07
#define MAXLED_RD7 0x08
#define MAXLED_RDM 0x09
#define MAXLED_RIN 0x0A
#define MAXLED_RSL 0x0B
#define MAXLED_RSD 0x0C
#define MAXLED_RDT 0x0F

// Register value definitions
#define MAXLED_NO_DECODE 0x00
#define MAXLED_CODE_B 0x01
#define MAXLED_SHUTDOWN 0x00
#define MAXLED_ACTIVE 0x01
#define MAXLED_NORMAL_OP 0x00
#define MAXLED_DISPLAY_TEST 0x01

class MaxLed {
    public:
        MaxLed(uint8_t cs);
        MaxLed(uint8_t cs, unsigned int chipCount);
        void initialize();
        void displayCharAt(const char c, const unsigned int pos);
        void refreshScreen();
        void clear();
        void print(const char * str);
        void setDP(unsigned int pos);
        void clearDP(unsigned int pos);
        void toggleDP(unsigned int pos);
    private:
        static const byte DEFAULT_REGISTER_VALUES[];
        static const byte ASCII_FONT[];
        uint8_t _cs;
        unsigned int _chipCount;
        char * _screenBuffer;
        const char lower7copy(char * const destination, const char source);
        char * lower7strncpy(char * destination, const char * source, size_t num);
        byte transfer(byte _data);
};

#endif

MaxLed.cpp:

#include "MaxLed.h"
#include "Arduino.h"
#include <SPI.h>

const byte MaxLed::DEFAULT_REGISTER_VALUES[] = {
    0x00,
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    0x00, 0x0F, 0x07, 0x01};
const byte MaxLed::ASCII_FONT[] = {
    0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 0-7
    0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 8-15
    0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 16-23
    0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 24-31
    0x00, 0x65, 0x22, 0x3F, 0x65, 0x65, 0x65, 0x02, // ASCII (Space) ! " # $ % & '
    0x4E, 0x78, 0x63, 0x65, 0x18, 0x01, 0x65, 0x25, // ( ) * (converted to degrees symbol) + , - . /
    0x7E, 0x30, 0x6D, 0x79, 0x33, 0x5B, 0x5F, 0x70, // 0 1 2 3 4 5 6 7
    0x7F, 0x7B, 0x65, 0x65, 0x65, 0x09, 0x65, 0x65, // 8 9 : ; < = > ?
    0x6E, 0x77, 0x1F, 0x4E, 0x3D, 0x4F, 0x47, 0x5E, // @ A B C D E F G
    0x37, 0x30, 0x3C, 0x2F, 0x0E, 0x66, 0x15, 0x7E, // H I J K L M (first half) N O
    0x67, 0x73, 0x05, 0x5B, 0x0F, 0x3E, 0x0C, 0x1E, // P Q R S T U V W (first half)
    0x49, 0x3B, 0x6D, 0x4E, 0x13, 0x78, 0x62, 0x08, // X Y Z [ \ ] ^ _ 
    0x20, 0x77, 0x1F, 0x0D, 0x3D, 0x4F, 0x47, 0x5E, // ` a b c d e f g
    0x17, 0x10, 0x3C, 0x2F, 0x06, 0x72, 0x15, 0x1D, // h i j k l m (second half) n o
    0x67, 0x73, 0x05, 0x5B, 0x0F, 0x1C, 0x0C, 0x3C, // p q r s t u v w (second half)
    0x49, 0x3B, 0x6D, 0x4E, 0x06, 0x78, 0x65, 0x65}; // x y z { | } ~ (DEL)
                                                
MaxLed::MaxLed(uint8_t cs) {
    MaxLed(cs, 1);
}

MaxLed::MaxLed(uint8_t cs, unsigned int chipCount) {
    _cs = cs;
    _chipCount = chipCount;
    _screenBuffer = (char *) malloc(_chipCount * MAXLED_CHARS_PER_CHIP * sizeof(char));
    for (unsigned int i = 0; i < (_chipCount * MAXLED_CHARS_PER_CHIP); i++) {
        _screenBuffer[i] = 0;
    }
}

/* When calling initialize(), please ensure you have already
   initialized the hardware SPI port with MSBFIRST and SPI_MODE3. */
void MaxLed::initialize() {
    pinMode(_cs, OUTPUT);
    digitalWrite(_cs, HIGH);
    delay(1);
    
    for (byte i = 1; i <= 12; i++) {
        digitalWrite(_cs, LOW);
        SPI.transfer(i);
        SPI.transfer(DEFAULT_REGISTER_VALUES[i]);
        digitalWrite(_cs, HIGH);
        delay(1);
    }
    digitalWrite(_cs, LOW);
    SPI.transfer(MAXLED_RDT);
    SPI.transfer(MAXLED_NORMAL_OP);
    digitalWrite(_cs, HIGH);
    delay(1);
    
    Serial.println("Library initialized");
}

void MaxLed::displayCharAt(const char c, const unsigned int pos) {
    unsigned int chipNumber = pos / MAXLED_CHARS_PER_CHIP;
    byte chipRegister = (byte) ((pos % MAXLED_CHARS_PER_CHIP) + 1);
    byte fontData = ASCII_FONT[c & 0x7F] | (c & 0x80);
    digitalWrite(_cs, LOW);
    for (unsigned int n = 0; n < chipNumber; n++) {
        SPI.transfer(MAXLED_RNO);
        SPI.transfer(0x00);
    }
    SPI.transfer(chipRegister);
    SPI.transfer(fontData);
    for (unsigned int n = (chipNumber + 1); n < _chipCount; n++) {
        SPI.transfer(MAXLED_RNO);
        SPI.transfer(0x00);
    }
    digitalWrite(_cs, HIGH);
    delay(1);
}

void MaxLed::refreshScreen() {
    for (unsigned int pos = 0; pos < (_chipCount * MAXLED_CHARS_PER_CHIP); pos++) {
        displayCharAt(_screenBuffer[pos], pos);
    }
}

void MaxLed::clear() {
    for (unsigned int pos = 0; pos < (_chipCount * MAXLED_CHARS_PER_CHIP); pos++) {
        _screenBuffer[pos] = 0;
    }
    refreshScreen();
}

void MaxLed::print(const char * str) {
    lower7strncpy(_screenBuffer, str, _chipCount * MAXLED_CHARS_PER_CHIP);
    for (unsigned int pos = strlen(str); pos < (_chipCount * MAXLED_CHARS_PER_CHIP); pos++) {
        _screenBuffer[pos] &= 0x80;
    }
    refreshScreen();
}

void MaxLed::setDP(unsigned int pos) {
    _screenBuffer[pos] |= 0x80;
    displayCharAt(_screenBuffer[pos], pos);
}

void MaxLed::clearDP(unsigned int pos) {
    _screenBuffer[pos] &= 0x7F;
    displayCharAt(_screenBuffer[pos], pos);
}

void MaxLed::toggleDP(unsigned int pos) {
    _screenBuffer[pos] ^= 0x80;
    displayCharAt(_screenBuffer[pos], pos);
}

const char MaxLed::lower7copy(char * const destination, const char source) {
    *destination = (*destination & 0x80) | (source & 0x7F);
    return source;
}

char * MaxLed::lower7strncpy(char * destination, const char * source, size_t num) {
    char * temp = destination;
    while ((num-- > 0) && (lower7copy(destination++, *source++) != (char) 0))
        ;
    return temp;
}

As a matter of interest, let me post some code that DOES work. I converted the previously posted code from object oriented to entirely procedural, no classes involved. The following code works flawlessly, and the messages are being displayed on my LED display. Can anyone help me understand why this works but the previous code doesn’t?

led_test.ino:

#include <SPI.h>
#include "MaxLed.h"

#define DEBUG

int state = 0;

void setup() {
  #ifdef DEBUG
  Serial.begin(9600);
  #endif
  
  SPI.begin();
  SPI.setBitOrder(MSBFIRST);
  SPI.setDataMode(SPI_MODE3);
  delay(1);
  
  MaxLed::initialize(10);
  
  #ifdef DEBUG
  Serial.println("Setup complete");
  #endif
}

void loop() {
  
  if (state == 0) {
    MaxLed::print(" HELLO  ");
    #ifdef DEBUG
    Serial.println("Hello");
    #endif
    state = 1;
  } else {
    MaxLed::print(" Wworld ");
    #ifdef DEBUG
    Serial.println("World");
    #endif
    state = 0;
  }
  
  delay(1000);
  
}

MaxLed.h:

#ifndef MAXLED_H
#define MAXLED_H

#include "Arduino.h"

#define MAXLED_CHARS_PER_CHIP 8

// Register name definitions
#define MAXLED_RNO 0x00
#define MAXLED_RD0 0x01
#define MAXLED_RD1 0x02
#define MAXLED_RD2 0x03
#define MAXLED_RD3 0x04
#define MAXLED_RD4 0x05
#define MAXLED_RD5 0x06
#define MAXLED_RD6 0x07
#define MAXLED_RD7 0x08
#define MAXLED_RDM 0x09
#define MAXLED_RIN 0x0A
#define MAXLED_RSL 0x0B
#define MAXLED_RSD 0x0C
#define MAXLED_RDT 0x0F

// Register value definitions
#define MAXLED_NO_DECODE 0x00
#define MAXLED_CODE_B 0x01
#define MAXLED_SHUTDOWN 0x00
#define MAXLED_ACTIVE 0x01
#define MAXLED_NORMAL_OP 0x00
#define MAXLED_DISPLAY_TEST 0x01

namespace MaxLed {
    void initialize(uint8_t cs);
    void initialize(uint8_t cs, unsigned int chipCount);
    void displayCharAt(const char c, const unsigned int pos);
    void refreshScreen();
    void clear();
    void print(const char * str);
    void setDP(unsigned int pos);
    void clearDP(unsigned int pos);
    void toggleDP(unsigned int pos);
    
    static uint8_t _cs;
    static unsigned int _chipCount;
    static char * _screenBuffer;
    const char lower7copy(char * const destination, const char source);
    char * lower7strncpy(char * destination, const char * source, size_t num);
    byte transfer(byte _data);
    
    static const byte DEFAULT_REGISTER_VALUES[] = {
        0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x0F, 0x07, 0x01};
    static const byte ASCII_FONT[] = {
        0x00, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 0-7
        0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 8-15
        0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 16-23
        0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, // Control characters 24-31
        0x00, 0x65, 0x22, 0x3F, 0x65, 0x65, 0x65, 0x02, // ASCII (Space) ! " # $ % & '
        0x4E, 0x78, 0x63, 0x65, 0x18, 0x01, 0x65, 0x25, // ( ) * (converted to degrees symbol) + , - . /
        0x7E, 0x30, 0x6D, 0x79, 0x33, 0x5B, 0x5F, 0x70, // 0 1 2 3 4 5 6 7
        0x7F, 0x7B, 0x65, 0x65, 0x65, 0x09, 0x65, 0x65, // 8 9 : ; < = > ?
        0x6E, 0x77, 0x1F, 0x4E, 0x3D, 0x4F, 0x47, 0x5E, // @ A B C D E F G
        0x37, 0x30, 0x3C, 0x2F, 0x0E, 0x66, 0x15, 0x7E, // H I J K L M (first half) N O
        0x67, 0x73, 0x05, 0x5B, 0x0F, 0x3E, 0x0C, 0x1E, // P Q R S T U V W (first half)
        0x49, 0x3B, 0x6D, 0x4E, 0x13, 0x78, 0x62, 0x08, // X Y Z [ \ ] ^ _ 
        0x20, 0x77, 0x1F, 0x0D, 0x3D, 0x4F, 0x47, 0x5E, // ` a b c d e f g
        0x17, 0x10, 0x3C, 0x2F, 0x06, 0x72, 0x15, 0x1D, // h i j k l m (second half) n o
        0x67, 0x73, 0x05, 0x5B, 0x0F, 0x1C, 0x0C, 0x3C, // p q r s t u v w (second half)
        0x49, 0x3B, 0x6D, 0x4E, 0x06, 0x78, 0x65, 0x65}; // x y z { | } ~ (DEL)
};

#endif

MaxLed.cpp:

#include "MaxLed.h"
#include "Arduino.h"
#include <SPI.h>

void MaxLed::initialize(uint8_t cs) {
    initialize(cs, 1);
}

/* When calling initialize(), please ensure you have already
   initialized the hardware SPI port with MSBFIRST and SPI_MODE3. */
void MaxLed::initialize(uint8_t cs, unsigned int chipCount) {
    _cs = cs;
    _chipCount = chipCount;
    _screenBuffer = (char *) malloc(_chipCount * MAXLED_CHARS_PER_CHIP * sizeof(char));
    for (unsigned int i = 0; i < (_chipCount * MAXLED_CHARS_PER_CHIP); i++) {
        _screenBuffer[i] = 0;
    }
    pinMode(_cs, OUTPUT);
    digitalWrite(_cs, HIGH);
    delay(1);
    
    for (byte i = 1; i <= 12; i++) {
        digitalWrite(_cs, LOW);
        SPI.transfer(i);
        SPI.transfer(DEFAULT_REGISTER_VALUES[i]);
        digitalWrite(_cs, HIGH);
        delay(1);
    }
    digitalWrite(_cs, LOW);
    SPI.transfer(MAXLED_RDT);
    SPI.transfer(MAXLED_NORMAL_OP);
    digitalWrite(_cs, HIGH);
    delay(1);
    
    Serial.println("Library initialized");
}

void MaxLed::displayCharAt(const char c, const unsigned int pos) {
    unsigned int chipNumber = pos / MAXLED_CHARS_PER_CHIP;
    byte chipRegister = (byte) ((pos % MAXLED_CHARS_PER_CHIP) + 1);
    byte fontData = ASCII_FONT[c & 0x7F] | (c & 0x80);
    digitalWrite(_cs, LOW);
    for (unsigned int n = 0; n < chipNumber; n++) {
        SPI.transfer(MAXLED_RNO);
        SPI.transfer(0x00);
    }
    SPI.transfer(chipRegister);
    SPI.transfer(fontData);
    for (unsigned int n = (chipNumber + 1); n < _chipCount; n++) {
        SPI.transfer(MAXLED_RNO);
        SPI.transfer(0x00);
    }
    digitalWrite(_cs, HIGH);
    delay(1);
}

void MaxLed::refreshScreen() {
    for (unsigned int pos = 0; pos < (_chipCount * MAXLED_CHARS_PER_CHIP); pos++) {
        displayCharAt(_screenBuffer[pos], pos);
    }
}

void MaxLed::clear() {
    for (unsigned int pos = 0; pos < (_chipCount * MAXLED_CHARS_PER_CHIP); pos++) {
        _screenBuffer[pos] = 0;
    }
    refreshScreen();
}

void MaxLed::print(const char * str) {
    lower7strncpy(_screenBuffer, str, _chipCount * MAXLED_CHARS_PER_CHIP);
    for (unsigned int pos = strlen(str); pos < (_chipCount * MAXLED_CHARS_PER_CHIP); pos++) {
        _screenBuffer[pos] &= 0x80;
    }
    refreshScreen();
}

void MaxLed::setDP(unsigned int pos) {
    _screenBuffer[pos] |= 0x80;
    displayCharAt(_screenBuffer[pos], pos);
}

void MaxLed::clearDP(unsigned int pos) {
    _screenBuffer[pos] &= 0x7F;
    displayCharAt(_screenBuffer[pos], pos);
}

void MaxLed::toggleDP(unsigned int pos) {
    _screenBuffer[pos] ^= 0x80;
    displayCharAt(_screenBuffer[pos], pos);
}

const char MaxLed::lower7copy(char * const destination, const char source) {
    *destination = (*destination & 0x80) | (source & 0x7F);
    return source;
}

char * MaxLed::lower7strncpy(char * destination, const char * source, size_t num) {
    char * temp = destination;
    while ((num-- > 0) && (lower7copy(destination++, *source++) != (char) 0))
        ;
    return temp;
}

Well, I don't know what you're doing wrong, but your first post compiles perfectly fine for me.

It compiles for me too, however, it doesn’t run as expected. On the same exact hardware setup, the 1st displays nothing on the LED’s, and the 2nd displays “Hello” and “World” alternatively, as expected. The only difference is the SPI.transfer() commands are done from inside a class in the 1st.

Does it make any difference if you move the screen buffer setup (in particular, the malloc call) to the initialize function, instead of doing it in the constructor?

Yes, actually, I tried doing exactly that, and still the same results. Won't work when it's in a class.

I'm starting to think mine is an isolated case. I'm interested to know if any other programmers have successfully written a class-based library that uses the hardware SPI library?

gregallenwarner:
Yes, actually, I tried doing exactly that, and still the same results. Won't work when it's in a class.

I'm starting to think mine is an isolated case. I'm interested to know if any other programmers have successfully written a class-based library that uses the hardware SPI library?

Most of my libraries do exactly that.

One thing I noticed is that your first constructor definition is wrong:

MaxLed::MaxLed(uint8_t cs) {
    MaxLed(cs, 1);
}

You can’t define one constructor in terms of another like that. Most likely, if you call that constructor, it will create a temporary MaxLed and then destroy it, leaving the MaxLed you are trying to construct uninitialised. [There is a way of delegating one constructor to another in C++11, but that isn’t the syntax for it.] What you should do instead is define a single constructor, with a default parameter of 1 for the second parameter in the declaration in the .h file:

MaxLed(uint8_t cs, unsigned int chipCount = 1);

However, your example sketch doesn’t use the single-argument constructor, so this doesn’t help solve the current problem.

And this explains your problem, because it is the single-argument constructor you are using.

majenko:
Most of my libraries do exactly that.

Would you care if I viewed some example source code? Perhaps if I compare mine against yours I can discover something.

gregallenwarner:

majenko:
Most of my libraries do exactly that.

Would you care if I viewed some example source code? Perhaps if I compare mine against yours I can discover something.

http://sourceforge.net/p/wavepro/code/32/tree/trunk/arduino/WavePro/

See my edited post #7 for the fix.

PS - if you enable verbose mode in the IDE, you may find that there is a compiler warning about that constructor definition. I consider it highly irresponsible of the Arduino IDE authors to disable compiler warnings by default. But then I work in safety-critical software, where we take software bugs seriously.

dc42:
See my edited post #7 for the fix.

PS - if you enable verbose mode in the IDE, you may find that there is a compiler warning about that constructor definition. I consider it highly irresponsible of the Arduino IDE authors to disable compiler warnings by default. But then I work in safety-critical software, where we take software bugs seriously.

Ah, thank you very much! I'm a Java developer by trade, so sometimes I'm not so well informed on these kinds of things. I appreciate the help!