8 bit DAC - Code recommendations

Forum 2005-2010 (read only) Software Syntax & Programs
1

Greetings, I am fairly new to programming. I have an 8-bit SPI DAC with 4 DACs on the chip which I have written a library for. The code already works for my project, but I am looking for suggestions to improve the syntax of my code. (I can't get any better at programming if I don't know what to improve!)

I guess the only thing you need to know about this is that the DAC has 4 different converters on it, and when the code is all done, I should be easily be able to switch via the slave select to multiple 8bit DAC chips.

Without further adieu...

this is the main file. (this is just a test program I made to test the custom library. This is the first library I have ever made)

#include <SPIDAC.h>
#include <SPI.h>
  SPIDAC SPIDAC;
  
   int SLAVESELECT = 53;
void setup(){
SPIDAC.SPIDACinit(SLAVESELECT);
  Serial.begin(9600);
}

void loop(){

  int pedalin=analogRead(0)/4;

  SPIDAC.SPIDACset(SLAVESELECT,pedalin, pedalin, pedalin, pedalin);
..
..
..
(read functions to verify DAC functionality)
}

this is the h-file

#ifndef SPIDAC_h
#define SPIDAC_h

#include "WProgram.h"

class SPIDAC
{
public:
      int SLAVESELECT; 
      void SPIDACinit(int SLAVESELECT);
      void SPIDACset(int SLAVESELECT, int frontleft,int frontright,int rearleft,int rearright);
private: 
static const int DATAOUT = 51;
static const int SPICLOCK = 52;
};

#endif

this is the cpp

#include "WProgram.h"
#include "SPIDAC.h"
#include "SPI.h"

void SPIDAC::SPIDACinit(int SLAVESELECT) {

  SPCR = (1<<SPE)|(1<<MSTR);
  // enable SPI, set as master
  SPCR = (1<<CPHA);
  // =Samples taken on falling edge
  byte clr;
  clr=SPSR; //clear bad data
  clr=SPDR; //clear bad data
  delay(10); 
  // set the SLAVESELECT as an output:
  pinMode(SLAVESELECT, OUTPUT);
  pinMode(DATAOUT, OUTPUT);
  pinMode(SPICLOCK,OUTPUT);
  SPI.begin();

}


void SPIDAC::SPIDACset(int SLAVESELECT,int frontleft,int frontright,int rearleft,int rearright){

        for(int address=1;address<=4; address++){ 
 // take the SS pin high to begin data transmission:
           digitalWrite(SLAVESELECT,HIGH);
  //  send in the address and value via SPI:
        switch(address){

       case 1:
        SPDR = 0b00000001; //load second byte with the DAC address 0 and range 2
         while (!(SPSR & (1<<SPIF))); //wait to end magnitude transmission
       SPDR = frontleft; //load first byte with the magnitude
       break;
       
       case 2:
       SPDR = 0b00000011; //load second byte with the DAC address 1 and range 2
         while (!(SPSR & (1<<SPIF))); //wait to end magnitude transmission
       SPDR = frontright; //load first byte with the magnitude
      break;
      
    case 3:
         SPDR = 0b00000101; //load second byte with the DAC address 2 and range 2
         while (!(SPSR & (1<<SPIF))); //wait to end magnitude transmission
       SPDR = rearleft; //load first byte with the magnitude
       break;
       
    case 4: 
     SPDR = 0b00000111; //load second byte with the DAC address 3 and range 2
         while (!(SPSR & (1<<SPIF))); //wait to end magnitude transmission
       SPDR = rearright; //load first byte with the magnitude                        
       break; 
 
   }
 while (!(SPSR & (1<<SPIF))); //wait to end address byte transmission 
    digitalWrite(SLAVESELECT,LOW); 
      delayMicroseconds(3);
  }
 
}

I guess some suggestions on what i could use suggestions with is pointers. I think I could use those in here somewhere....
Are there any major syntax redflags? Can I simplify or generalize the code in anyway? Other suggestions?

I don't have much of a formal education on programming. You can assume if the code has poor form, it's because I don't know "the right way" to do it. All suggestions are welcome.

Also, idk if this is the wrong forum. I'd say this is a syntax improvement. not really an interfacing problem.

Thanks for reading. Thanks for helping me be a better programmer.

-Pearl

2

I really question the need for a loop here. You are looping 1 to 4 everytime. How about this ..

Create a function ... (untested)

