SPI Connection Class on embedded Linux

Question

I'm currently developing on a embedded Linux chip with a couple of spi devices attached. I wrapped the code to do SPI transfers in a C++ class. A Class represents a single SPI Connection to a single external chip. My C++ is quit rusty and I'm not sure about my code style here. The class itself works well.

SPIConnection.hpp

#ifndef SPIVERBINDUNGHPP3456 #define SPIVERBINDUNGHPP3456 #include <linux/spi/spidev.h> // spi_ioc_transfer u.a #include <string> #include <stdint.h> // for uint8_t, uint16_t etc class SPIConnection { private: int fd; struct spi_ioc_transfer spi; int frequency; std::string name; uint8_t bits; public: SPIConnection(); virtual ~SPIConnection(); /** * Init's the SPI Connection * @method init * -------------------- * @param device the Path to the SPI file * @param mode spi mode * @param bits bits per word * @param LSB lsb or msb * @param speed speed in herz * @param nameParam human readable name of spi conn * @return file descriptor if successfull */ int init(std::string device, uint8_t mode, uint8_t bits, uint8_t LSB, uint32_t speed, std::string nameParam); /** * Transfers data * @method transfer * -------------------- * @param data pointer to send buffer, will be overwritten with received data * @param length number of bytes to be sned * @return 0 if successfull */ int transfer(uint8_t *data, int length); }; #endif /* end of include guard: SPIVERBINDUNGHPP3456 */ 

SPIConnection.cpp

#include "SPIConnection.hpp" #include <fcntl.h> // Needed for SPI port #include <sys/ioctl.h> // Needed for SPI port #include <errno.h> // linux errors #include <unistd.h> // sleep() #include <stdio.h> #include <sys/time.h> #include <stdint.h> // for uint8_t, uint16_t etc #include <linux/spi/spidev.h> #include <cstring> #include <iostream> using namespace std; SPIConnection::SPIConnection() {} SPIConnection::~SPIConnection() { if (fd > 0) { close(fd); } } int SPIConnection::init(const std::string device, const uint8_t mode, const uint8_t bitsParam, const uint8_t LSB, const uint32_t speed, const std::string nameParam) { name = nameParam; frequency = speed; bits = bitsParam; // filedecriptor which will be given back int ret; /* open device */ if ((fd = open(device.c_str(), O_RDWR)) < 0) { perror("Error Open Device"); return -1; } /* set mode */ ret = ioctl(fd, SPI_IOC_WR_MODE, &mode); if (ret < 0) { perror("Error Set SPI-Mode"); return -2; } // /* check mode */ ret = ioctl(fd, SPI_IOC_RD_MODE, &mode); if (ret < 0) { perror("Error Get SPI-Mode"); return -3; } ret = ioctl(fd, SPI_IOC_WR_LSB_FIRST, &LSB); if (ret < 0) { perror("Error Set LSB-Mode"); return -2; } /* set word length*/ ret = ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits); if (ret < 0) { perror("Error Set wordlength"); return -4; } // /* check word length */ ret = ioctl(fd, SPI_IOC_RD_BITS_PER_WORD, &bits); if (ret < 0) { perror("Error Get wordlength"); return -5; } /* set datarate */ ret = ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed); if (ret < 0) { perror("Error Set Speed"); return -6; } // /* check datarate */ ret = ioctl(fd, SPI_IOC_RD_MAX_SPEED_HZ, &speed); if (ret < 0) { perror("Error Get Speed"); return -7; } /* Print */ printf("SPI-Connection.: %s\n", name.c_str()); printf("SPI-Device.....: %s\n", device.c_str()); printf("SPI-Mode.......: %d\n", mode); printf("Wordlength.....: %d\n", bits); printf("Speed..........: %d Hz (%d kHz)\n", speed, speed / 1000); printf("Filehandle.....: %d\n", fd); return 0; } int SPIConnection::transfer(uint8_t *data, int length) { int ret; uint8_t rx[length]; //clear memory memset(rx, 0, sizeof(uint8_t) * length); memset(&spi, 0, sizeof(spi)); spi.tx_buf = (unsigned long)data; spi.rx_buf = (unsigned long)rx; spi.len = length; spi.delay_usecs = 0; spi.speed_hz = frequency; spi.bits_per_word = bits; spi.cs_change = 0; // lets go ret = ioctl(fd, SPI_IOC_MESSAGE(1), &spi); if (ret < 0) { if (errno == EBADF) { std::cout << "ERROR: fd is not a valid file descriptor." << std::endl; } else if (errno == EFAULT) { std::cout << "ERROR: argp references an inaccessible memory area" << std::endl; } else if (errno == EINVAL) { std::cout << "ERROR: request or argp is not valid" << std::endl; } else if (errno == ENOTTY) { std::cout << "ERROR: fd is not associated with a character special device" << std::endl; } else if (errno == ENOTTY) { std::cout << "ERROR: The specified request does not apply to the kind of object that the file descriptor fd references." << std::endl; } perror("Error on I/O - ioctl"); return -1; } //copy to output memcpy(data, rx, sizeof(uint8_t) * length); return 0; } 

main.cpp

#include "SPIConnection.hpp" int main(int argc, char const *argv[]) { /** * Example usage */ SPIConnection spi_conn; spi_conn.init("/dev/spidev32766.0", SPI_CPOL | !SPI_CPHA, 8, 0, 16000, "MX11060"); uint8_t data[10] = "Hello SPI"; spi_conn.transfer(data, 10); return 0; } 

