help simplifying function

[darnell]

Im trying to simplify this function, just to get rid of space and lines, basically i want to prevent from calling the Display rs232 error the way im doing it now.

do you think i should put all the if statements in the function call and then just call the one function, any suggestion?

check the see file. you will see how im calling all the if statements, i want to prevent that. 

************************************************************                                                                          
check this particular function at the bottom of the .c file

void DisplayRS232Error (int RS232Error, int read_data)                     
                                                                           

[JGS]

Hello, a minor cleanup is to move the two repeated lines outside the switch

if (RS232Error < 0)
{  
    MessagePopup ("RS232 Message", ErrorMessage);
    DEBGPRNT(ErrorMessage);

}

else

{

    MessagePopup ("RS232 Message", "No Errors");
}

Inside the switch (or if-ifelse-ifelse) you can set content of ErrorMessage on a single line

if (RS232Error < 0)
{

    switch(RS232Error)

    case -2: Fmt (ErrorMessage, "%s", "Invalid port number " "(must be in the range 1 to 8).\n\n"); break;

    case -3: Fmt (ErrorMessage, "%s", "No port is open." "Check COM Port setting in Configure.\n\n"); break;
}

Succes, Jos

[RobertoBozzolo]

As we already have told you in some other threads, you could benefit from using GetRS232ErrorString () function, which returns a descriptive string for any serial communication error you may find.

And somebody already pointed you to bytes_sent and bytes_read parameters being passed to the function and not used by it.

A minor cleaning could be to cut out the MessagePopup for case 0 (you never call this function for case 0; moreover if you receive a popup message for any successful call to a function... )

[Al_S]

As Roberto says, in your current code you never call DisplayRS232Error() for error 0: every call to DisplayRS232Error() is preceded by if (RS232Error).

However, I would probably leave a do-nothing case 0 just in case at some point DisplayRS232Error() gets called with checking RS232Error.

As I mentioned in my post here: http://forums.ni.com/ni/board/message?board.id=180&message.id=43184&query.id=2431002#M43184, put GetRS232ErrorString in your default case, because then you get an error message, and not just the error number.

Don't create cases for standard RS232 errors unless you have something to add to the predefined error message.  Just let GetRS232ErrorString in your default case generate the error message.

I would also create cases only for error messages inteneded for the end user, not for the developer.  Will the user be opening a port and selecting a timeout value?  If so, your messages are good.  However, if your flow of the program forces you to open a port, you don't need a special case for to say "No port is open" vs. "Port not open".

Other specific comments:

Line 124: why do anything before you intialize CVI run time?  Whether or not you have an INI file doesn't matter if you can't start the run time.

Line 130: As I mentioned in other posts looking at this file, use meaningful variable names.  OpenComConfig returns an error code, not the number of bytes_sent.  OpenComConfig doesn't send any bytes.  You code will be easier to maintain if you use meaningful names.

Line 142: bytes_read has not been initialized.  Why even pass it to a function that doesn't use it?

Line 174: meaningful variable names: GetInQLen returns the bytes in queue or bytes2read, not bytes_read

Line 190: bytes_read=GetInQLen(1);  Why have it hardcoded to be port 1, instead of PHYSICAL_PORT?  Why read the bytes in queue again without really doing anything after line 174?

Line 192: So if you can any response back after sending a SET_POINT_QUERY, your instrument is not attached?  That doesn't sound right, but I don't know your instrument.  It semms like you would expect something back after sending a query.

Line 199: meaningful variable names: you're using bytes_read as an input and an output.

Line 226: why say "IF LED turns red..." right after you turn it red?  There should be no "IF" about it.

Line 248: why say "IF LED turns green..." right after you turn it green?

Line 292: meaningful variable names: CloseCom() returns an error code, not the number of bytes_sent.

Line 345: It looks like you're using some kind of modified Hungarian notation.  A variable prefixed with s would be expected to be a string, not an array of integers.  Also as I mentioned in one of our other posts, int sTmpRcvData[100]={0}; is only setting the first element of the array to 0, not the entire array.

Line 371: It looks like bytes_read is not initialized.  What calls this callback?  Are you sure bytes_read is initialized first?

Line 374: Why have it hardcoded to be port 2, instead of PHYSICAL_PORT?

Lines 378 and 396: If you are going to read and display the RS232 error in both cases, why have that duplicated code in the if/else block?  Just do it once before the if at line 378.

Line 407: How do you know that i won't be greater than 99 (you declared int sTmpRcvData[100])?  Are you updating sTmpRcvData just for development purposes?  You never use it.

