Passing method's pointer into library function

Renewer

There is an usb device (CAN-converter i-7565-H1) which is working with a proprietary dll.
It's API propose to use user defined ISR function.
"This function is used to set the user-defined callback ISR function and
the assigned port number, mode and ID of the received CAN message."

//Syntax:
int VCI_Set_UserDefISR (
BYTE ISRNo,
BYTE CAN_No,
BYTE Mode,
DWORD CANID,
void (*UserDefISR)()
);

I've tried to pass there pure method's pointer, but compiler said:

cannot convert 'void (QSerialTranciever::*)()' to 'void (*)()'

Then I converted types like this. It works.

VCI_Set_UserDefISR(ISRNO_0,
                   ISR_CANPORT_ALL,
                   ISR_CANMODE_ALL,
                   ISR_CANID_ALL,
                   reinterpret_cast<void(*)()>(&(this->readBuffer)));

But when my readBuffer method emitting a singal the app was crashing. Even if not, signal was not recieved.
I actually don't know, but I guess this happening because of type convertion. Method is trying to emit the signal, but don't have any data what instance it's owning.

Thanks to I'm using only one implementation this class QSerialTranciever, I added static field with last created instance pointer:

//////////// .h
private:
    static QSerialTranciever * instance;
//////////// .cpp
QSerialTranciever::QSerialTranciever() { //Class constructor
    QSerialTranciever::instance = this;
}

void QSerialTranciever::readBuffer() { //My ISR Method
//// some ////
QSerialTranciever::instance->sendSignal(message);
//// some ////
}

void QSerialTranciever::sendSignal(_VCI_CAN_MSG &message) { //Signal Emiting
    emit serialMessage(message);
}

This is working solution, but I'm not sure it's absolutely correct.
Could I make it better and how?

kshegunov

@Renewer

cannot convert 'void (QSerialTranciever::*)()' to 'void (*)()'

Well pointer to members are a bit different from regular function pointers, so that's why the compiler is complaining.

reinterpret_cast<void(*)()>(&(this->readBuffer)))