void sendBytes(byte theByte1, byte theByte2,int SLAVESELECT){
         digitalWrite(SLAVESELECT,HIGH);
          SPDR = theByte1; //load second byte with the DAC address 1 and range 2
         while (!(SPSR & (1<<SPIF))); //wait to end magnitude transmission
          SPDR = theByte2; //load second byte with the DAC address 1 and range 2
         while (!(SPSR & (1<<SPIF))); //wait to end magnitude transmission
         digitalWrite(SLAVESELECT,LOW);
       delayMicroseconds(3);
}

Then your code is much easier to read ...

void SPIDAC::SPIDACset(int SLAVESELECT,int frontleft,int frontright,int rearleft,int rearright){

sendBytes(0b00000001,frontleft,SLAVESELECT);
sendBytes(0b00000011,frontright,SLAVESELECT);
sendBytes(0b00000101,rearleft,SLAVESELECT);
sendBytes(0b00000111,rearright,SLAVESELECT);

}

There is also a function that may make your code more readable by changing ...
while (!(SPSR & (1<<SPIF)));
to:
loop_until_bit_is_set(SPSR, SPIF);

3

I would also suggest

#define DAC_FRONT_LEFT 0b00000001

and then use that ..

sendBytes(DAC_FRONT_LEFT,frontleft,SLAVESELECT);

More functions and good constant values will help your code be easier to read and maintain.

4

Ah! those look like good suggestions.

There is also a function that may make your code more readable by changing ...
while (!(SPSR & (1<<SPIF)));
to:
loop_until_bit_is_set(SPSR, SPIF);

Let me think about this one more. So I would just create a 3rd function for this or is it a built in function? Eh, I guess I will find out. Gimme a few hours to make some changes. I will repost when done.

5

Alright, that looks much better:
h

#ifndef SPIDAC_h
#define SPIDAC_h

#include "WProgram.h"

class SPIDAC
{
public:

      void SPIDACinit(int SLAVESELECT);
      void SPIDACset(int SLAVESELECT, int DACA,int DACB,int DACC,int DACD);
private: 

      void sendBytes(byte address, byte value,int SLAVESELECT);

static const int DATAOUT = 51;
static const int SPICLOCK = 52;
#define DAC_ADDRESS_A 0b00000001
#define DAC_ADDRESS_B 0b00000011
#define DAC_ADDRESS_C 0b00000101
#define DAC_ADDRESS_D 0b00000111
};

#endif

cpp

#include "WProgram.h"
#include "SPIDAC.h"
#include "SPI.h"

void SPIDAC::SPIDACinit(int SLAVESELECT)
 {

  SPCR = (1<<SPE)|(1<<MSTR);
  // enable SPI, set as master
  SPCR = (1<<CPHA);
  // =Samples taken on falling edge
  byte clr;
  clr=SPSR; //clear bad data
  clr=SPDR; //clear bad data
  delay(10); 
  // set the SLAVESELECT as an output:
  pinMode(SLAVESELECT, OUTPUT);
  pinMode(DATAOUT, OUTPUT);
  pinMode(SPICLOCK,OUTPUT);
  SPI.begin();

}

void SPIDAC::SPIDACset(int SLAVESELECT,int DACA,int DACB,int DACC,int DACD){

      sendBytes(DAC_ADDRESS_A,DACA,SLAVESELECT);
      sendBytes(DAC_ADDRESS_B,DACB,SLAVESELECT);
      sendBytes(DAC_ADDRESS_C,DACC,SLAVESELECT);
      sendBytes(DAC_ADDRESS_D,DACD,SLAVESELECT);

}

void sendBytes(byte address, byte value,int SLAVESELECT){
         digitalWrite(SLAVESELECT,HIGH);

           SPDR = address; //load first byte with the DAC address 1 and range
               while (!(SPSR & (1<<SPIF))); //wait to end transmission

            SPDR = value; //load second byte with magnitude
               while (!(SPSR & (1<<SPIF))); //wait to end transmission

         digitalWrite(SLAVESELECT,LOW);
         delayMicroseconds(3);
}

Is there anything I can do to clean up my initialization function? As I said in my first post, I will have to select between DAC chips on my final program. I have the SPI initializing and and Slaveselect initializing in the SPIDACinit function. I think I would have to use the initialize function again if I wanted to switch DAC chips (aka, change the slaveselect pin). Would I have problems initializing SPI more then once? There has to be a more proper way to initialize.

6

I would try it "as-is" and if using SPI.begin twice causes an issue move the call to SPI.begin outside the function and into the main app after creating your class instances.

Your init function is fine. You may be able to push some or all of that into the constructor .. but don't see any major advantages (minus one call extra call needed to call the init function).

7

Might I suggest that including the class name in the function name is not really necessary?

Might I also suggest that init() is not the common name for the function that sets up the class for use? The common name, for Arduino related classes, is begin().