Driver-layer from top-level app to hardware

Question

I'm attempting to implement driver-layer (shared library) from top-level app to hardware.

In the first, I extracted interface functions, which further will be called from "extern "C" SHARED" wrappers to separate class IDriverInterface.

In the second, I want to hot-test some functions before the correct implementation and don't want to use #define, so I created a QDriverInterfaceSim class which inherits QDriverInterface.

Can anyone suggest any improvements on this?

IDriverInterface class:

#ifndef IDRIVERINTERFACE_H
#define IDRIVERINTERFACE_H
#include "types.h"
class IDriverInterface
{
public:
 virtual int connectToHardware() = 0;
 virtual int sendData(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz) = 0;
 virtual int sendDataWithAck(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz, ack_id_t *p_ack_id) = 0;
 virtual int sendDataWithAckBlocked(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz, timeout_t timeout_ms) = 0;
 virtual void reg_OnData_callback(onData_callback cb) = 0;
 virtual void reg_OnAck_callback(onAck_callback cb) = 0;
 virtual int addRouteRecord(dev_addr_t dev, dev_addr_t dest, dev_addr_t next) = 0;
 virtual int removeRouteRecord(dev_addr_t dev, dev_addr_t dest) = 0;
 virtual int getRouteTable(dev_addr_t dev, route_t route_table[1], rows_cnt_t *prow_count) = 0;
 virtual int setAutoRoute(dev_addr_t dev, bool en) = 0;
 virtual int ping(dev_addr_t dev, timeout_t timeout_ms) = 0;
};
#endif // IDRIVERINTERFACE_H

QDriverInterface class:

#ifndef QDRIVERINTERFACE_H
#define QDRIVERINTERFACE_H
#include <QObject>
#include <QMutex>
#include <QDebug>
#include "qmyserialport.h"
#include "qserialportconfigdialog.h"
#include "qserialportinterface.h"
#include "idriverinterface.h"
#include "types.h"
class QDriverInterface : public QObject, public IDriverInterface
{
 Q_OBJECT
public:
 explicit QDriverInterface(QObject *parent = 0);
 // from IDriverInterface
 virtual int connectToHardware();
 virtual int sendData(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz);
 virtual int sendDataWithAck(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz, ack_id_t *p_ack_id);
 virtual int sendDataWithAckBlocked(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz, timeout_t timeout_ms);
 virtual void reg_OnData_callback(onData_callback cb);
 virtual void reg_OnAck_callback(onAck_callback cb);
 virtual int addRouteRecord(dev_addr_t dev, dev_addr_t dest, dev_addr_t next);
 virtual int removeRouteRecord(dev_addr_t dev, dev_addr_t dest);
 virtual int getRouteTable(dev_addr_t dev, route_t route_table[1], rows_cnt_t *prow_count);
 virtual int setAutoRoute(dev_addr_t dev, bool en);
 virtual int ping(dev_addr_t dev, timeout_t timeout_ms);
private slots:
 void setSerialParams();
protected:
 onData_callback m_onData;
 onAck_callback m_onAck;
 void debug_print(QString fun_name, QString msg = "");
private:
 void init(); 
 void showConfigDialog();
 void sendTestPacket();
 QMySerialPort *m_serialPort;
 QSerialPortConfigDialog *m_serialConfigDialog;
 QSerialPortInterface *m_SerialPortInterface;
};
#endif // QDRIVERINTERFACE_H

QDriverInterfaceSim class:

