Help with OO instrument setup

I'm working on making a class for an instrument I'm working with. This is done with a driver from the vendor.

Once I get the class done I will work on making some sort of GUI to see if my commands are working and setting the instrument as I want.

Am I on the right track? I think I define my own types in case I want to change the return types easily, error types, etc.

Code:

---

#pragma once#include <string>


class InstrumentX
{
public:
        InstrumentX();
        ~InstrumentX();
        long NA_Init(std::string rscr);
        long NA_Close();
        long NA_ErrorQuery(long *errorCode, std::string errorMessage);


private:
        unsigned long handle;
};

---

Code:

---

#include "InstrumentX.h"#include "IXdriver.h"
#include <IviVisaType.h>


InstrumentX::InstrumentX()
{
}


InstrumentX::~InstrumentX()
{
}


long InstrumentX::NA_Init(std::string rscr)
{
        long status;
       
        status = IX_InitWithOptions(const_cast<char *>(rscr.c_str()), VI_TRUE, VI_TRUE, "", &handle);
       
        return status;
}


long InstrumentX::NA_Close()
{
        long status;
       
        status = IX_close(handle);


        return status;
}


long InstrumentX::NA_ErrorQuery(long *errorCode, std::string errorMessage)
{
        long status;
        char* temp = nullptr;
       
        status = IX_error_query(handle, errorCode, temp);


        errorMessage = temp;


        return status;
}

---

Some comments...

NA_Init should really be the constructor (it is the constructor's job to set up a class, which is what this function apparently does).
NA_Close should really be the destructor (it is the destructor's job to tear down class, which is what this function apparently does).
Prefer exceptions to status codes. If something goes wrong when constructing, throw an exception. If something goes wrong when destructing, log the error or ignore it (but don't throw from your destructor).

Not sure what NA_ErrorQuery is supposed to do?
Why does IX_InitWithOptions that a char* argument?

Deeply think about if the class is supposed to be copyable or not. If someone copies your class, both instances end up with the same handle, and if one of them closes it, chances are the instance will crash when trying to use it.
Also think about whether it's supposed to be moveable or not. Most likely it is, but you have to provide some overloads for move operations; otherwise you're going to run into problems since both instances will share the same handle.
If some operation is inapplicable, disable those operations (e.g. use = delete on appropriate constructors/operators).

The calls to IX_xxxx are the vendor's drivers and I can't really change what parameters they need. It may make sense to make this a char*, since every driver I have worked with usually does that.

I'm trying to make the class generic enough so if I used a different model number that had a different driver I could still use this code. For example in the constructor, I should add a enum for the model number and depending on the result I should use the appropriate driver, or return model isn't supported. The constructor will eventually allocate memory, set initial values, etc. if I need to. The init is when I'm actually going to out and get handle from the instrument itself. The destructor should undo whatever my constructor eventually does.

The error query is a way to take the result of init, close, and other functions if they don't return successfully to see why the instrument failed. Errors could be "invalid session.", "out of range", "function not supported", etc.

I will throw an exception when constructing and not throw from the destructor.

I will need to give some thought if the class is copyable, and I'm thinking it should be, and you right they would need different handles. Thinking out loud, if someone else lost the handle, but the instrument was still up and running, their handle should still be valid.

Great advice and it's very different reading about classes and going out and implementing one.

> status = IX_InitWithOptions(const_cast<char *>(rscr.c_str()), VI_TRUE, VI_TRUE, "", &handle);
If this were me, I would go to the trouble of creating a temporary copy of rscr in a writeable char array, just in case the driver decides to mess with your string.

Just casting away the const-ness is lazy.

Or get the driver vendor to make their interface const-correct.

Quote:

---

Originally Posted by CodeSlapper

The calls to IX_xxxx are the vendor's drivers and I can't really change what parameters they need. It may make sense to make this a char*, since every driver I have worked with usually does that.

---

What I meant to say is... you're sure it doesn't want a modifiable string? const_cast is the absolute last thing you should do, and only to work around broken code.

Quote:

---

I'm trying to make the class generic enough so if I used a different model number that had a different driver I could still use this code. For example in the constructor, I should add a enum for the model number and depending on the result I should use the appropriate driver, or return model isn't supported.

---

I think you should use a base class with the common functionality and a derived class for each unique type of driver rather than an emum.

Quote:

---

The constructor will eventually allocate memory, set initial values, etc. if I need to. The init is when I'm actually going to out and get handle from the instrument itself. The destructor should undo whatever my constructor eventually does.

---

If you create the class instance near first use, there is no problem with the constructor getting the handle.

Quote:

---

The error query is a way to take the result of init, close, and other functions if they don't return successfully to see why the instrument failed. Errors could be "invalid session.", "out of range", "function not supported", etc.

---

I'd rather see individual functions checking for error, and if there is one, querying the error and wrapping that in an exception and throwing. There's no reason why the user has to manually query that.