Line 409: Are you updating iTotalBytesRead just for development purposes?  You never use it.

Lines 438 and 439: why initialize send_data and read_data?  You never use them.

Line 478: How long is your timer tick?  Do you know that there's nothing in the input buffer before you send your query?

Line 502 and 521: meaningful variable names: bytes_read as discussed above.

Line 504: Do you know that a one second delay is enough to get your response?

Line 551 and 553 could be combined to   SetCtrlVal(chiller,CHILLER_TEMP_MON, atoi(&read_data[11]));

Lines 580 and 585: Why are you storing the number of characters written by sprintf?

Line 607: Why pass bytes_sent and bytes_read to DisplayRS232Error() if you never use them?

Lines 616: call GetRS232ErrorString() as discussed earlier.

Line 687: Is CONFIG_FILE defined in a .h file?  Does it start with a "\"?  The directory returned by GetDir does not end with "\" unless it's the root directory.  You need to make sure that a "\" separates the directory name with the file name when you're done with strcat().

[Al_S]

Sorry for a few typos in my long-winded response.

First paragraph should be:

I would probably leave a do-nothing case 0 just in case at some point DisplayRS232Error() gets called without checking RS232Error.

Line 192 comment should be:

Line 192: So if you get any response back after sending a SET_POINT_QUERY, your instrument is not attached?

[RobertoBozzolo]

As a last addition to the detailed list provided by Al, I would probably jump out in case of an error in RS232 communication: it seems useless to me continuing on the serial port after a serious error has occurred. Additionally, I would prefere not to have a global variable holding the error: you could use a local variable and pass it to the display function instead of the unused parameters:

// Function definition 

void DisplayRS232Error (int error);

int SomeFunction (void)

{

   int    RS232error = 0;

   // some code here

   RS232error = ComWrt (port, message, bytes);

   if (RS232error < 0) goto Error;     // Terminate function

   // Other code here

Error:

   if (RS232error < 0)

      DisplayRS232Error (RS232error);

   }

   return RS232error;

}

[darnell]

ok this is new version which im using the  GetRS232ErrorString

it works well. but come to find out i  still got to use more if statements, im trying to prevent from using if statements

in my main , I got to do another check on top the check thats in the function call below. so is there a unique to do that. 

using typedef structs, or unions,  below im going to post my main so you can see what im talking about.

[darnell]

continue

[Al_S]

For future posts, please post your code as .c files, not as .docx files.

You call GetRS232ErrorString(), but you never do anything with the string it returns: RS232Error.  You should include the RS232Error string in the string that you are reporting.

Something more like this:

sprintf(ErrorMessage,"\n\n RS232 Error %d occured:\n%s\n\n",status_check, RS232Error);

What are you trying to do with the first if statement in DisplayRS232Error?

Line 7: if{

...

}

If what?

Line 17: Don't put new-line characters into the title of your message box.  They won't be displayed as new-lines.

Why do you want to avoid if statements?  Why do you think people can't understand them (if they're properly used, not like your line 7)?

There's nothing wrong with if statements, and there's nothing wrong with the way you're using them in the last code you posted, other than line 7.

If you want the developer to do something with any error message that's generated, you may want to include some additional information.  LabWindows has macros that return the source file name, the function name, and the line number.  You can do something like this:

// calling routine

//...

DisplayRS232Error(-3, __FILE__, __FUNCTION__, __LINE__);

//...

void DisplayRS232Error (int status_check, char* file_name, char* function_name, int line_num)
{                                                                                

  char *RS232Error=0;

  char ErrorMessage[256]={0};

  if (status_check<VI_SUCCESS)
  {
     RS232Error=GetRS232ErrorString(status_check);

     sprintf(ErrorMessage, "Error %d \"%s\"\noccurred in file %s, function %s, line %d",
                status_check, RS232Error, file_name, function_name, line_num);

     MessagePopup ("RS232 Error Message", ErrorMessage);

     DEBGPRNT(ErrorMessage);
  }

}

[RobertoBozzolo]

In the way of simplification, you don't even need to save the error string to a variable: simply use it inside the sprintf statement!

sprintf(ErrorMessage, "Error %d \"%s\"\noccurred in file %s, function %s, line %d",
                status_check, GetRS232ErrorString(status_check), file_name, function_name, line_num);

But is seems to me that we are running in the exact way that I have described in my contribution on error checking. You told you have tried integrating my suggestions but have found several errors: maybe you should try again now and tell us which errors you have found so that I can help you in this instead of reinventing something already written and discussed.