HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

NI-DAQmx C++ wrapper code (for analog data acquisition using National Instruments measurement hardware)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nationalinstrumentsmeasurementhardwarewrapperforusingcodedaqmxdata

Problem

This is a C++ wrapper for the National Instruments ANSI C library for interfacing with NA-DAQmx devices such as the USB-6008. I wrote it for use in real-time data processing to test algorithms developed in my dissertation research.

I'm sure there's lots of room for improvement. Suggestions on style are welcome, in addition to contributions to support more features of the NI hardware (the emphasis of the current code is on near-simultaneous acquisition of a few analog voltage channels)

```
#pragma once

/** @file daqmxcpp.h
** @author R Benjamin Voigt (richardvoigt@gmail.com)
**
** Rights reserved. This code is not public domain.
**
** Licensed under CC BY-SA 3.0 http://creativecommons.org/licenses/by-sa/3.0/
** or Lesser GPL 3 or later http://www.gnu.org/copyleft/lesser.html
** with the following restrictions (per GPL section 7):
** - all warranties are disclaimed, and if this is prohibited by law, your sole remedy shall be recovery of the price you paid to receive the code
** - derived works must not remove this license notice or author attribution
** - modifications must not be represented as the work of the original author
** - attribution is required in the "about" or "--version" display of any work linked hereto, or wherever copyright notices are displayed by the composite work
**/

#include

namespace NIDAQmx
{
namespace innards
{
extern "C" {
#include
#pragma comment(linker, "/DEFAULTLIB:nidaqmx-32.lib")
}
}

class DAQException : public std::exception
{
const innards::int32 m_code;

DAQException& operator=(const DAQException&);
public:
DAQException(innards::int32 errorCode)
: exception("exception in NI-DAQmx library")
, m_code(errorCode)
{
}

int code() const { return m_code; }
std::string description() const
{
std::string buffer;
innards::int32 neededSize =

Solution

A C header inside extern "C" inside a nested namespace?
This is a new one on me, so maybe I am missing something.
Apparent benefit of the namespace nesting:

  • the origin of the definitions becomes apparent by the innards:: qualification in this header.



  • a module including this header only gets its namespace a little bit polluted by the C definitions.


It gets any #defines from the C header, but not other definitions and declarations.

Apparent drawbacks:

  • If any of those #defines get used in the module and they reference the namespace-hijacked declarations, things could get awkward.



  • Also, if the C header has traditional macro-based guards against multiple inclusion,


any module with a prior include of the C header -- possibly added in desperation to resolve the references leaked via #defines -- will effectively empty innards, breaking the C++ header code.

.

namespace innards
{
    extern "C" {
#include 
#pragma comment(linker, "/DEFAULTLIB:nidaqmx-32.lib")
    }
}


It would be clearer to use a typedef like DAQErrorCode in place of innards::int32
in the cases (predominant) where that is its purpose.
The details of its representation are secondary.

If you are going to use "sealed", #define an ifdef'ed shim for it for platforms that don't support it.

Initial uppercase for member functions -- is that a Windows convention?

The following block could be encapsulate as DAQException::throwIfError(error);
everywhere it occurs, leaving it to that function to determine which values justify an
exception and possibly which type of exception to throw for each value.

if (error < 0)
            throw DAQException(error);


Suggestion: It MAY be better to force the "no custom scale" case of AddChannel to explicitly use a different overload.
The code for the simplified overload would be considerably more readable.
This could also enable treating use of an explicit empty string as an
error, so that if the caller bothers to pass a customScale, it should serve a purpose vs. specifying the default.
The usefulness of such error catching depends on how convenient it is for the
caller(s) to distinguish the default case vs. it being typically hidden
from them in a string that was passed down from a higher level caller.

If an already constructed std::string is typically being passed into the caller,
a const std::string & customScale argument would seem preferable.

Is there any assurance from the C API documentation that the customScale c_str passed into the C api can be immediately deleted upon return (i.e. is never cached as a pointer)?

If the expectation is that bufferSize can ALWAYS be determined by a static vector declaration matching a template argument, the explicit bufferSize overloads wrapped by the template member functions of Task should be made private.

It seems strangely inconsistent not to have a non-templated variant of SyncReadTimeout as there are for these others.

SyncReadNonInterleaved and SyncReadTimeout (adjusting for it being a single template function vs. an overloaded function with a template wrapper) are 20-line functions that differ only in 2 function arguments, which SyncReadNonInterleaved simply hard-codes.
SyncReadNonInterleaved could take additional arguments defaulting to its hard-coded values: double secondsTimeout = -1, innards::bool32 fillMode = DAQmx_Val_GroupByChannel
so that SyncReadTimeout could be/make a call to SyncReadNonInterleaved overriding those defaults.

OR a separate neutral private function with no argument defaults could be factored out and called by both functions.

A generic declaration like this can help to take the static casting clutter out of the callback code (see modified code below).
Disclaimer: This change has not been test-compiled.

typedef innards::int32 CVICALLBACK (*genericCallback)(innards::TaskHandle, innards::int32, innards::uInt32 /*nSamples*/, void* pCallbackData);


This void* return seems like a dead end. ReadSamplesRegistration is so well-hidden that there's not even any way to deallocate one.
Should ReadSampleRegistration be broken out of the function AND some more explicitly typed (but possibly still opaque) pointer be returned, so that a corresponding Unsubscribe function can unregister and delete the instance?

template
    void* SubscribeSamplesRead(unsigned int blockSize, TFunctor* callbackFunctor)
    {
        struct ReadSamplesRegistration
        {
            Task* const m_pTask;
            TFunctor* const m_functor;


I'd properly specialize this signature and use a function pointer cast (below) instead of a callbackData cast (here).

static innards::int32 CVICALLBACK Callback(innards::TaskHandle taskHandle, innards::int32, innards::uInt32 /*nSamples*/, ReadSamplesRegistration* that)
            {


The member function of a struct defined within a member function gets private access (like to m_handle) just like its containing member function? I had no idea.
I'd have provided (public) ove

Code Snippets

namespace innards
{
    extern "C" {
#include <NIDAQmx.h>
#pragma comment(linker, "/DEFAULTLIB:nidaqmx-32.lib")
    }
}
if (error < 0)
            throw DAQException(error);
typedef innards::int32 CVICALLBACK (*genericCallback)(innards::TaskHandle, innards::int32, innards::uInt32 /*nSamples*/, void* pCallbackData);
template<typename TFunctor>
    void* SubscribeSamplesRead(unsigned int blockSize, TFunctor* callbackFunctor)
    {
        struct ReadSamplesRegistration
        {
            Task* const m_pTask;
            TFunctor* const m_functor;
static innards::int32 CVICALLBACK Callback(innards::TaskHandle taskHandle, innards::int32, innards::uInt32 /*nSamples*/, ReadSamplesRegistration* that)
            {

Context

StackExchange Code Review Q#8840, answer score: 3

Revisions (0)

No revisions yet.