1
\$\begingroup\$

I have this API to control some hardware. The API is not very well documented, but I've been able to make do with what I have. It is a API for a RFID device, but also has LED's on it that are controlled from the API.

So the idea is that this is a OPOS program. The RFID device opens/claims/and enables. When it is enabled I start a thread that polls the reader to see if there is a RFID card in the field. The user has the ability to turn on and off the LED's when ever they want by using a DirectIO call.

Here is the hard part. I've found that if i make concurrent calls to the API that the commands will give me back some gibberish (might be wrong word) answer. Valid messages returned are 0 to -7. We wrote a simple parser to run commands for us so we don't have to change the program it self to test for bugs.

Open
Claim
AllEnable
LightOn(White,45)

that is the bit of code I use to test this bug. I can't seem to fix it, and I've concluded that my multithreaded skills are.. lacking.

In my second thread that is checking for the cards it works something like this.

while (pSO->reader.m_bDeviceEnabled && !closeForAutoDisable)
{
    if(pSO->reader.m_nState == OPOS_S_IDLE)
    {
        Sleep(10);
        if (CheckCardTimeout(pSO->reader.readerID, pSO->reader.m_nCardDelay))
        {
            pSO->reader.SetState(OPOS_S_BUSY);
            int state = CheckForCard(pSO);
            pSO->reader.SetState(OPOS_S_IDLE);

I use the state to tell if the reader is potentially sending commands to the API.

I tried putting in a CCtricialSection in my reader class but when I compile it says cannot access private memdeclared in class 'CObject'. I find that stupid because in my main class it has a CCriticalSection declared with no problem. so I had to comment out the CCriticalSection and the 2 methods I tried to use in reader.SetState

void SetState(LONG state)
{
    //locker.Lock();
    m_nState = state;
    //locker.Unlock();
}

Now for the section is bothers me. The "LED" on the device is actually a RGB LED. So to get White i have to make 3 api calls to turn on Red Green and Blue. Same goes with any other color I want. Below is the code. I took out the boring non-relevant code.

LONG ReaderHelper::SetLED(LONG duration, CString color) 
{
    m_nResultCode = OPOS_E_BUSY;
    for(int i=0; i<300; i++)
    {
        if(m_nState==OPOS_S_IDLE)
        {
            SetState(OPOS_S_BUSY);
            m_nResultCode = OPOS_SUCCESS;
            break;
        }
        Sleep(1); //wait at most 300ms
    }
    if(m_nResultCode == OPOS_E_BUSY)
    {
        WRITE_ERROR("Reader busy");
        return WRITE_ENDO(m_nResultCode);
    }
    LONG err = SetLED(color[0], LEDtimes, duration);
    SetState(OPOS_S_IDLE);
    return err;
}
LONG ReaderHelper::SetLED(char color, BYTE times, BYTE duration)
{
    WRITE_START;
    if (IsNotOpen())
        return WRITE_ENDO(m_nResultCode);
    //if(IsBusy())
    //    return WRITE_ENDO(m_nResultCode);

    SetState(OPOS_S_BUSY);
    switch(color)
    {
    case 'r':
    case 'R':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(true, false, false, times, duration);
        break;
    case 'g':
    case 'G':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(false, true, false, times, duration);
        break;
    case 'b':
    case 'B':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(false, false, true, times, duration);
        break;
    case 'y':
    case 'Y':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(true, true, false, times, duration);
        break;
    case 'o':
    case 'O':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(true, true, false, times, duration);
        break;
    case 'c':
    case 'C':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(false, true, true, times, duration);
        break;
    case 'p':
    case 'P':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(true, false, true, times, duration);
        break;
    case 'w':
    case 'W':
        SetAllLights(true, true, true, 0, LedOff);
        m_nResultCode = SetAllLights(true, true, true, times, duration);
        break;
    }
    return m_nResultCode;
}
LONG ReaderHelper::SetAllLights(bool red, bool green, bool blue, BYTE times, BYTE duration)
{
    //there is not State checking or setting with this method because of concurrency
    TurnOffAntenna();
    LONG rc = OPOS_SUCCESS;
    if(red)
    {
        rc |= GetError(TrySetLight(RedLed, times,duration));
    }
    if(blue)
    {
        rc |= GetError(TrySetLight(BlueLed, times, duration));
    }
    if (green)
    {
        rc |= GetError(TrySetLight(GreenLed, times, duration));
    }
    return rc;
}
int ReaderHelper::TrySetLight(BYTE led, BYTE blinks, BYTE duration)
{
    int rc = LIB_SUCCESS;
    //tried different time to try.. sticking with one for now
    for(int i=0; i<1; i++) 
    {
        rc = rf_Reader_SetLed(readerID, led, blinks,duration);
        if(rc==LIB_SUCCESS)
            break;
    }
    return rc;
}

The code at top that should turn the light white for 450ms. If I run that code in a completly non-OPOS environment all by itself it will flash white a billion times with no problems. In the multi-threaded OPOS program..it does strange things. most frequently red turns on first for a moment then green and blue turn on and then turn off at different intervals. (results vary, but this is most typical) I do know that the calls to the LED's are asynchronous (they have to be to allow for different color combo's) but I am completly lost as to how to block other API calls from my other threads until the coast is clear. Any suggestions?

\$\endgroup\$
  • \$\begingroup\$ There's too little to really understand what this is about. You claim multithreading; I do not see anything threading. And by-the-way what is OPOS? \$\endgroup\$ – user23573 Oct 1 '15 at 10:02
  • \$\begingroup\$ @BogdanWilli I mention threading a few times in the question, but I don't show any of the code that does the actual spinning up of any threads... Which after reading this question again I think it doesn't fit the bill of a proper question for this site I believe. I have since thrown all that C++ code away and re-wrote it in C#. The problem also ended up resolving itself because the user changed their mind. As for OPOS, it's now called UPOS and stands for Point Of Service (you've probably heard of it as Point Of Sale): Essentially it's a set of interfaces you use/implement for the retail world. \$\endgroup\$ – Robert Snyder Oct 1 '15 at 12:03
  • \$\begingroup\$ There exists a world beyond Windows. I never came across OPOS despite programming for Windows for years. You know that you can include links in your questions (and also edit your questions) You could turn it into a C# question if the problem still exists. The idea here on CodeReview is to provide more complete code examples. If you have more specific question it is probably better to ask them on stackoverflow. Unfortunately I'm not a C# guy. \$\endgroup\$ – user23573 Oct 2 '15 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.