Answer 1

There is a Bug in the Code

The code in SPIConnection::transfer(uint8_t *data, int length) contains at least one bug, it might have been easier to prevent if the multiple if statements were a switch/case statement.

 if (ret < 0) { if (errno == EBADF) { std::cout << "ERROR: fd is not a valid file descriptor." << std::endl; } else if (errno == EFAULT) { std::cout << "ERROR: argp references an inaccessible memory area" << std::endl; } else if (errno == EINVAL) { std::cout << "ERROR: request or argp is not valid" << std::endl; } else if (errno == ENOTTY) { std::cout << "ERROR: fd is not associated with a character special device" << std::endl; } else if (errno == ENOTTY) { std::cout << "ERROR: The specified request does not apply to the kind of object that the file descriptor fd references." << std::endl; } perror("Error on I/O - ioctl"); return -1; } 

The second if (errno == ENOTTY) { will never execute because there is the previous if (errno == ENOTTY) {.

Many modern compilers will actually warn the programmar when it sees the second if.

As I said it might be easier to catch the problem with this code, which is also easier to extend:

 if (ret < 0) { switch (errno) { case EBADF: std::cout << "ERROR: fd is not a valid file descriptor." << std::endl; break; case EFAULT: std::cout << "ERROR: argp references an inaccessible memory area" << std::endl; break; case EINVAL: std::cout << "ERROR: request or argp is not valid" << std::endl; break; case ENOTTY: std::cout << "ERROR: fd is not associated with a character special device" << std::endl; break; case ENOTTY: // Perhaps this should be default: std::cout << "ERROR: The specified request does not apply to the kind of object that the file descriptor fd references." << std::endl; break; } perror("Error on I/O - ioctl"); return -1; } 

The Second Bug in the Code
The return value of spi_conn.init() is never tested to see if the connect was made or not, the following call to spi_conn.transfer() may not perform as expected if there was an error with the connection. Within the call to spi_conn.transfer() there is no check to see if the variable fd has a valid value or not.

Inconsistent Block Indentation
In SPIConnection.cpp the opening brace for int SPIConnection::transfer(uint8_t *data, int length) is on the same line as the function declaration, however, for int SPIConnection::init() the opening brace is on a new line. For all the if statements the opening brace is also on a new line. You should choose one way to do it and be consistent throughout the code.

Magic Numbers
When the int SPIConnection::init() function fails it returns a variety of hard coded integers. It might be better to use symbolic constants that indicate what the errors are:

const int ERROR_OPEN_DEVICE = -1; const int ERROR_SPI_MODE_SETUP = -2; 

These should be defined in the HPP file so that they are shared with the implementation of the function and any functions that call the init function.

Please note that the call to the init function in main() contains 8, 0 and 16000 which are also magic numbers.

Anyone that maintains this code is going to wonder why these values were chosen, symbolic constants helps them understand what the code is doing. It also allows the code to be changed in exactly one place if the values need to change. In two years time if you have to come back to this code you may not know why you chose these particular numbers yourself.

Prefer Clear Naming over using namespace std

According to the [MSDN website]:

Namespaces are used to organize code into logical groups and to prevent name collisions that can occur especially when your code base includes multiple libraries.

A collision is when 2 different functions have the same name, the same arguement types and a similar functionallity (this is why they have the same name). Someone developing software may want to override a function such as std::cout, std::cin or they may want to override the functionallity of a class such as std::vector or std::stack. Namespaces allow these constructs to be overriden.

The use of the programming statement:

using namespace std; 

hides the fact that cin, cout, vector and stack are coming from the namespace std where cin, cout, vector and stack are used in the code. This can cause confusion of where the code is actually coming from.

As the software becomes more complex and uses more libraries this becomes a bigger problem.

For a more detailed discussion of why it is a bad idea to use using namespace std; see this stackoverflow question and stackoverflow question.

Don't mix printf() with std::cout It would be better to choose just one output method, either printf() or std::cout. In this case since you are using perror() it might be better to stick with printf(). Generally in C++ printf() is ignored in favor of std::cout. The output to std::cout can also be formatted.

You may be able to use an errno.h that gives you the correct string and use std::cout rather than perror().

Answer 2

@pacmaninbw has covered most of the points already, but still.

In my opinion, this looks like mess:

if (ret < 0) { perror("Error Set wordlength"); return -4; } // /* check word length */ ret = ioctl(fd, SPI_IOC_RD_BITS_PER_WORD, &bits); if (ret < 0) { perror("Error Get wordlength"); return -5; } // Return -6, -7 and so on 

A better way to do this might be to declare a enum (more c way - C++ people hate this usually) and return those:

enum myErrors { BAD_ERROR = -1, VERY_BAD_ERROR, FIRE, BURN }; 

Or another better way might be to separate out the error handling part to a exception class. E.g.

// Following is just a pseudo-code MyClass::MyFunction throws NastyException { // throw exceptions if(meetError) throw NastyException(); } // Keep error handling separate NastyException() { // Handle error // do whatever to handle this case } 

Introducing magic number in code base is real nightmare - Please avoid them. Exception separate outs error handling code from normal functionality code. This way, it's easier to maintain the code and readability is also improved.