#ifndef QDRIVERINTERFACESIM_H
#define QDRIVERINTERFACESIM_H
#include <QObject>
#include <QDebug>
#include "qdriverinterface.h"
class QDriverInterfaceSim : public QDriverInterface
{
 Q_OBJECT
public:
 explicit QDriverInterfaceSim(QObject *parent = 0);
 // from IDriverInterface
 //virtual int connectToHardware() { return 0;}
 virtual int sendData(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz);
 //virtual int sendDataWithAck(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz, ack_id_t *p_ack_id) { return 0;}
 //virtual int sendDataWithAckBlocked(dev_addr_t dev, com_t com, pdata_t pdata, data_sz_t data_sz, timeout_t timeout_ms) { return 0;}
 //virtual void reg_OnData_callback(onData_callback cb);
 //virtual void reg_OnAck_callback(onAck_callback cb);
 //virtual int addRouteRecord(dev_addr_t dev, dev_addr_t dest, dev_addr_t next) { return 0;}
 //virtual int removeRouteRecord(dev_addr_t dev, dev_addr_t dest) { return 0;}
 //virtual int getRouteTable(dev_addr_t dev, route_t route_table[1], rows_cnt_t *prow_count) { return 0;}
 //virtual int setAutoRoute(dev_addr_t dev, bool en) { return 0;}
 //virtual int ping(dev_addr_t dev, timeout_t timeout_ms) { return 0;}
private slots:
 void callOnData();
 void callOnAck();
private:
 void setOnData(onData_callback cb, dev_addr_t *pdev, com_t *pcom, pdata_t pdata, data_sz_t *pdata_sz);
private:
 dev_addr_t *m_pdev;
 com_t *m_pcom;
 pdata_t m_pdata;
 data_sz_t *m_pdata_sz;
};
#endif // QDRIVERINTERFACESIM_H

Answer 1

I do not really understand what you mean by first and second. Does this source code go all into one single module/dll/exe? Or is there an intended hierarchy?

There are a few (more general) suggestions I can give:

• avoid include-file name clashes

  I would change the name of your header "types.h" into something like "legotron_driver_types.h" because there are thousands of software environments and libraries that also have a header called "types.h". You want to avoid these name clashes.

• make your interface symmetric

  There are sendData() functions for sending data. Are there only call-back functions for recieving available? I see a getRouteTable() function while there is no setRouteTable() function. You should have the following functions as well:

  disconnectFromHardware();
  receiveData();
  receiveDataWithAck();
  receiveDataWithAckBlocked();
  setRouteTable();
  removeAutoRoute();
  

  Having the interface completely symmetric can divide your problem up into more manageable chunks.

• divide the interface into "levels"

  You may also want to divide the interface into high-level, middle-level and low-level functions. For example the sendData(); receiveData(); function pair may be considered low-level. While the sendDataWithAck(); receiveDataWithAck(); may be considered a higher level. This allows you to test the levels independently and to first build the lower level where you can build on higher levels later.

• use const keyword to specify input arguments

  I'm not a big fan of name pre-fixes however, you have an interesting way of using them in the type definitions. But you could qualify the functions arguments that are not changed by the function with const. This can effectively specify which arguments are input-only and which have output:

   sendDataWithAck(dev_addr_t const dev, // "in" device
   com_t const com, // "in" com port ?
   pdata_t const pdata, // "in" pointer to data
   data_sz_t const data_sz, // "in" data size
   ack_id_t *p_ack_id); // "out" ack ID
  

• indicate error

  Most of your functions have int as a return value. I assume that this will be an error code. Declare a type for this:

  enum class retCode {
   NO_ERROR = 0,
   SYNTAX_ERROR,
   OTHER_ERROR,
   INTERFACE_ERROR = 400,
   CANNOT_CREATE_INTERFACE_ERROR
  };
  

• be clear about C/C++ borders

  You mention in the intro text something about "extern "C" SHARED" wrappers. Where exactly is your border from C to C++? Do you really need to fall back to C? I see that your member functions mostly use C-Style types. Where is the exact border where you jump from C to C++? In some cases, this may be the only way to do it, but try to use "C++ thinking" whenever you can. It will simplify your code and increase readability.

  Have a look at the defintition of getRouteTable():

  virtual int getRouteTable(dev_addr_t dev, 
   route_t route_table[1], 
   rows_cnt_t *prow_count) = 0;
  

  What is the second argument supposed to pass?

• in C++ use references when you can

  The sendData() function could look like this in more C++ style:

  class Device; // declaration of device class
  class Port; // declaration of com port class
  class DataBlock;
  virtual retCode sendData( Device const & dev,
   Port const & com,
   DataBlock const & pdata,
   size_t const & data_sz );