help simplifying function

[Al_S]

Darnell:

About your statement that the compiler is catching the -7 error, but your switch statement is not catching it:

The problem is not in your switch statement: the problem is that you are overwriting status_check before you get to the switch statement.

You set status_check with OpenComConfig at line 27, but then you overwrite status_check with ComWrt at line 31 and again with ComRd at line 34 before you get to the switch statement.  

So if OpenComConfig sets status_check to -7, "Cannot open port", ComWrt and ComRd will set it to -3, "Port not open".

By the time you get to your switch statement, status_check is no longer -7.  By single stepping through the code, you would have seen the value of status_check change.

Also remember my earlier comment: if you're using this switch statement as real code, there is no reason to build separate cases for identical actions. 

A couple of other comments:

If you want to manage the error messages instead of having CVI manage them, you can turn off breaking on library errors using

SetBreakOnLibraryErrors(0); 

Just be sure to SetBreakOnLibraryErrors(1); in code where you aren't trapping errors so a helpful error message gets displayed.

About your  OpenComConfig statement at line 27:

  status_check = OpenComConfig (physical_port, "COM10", baudrate,              

                 parity, databits, stopbits, inputq, outputq); 

You went back to specifying both the Port Number and the Device Name.  And, again, they don't match.  Roberto and I both warned you about this repeatedly earlier, for example: http://forums.ni.com/ni/board/message?board.id=180&message.id=42942&query.id=108944#M42942

I don't suppose there's any chance you were a history major.  Are humans doomed to continue repeating past mistakes?

About your DisplayRS232Error() function:

If you're using it as an error handler, typically 0 means no error.  You should allow for that without printing a message, as Roberto suggested earlier.

Also, you are calling DisplayRS232Error after calling ComRd, which returns a positive number of bytes read if no error occurred.  But you display a message for an incorrect number of bytes.  Think about what logic you want here.

For Roberto's earlier comment that you don't need a separate statement to save the RS232Error, you can do this instead:

sprintf(ErrorMessage,                                                

 "RS232 Error %d:\n%s\n\n",status_check, GetRS232ErrorString(status_check));                      

MessagePopup ("RS232 Error Message", ErrorMessage);                  

You say "i was using this three days ago".  So you're asking us to debug code you didn't even post?  Do you understand why there is some frustration with your posts?

[RobertoBozzolo]

Two minor additions to Al excellent comments.

Your error trapping routine should jump out of the serial communications functions, as is obviously useless for example to try using the port if it was not possible to open it. A normal code flow is like the following:

    Serial communications function: returns status code

    Test of status code value: if negative

        Display error

        go to the end

    else

         go on with the code

You didn't place a delay between the write and the read: every device will take some time to execute a command, so you normally cannot expect to be able to read back a response immediately. Now, serial communications functions have  a timeout, so you could rely on this to manage this delay, but you should trim it to match device capabilities. Default timeout value is 5 second, which usually is too long a time for you to wait for the answer, so you would like to reduce this timeout to some milliseconds using SetComTime immediately after OpenComConfig.

In case during your program you need to handle several types of message that will take very different time to execute you may want to disable common timeout value with SetComTime (port, 0); and add a Delay or Sleep instruction after every query with an appropriate time.

[darnell]

so to keep from overwriting do i just each command to a different variable,

do i do something like this

    status_check = OpenComConfig (physical_port, "COM10", baudrate,                    
                              parity, databits, stopbits, inputq, outputq);            
                                                                                       
                                                                                                                                 
    error_check = ComWrt(physical_port,set_point_query,sp_command_size);               
                                                                                       
                                                                                       
    status = ComRd (physical_port, read_data, sp_command_size);

also i want to test for a negative number, and i want to do all my error handling myself.

you were saying my switch statement  shouldnt have nothing in them , i just need to have to cases testing for -3, and -7? explain more im lost              
                                                                                       
 

[darnell]

explain this line and demostate if you dont mind

"Also remember my earlier comment: if you're using this switch statement as real code, there is no reason to build separate cases for identical actions. "

[darnell]

also this tthe code to debug, im really lost at this moment

[RobertoBozzolo]

Hi darnell, I will try commenting your code (only the relevant one), next giving my own solution for you to consider. You will notice that by compacting the code some errors become more evident.

int main()
{
 char ErrorMessage[256]={0};   // Unused
 char *RS232Error=0;     // Unused: moved to DisplayRS232Error ()
 int counter;    // Unused
 char name[80];     // Unused: moved to DisplayRS232Error ()
 char len[256]={0};  // Unused
 int username=0;     // Unused
 int error=0;

 texStart("STTO");

 status_check = OpenComConfig (physical_port, "COM10", baudrate,             
                     parity, databits, stopbits, inputq, outputq);

Please stop setting device name different from port number! Leave it blank!!!

 status_check = ComWrt(physical_port,set_point_query,sp_command_size);  
 status_check = ComRd (physical_port, read_data, sp_command_size);

status_check is overwritten from one call to the others: test its value after each call
  
 switch(status_check)  {
  case -3:
       if (status_check!=sp_command_size ) {                                 
          DisplayRS232Error ( status_check);
          return 0;
       }
       break;

Since you are executing case -3, is evident that status_check!=sp_command_size
     case -7:
       if (status_check!=sp_command_size )  {                                      
          DisplayRS232Error ( status_check);
          return 0;                           
       }                                       
       break;

Same as above
  default:
       if (status_check < 0) {                                                           
        DisplayRS232Error ( status_check);
        return 0;
       }                                                          

       break;
       if (status_check < 0)  {                                      
               DisplayRS232Error ( status_check);
              return 0;
      }                                     
     texFinish (NORMAL_FINISH);            

Unreachable code 

 }
  return 0;
}

I personally would have developed the code this way:

int main()
{
 int error = 0;

 texStart("STTO");

 error = OpenComConfig (physical_port, "", baudrate,             
                     parity, databits, stopbits, inputq, outputq);

 if (error < 0) goto Error;

 error = SetComTime (physical_port, 0);

 if (error < 0) goto Error;

 error = ComWrt(physical_port, set_point_query, sp_command_size);  
 if (error < 0) goto Error;

 Delay (0.05);     // Adjust time as needed

 error = ComRd (physical_port, read_data, sp_command_size);
 if (error < 0) goto Error;

 // Here only if all is successful: test return string

 texFinish (NORMAL_FINISH);            

  
Error:

  if error < 0) DisplayRS232Error (error);

  return 0;
  
}

[darnell]

i step through it and saw that the value would change, i just couldn't find out how to fix the problem. the way you did it,its much easy. is there a way where in i can incorporate the switch, so that i can strengthen my self in using the switch statement. that would help me out alot.

[darnell]

also for my job standards i cant use goto statement. so what can i to use the switch statement and keep from over riding the status check

[darnell]

this way works better, i put my switchment in a function call, which is the switch_error(); but i now i need to get the program to end; after each error.

status_check = OpenComConfig (physical_port, "COM10", baudrate,        
                         parity, databits, stopbits, inputq, outputq);  
                                                                       
switch_errors();                                                       
                                                                       
status_check = ComWrt(physical_port,set_point_query,sp_command_size);     
                                                                       
switch_errors();                                                       
                                                                       
status_check = ComRd (physical_port, read_data, sp_command_size);      
                                                                       
switch_errors();                                                       

[darnell]

                                                            
      this is my c file file ,  any suggestions this way?