WinCC c script read bits from word - general C suggestions

Question

I have a huge problem on my hands and I assume that the cause of it not functioning properly is in my C language knowledge which is, let's face it pretty newbie at best...

void * KT_Indirect_CNS(int NumCNS, char *CNSDescr[], char *CNSTag[], int *CNSIsBit[], char* lpszPictureName){

#include "apdefap.h"

char  TagNameEnable[255],
    TagNameTrue[255],
    TagNameFalse[255],
    TagNameString[255],
    TagNameCNS[255],
    TagNameCNSString[255],
    TagNameTemp[255];

int   i,
        count;

typedef struct 
{ 
    LONG x; 
    LONG y; 
} tPOINT;

#pragma code ("user32.dll");
BOOL GetCursorPos( tPOINT* lpPoint);
#pragma code();

tPOINT pt;

GetCursorPos(&pt);

for (i=1; i<=NumCNS; i++)
{
    sprintf(TagNameEnable, "CNS%d.Enabled", i);
    SetTagBit(TagNameEnable, 1);

    sprintf(TagNameCNS, CNSTag[i-1]);
    sprintf(TagNameTrue, "CNS%d.True", i);
    sprintf(TagNameFalse, "CNS%d.False", i);
    if (CNSIsBit[i-1] == 17)
    {
              BOOL temp = GetTagBit(TagNameCNS);
        if (temp==1)
        {
            SetTagBit(TagNameTrue, 1);
            SetTagBit(TagNameFalse, 0);
                    count++;
        }
        else
        {
            SetTagBit(TagNameTrue, 0);
            SetTagBit(TagNameFalse, 1);
        }
    }
    if ((CNSIsBit[i-1] >= 0) && (CNSIsBit[i-1] <=16))
    {
        int *bits = GetBitsFromWord(GetTagWord(TagNameCNS));
        int index = (int)CNSIsBit[i-1];
        if (bits[index]==1)
        {
            SetTagBit(TagNameTrue, 1);
            SetTagBit(TagNameFalse, 0);
                    count++;
        }
        if (bits[index]==0)
        {
            SetTagBit(TagNameTrue, 0);
            SetTagBit(TagNameFalse, 1);
        }
    }
    sprintf(TagNameString, "CNS%d.String", i);
    SetTagChar(TagNameString, CNSDescr[i-1]);
}

if (NumCNS <16)
{
    for (i=NumCNS+1; i<=16; i++)
    {
        sprintf(TagNameTrue, "CNS%d.True", i);
        sprintf(TagNameFalse, "CNS%d.False", i);
        sprintf(TagNameEnable, "CNS%d.Enabled", i);
        SetTagBit(TagNameTrue, 0);
        SetTagBit(TagNameFalse, 0);
        SetTagBit(TagNameEnable, 0);
    }
}

if (count != NumCNS)
{
    sprintf(TagNameTemp, "CNS_Page_AllGood");
    SetTagBit(TagNameTemp, 0);
        SetVisible(lpszPictureName,"KT_CNS",TRUE);  
    sprintf(TagNameTemp, "CNS_PAGE_MOUSE_X");
    SetTagSWord(TagNameTemp, pt.x-5);
    sprintf(TagNameTemp, "CNS_PAGE_MOUSE_Y");
    SetTagSWord(TagNameTemp, pt.y-150);
    sprintf(TagNameTemp, "CNS_PAGE_HEIGHT");
    SetTagSWord(TagNameTemp, 30*NumCNS);
}
if (count == NumCNS)
{
    sprintf(TagNameTemp, "CNS_Page_AllGood");
    SetTagBit(TagNameTemp, 1);
}

return ("Controller Initilized");}


int *GetBitsFromWord(WORD _word){

int bits[16];

bits[0] = (_word & 0x0001)?1:0;
bits[1] = (_word & 0x0002)?1:0;
bits[2] = (_word & 0x0004)?1:0;
bits[3] = (_word & 0x0008)?1:0;
bits[4] = (_word & 0x0010)?1:0;
bits[5] = (_word & 0x0020)?1:0;
bits[6] = (_word & 0x0040)?1:0;
bits[7] = (_word & 0x0080)?1:0;
bits[8] = (_word & 0x0100)?1:0;
bits[9] = (_word & 0x0200)?1:0;
bits[10] = (_word & 0x0400)?1:0;
bits[11] = (_word & 0x0800)?1:0;
bits[12] = (_word & 0x1000)?1:0;
bits[13] = (_word & 0x2000)?1:0;
bits[14] = (_word & 0x4000)?1:0;
bits[15] = (_word & 0x8000)?1:0;

return bits;}

These are the scripts that I use to collect general not running causes for devices in a plant... I assumed it worked when I tested it, stupidly I would add... I think that my pointer usage knowledge is low enough to have made a mistake that would get me killed I think... Essentially I need to collect a WORD type data (16-bit) from my PLC (s7-400) and I need to reed bits from said word to determine the status of objects on screen in WinCC. Thing is I don't get the values I expect... It compiles without problems but... Someone has some suggestions? How can I improve my code?

Answer 1

Oh boy, quite some remarks for code improvement and possibly solving your problem:

You do an include inside a function. That is bad practice. Then you define a typedef inside the function and have pragma's and a prototype. It is strongly suggested to do this before you start a function, so:

#include "apdefap.h"

typedef struct 
{ 
    LONG x; 
    LONG y; 
} tPOINT;

#pragma code ("user32.dll");
BOOL GetCursorPos( tPOINT* lpPoint);
#pragma code();

void * KT_Indirect_CNS(int NumCNS, char *CNSDescr[], char *CNSTag[], int *CNSIsBit[], char* lpszPictureName)
{
    char  TagNameEnable[255],
        TagNameTrue[255],
        TagNameFalse[255],
        TagNameString[255],
        TagNameCNS[255],
        TagNameCNSString[255],
        TagNameTemp[255];

    int   i, count;
    //...
}

In your loop for (i=1; i<=NumCNS; i++) you start at 1. In C all indexes start at 0. In the remainder of the code I see no compelling reason to start at 1, so just start at zero: for (i=0; i<NumCNS; i++), however, it is not wrong what you do.

But your GetBitsFromWord really has a problem. Inside this function you declare an array int bits[16] and at the end of the function you return this array. But the aray is a so-called automatic (local) variable that doesn't exist anymore when the function returns (it is allocated on the stack and that part is released upon return and may be overwritten by following function calls).

Instead, declare it as void GetBitsFromWord(WORD _word, int bits[16]) and pass the bits array as a parameter from the caller.