Eh, it should work, as long as there's no polymorphic behavior involved with/expected from that function. You're basically forcefully casting the address of the text section of the binary where the class method sits to a pointer to a regular function. However, as you saw already, there is no context (i.e. you can't use this and any other indirection that may involve).

reinterpret_cast is curious operator in that way (together with the union construct), as it doesn't actually translate to any assembler instructions. It's "reinterpreting" the data behind the address, hence the name, to a new type without performing any checks.

But when my readBuffer method emitting a singal the app was crashing.

What is happening is that the API you're working with (VCI_Set_UserDefISR) is calling your class method with an invalid this pointer so any call in QSerialTranciever::readBuffer that requires a valid object (namely a non-static invocation, what emitting a signal is) will just arbitrarily crash.

Thanks to I'm using only one implementation this class QSerialTranciever, I added static field with last created instance pointer.
This is working solution, but I'm not sure it's absolutely correct.

Well, it's correct up to the point that you want to pass along context when such is not expected, so you're forced by the way VCI_Set_UserDefISR works to work around that. I wouldn't personally call a member as a regular function (I'd just use a plain ol' C function), but in any case you need somehow to keep track of the object itself, so making a static pointer to it is a valid solution as any.

If you know for sure no threading is involved and the control will not be returned from VCI_Set_UserDefISR until your callback is called, you can wrap the call to the API in something like this:

class VCI_Set
{
public:
    VCI_Set(QSerialTranciever * str)
        : transceiver(str)
    {
    }

    bool operator () (BYTE isrNumber, BYTE canNumber, BYTE mode, DWORD canid)
    {
        return VCI_Set_UserDefISR(isrNumber, canNumber, mode, canid, VCI_Set::callback) == 0;
    }

private:
    static void callback()
    {
        transceiver->sendSignal( ... );
    }

    static QSerialTranciever * transceiver;
};

It's basically what you're doing, with the major exception that a static method is used, which should be implicitly convertible to a function pointer (I haven't checked the standard, but I believe they are one and the same). The improvement is only that the actual call is pulled out from the QSerialTranciever object and has a dedicated handler.

The above snippet is used as usual with functors:

void QSerialTranciever::readBuffer()
{
    // Some code

    VCI_Set myVCI_Set(this);
    myVCI_set(ISRNO_0, ISR_CANPORT_ALL, ISR_CANMODE_ALL, ISR_CANID_ALL);

    // Some more code
}

I hope that clears it up.

Kind regards.

Renewer

@kshegunov said:

I hope that clears it up.

That's what I was waiting for. Thanks much.

If you know for sure no threading is involved and the control will not be returned from VCI_Set_UserDefISR until your callback is called, you can wrap the call to the API in something like this:

Unfortunately I do use threading. What could be wrong?

P.S. I used @kshegunov example by the following way:

#include "qserialtranciever.h"

class VCI_ISR_Set
{
    static QSerialTranciever * tranciever;
    static int ISRNumber;

public:
    VCI_ISR_Set(QSerialTranciever * str)
    {
        tranciever = str;
    }
    ~VCI_ISR_Set()
    {
        unsetFunction();
    }

    int setFunction(BYTE ISRNo, BYTE CAN_No, BYTE Mode, DWORD CANID)
    {
        ISRNumber = ISRNo;
        return VCI_Set_UserDefISR(ISRNo,CAN_No,Mode,CANID, VCI_ISR_Set::callback);
    }
    int unsetFunction(BYTE ISRNo = ISRNumber)
    {
        return VCI_Clr_UserDefISR(ISRNo);
    }

private:
    static void callback()
    {
        tranciever->isrEmit();
    }

};

QSerialTranciever * VCI_ISR_Set::tranciever;
int VCI_ISR_Set::ISRNumber;


QSerialTranciever::QSerialTranciever(QObject *parent)
    : QObject(parent)
{
    vciSet = new VCI_ISR_Set(this);
    connect(this, SIGNAL(isr()), this, SLOT(readBuffer()));
}

QSerialTranciever::~QSerialTranciever()
{
    delete vciSet;
}

void QSerialTranciever::isrEmit()
{
    emit isr();
}

bool QSerialTranciever::errorCode(int code, QString & description)
{
//error code definition
}

void QSerialTranciever::portReconnect()
{
    vciSet->unsetFunction(ISRNO_0);
    //some code
    QString description;
    if (errorCode(VCI_CloseCAN(portNumber), description)) {
       //in case of error
    }
    //some more code
    if (errorCode(VCI_OpenCAN(params), description)) {
        //in case of failure some responce
    } else {
        vciSet->setFunction(ISRNO_0, CAN1, ISR_CANMODE_ALL, ISR_CANID_ALL);
    }
}

void QSerialTranciever::readBuffer()
{
    QString description;
    DWORD size;
    if (errorCode(VCI_Get_RxMsgCnt(CAN1, &size), description)) {
        //error response
    }
    while (size != 0)
    {
        _VCI_CAN_MSG message;
        if (errorCode(VCI_RecvCANMsg(CAN1, &message), description)) {
            //error response
        }
        emit serialMessage(message);
        if (errorCode(VCI_Get_RxMsgCnt(CAN1, &size), description)) {
            //error response
        }
    }
}

kshegunov

@Renewer

Unfortunately I do use threading. What could be wrong?

Well the pointer to the object is unprotected so it's vulnerable to race conditions, as well as the object itself. However, if you use the global variables from a single thread you'll be fine. It really depends on your program and how and where you do the threading.

P.S. I used @kshegunov example by the following way:

One remark:
Signals are regular functions and (as of Qt 5) are public, so you can emit the signal directly:

static void callback()
{
    tranciever->isr();
}

Kind regards.