I'm in the car, so will answer later. But for your consideration, I'd reframe your requirement. Staying in the State_Alarm check and continuing to check the ADCs may be at cross purposes.

I can't read your source now, so have to be general.

 

Since you keep talking about the "state" of the system I would stngly suggest you build this on a state machine model.

That means you have one variable that is the current state. Typically an enumeration variable is used for this purpose:

enum MajorStates{Normal, Short, Open, Alarm};
enum MajorStates MajorState = Normal;

Your main loop will looks something like this:

    while(1)
    {
         ConvertADC();                          //Start converting
         while(BusyADC());                      //Wait while ADC is converting
         adc_result = ReadADC();                //Read the ADC result and store it in variable
         // CloseADC();                            //Close the ADC
 
 
    switch (MajorState)
    {
        case Normal:
            // do something useful with adc_result
            break;
        case Short:
            // do something useful with adc_result
            break;
        case Open:
            // do something useful with adc_result
            break;
        case Alarm:
            // do something useful with adc_result
            break;
    }
}

By knowing the state at all times the switch/case code can only allow certain state transistions (like never going back to Normal).

It is possible you need more states than this, as you may want both Alarm and say Alarm_&_Open.

 

You also need to look at the constant data in the if statements. First I would use #define or even better use a const declaration on each to give it a human readable label. This allows the compiler to type check data. In your code I see integer to float comparisons that don't really do what you might think. (adc_result<0.20) Do try to avoid mixing integers and floating point.

Example for const data:
const int Alarm_Low_Voltage=96;

 

Yeah I noticed those too nasspook, I forgot to try some to make sure it's not some arcane but legal C I'm not familiar with (or just a misplaced float constant).

If this is the same XLCD as I am familiar with the "while(BusyXLCD());" statement/loop must be called before any other command is called to insure the hardware is free. I see some calls where this is not so.

Calling this is necessary to make sure the LCD works correctly every time. As there is little penality for calling it when not required I modified the library by inserting a "while(BusyXLCD());" into all the other routines.

If I never need to call it then I will never forget to call it.

 

Yeah I noticed those too nasspook, I forgot to try some to make sure it's not some arcane but legal C I'm not familiar with (or just a misplaced float constant).

It's perfectly 'legal' but wrong as the adc value can't be a fraction even if it was promoted to float. He needs to calculate and define an integer value that's equivalent to the fractional voltage reading and use that.

Another (better) way is to have a function to convert all ADC readings to a 32bit integer in millivolts and use those for calculations instead of raw ADC values.

 

Why do the conversion? It is additional processing and possibly a loss in precision. If the conversion results in a greater range, then error in the accuracy is increased. If the conversion results in a smaller range, then there is a loss in precision.

For self-documentation purposes as someone has also mentioned, the use of define statements could make the program more readable, without a loss of precision or lack of accuracy.

I only perform a conversion when interfacing with a library that requires a differently scaled input.

You could define the values as follows:

Define 200mv 432​

Along this lines, something else I noticed. Your program tests for an exact value to detect a short or open (e.g., inputvalue == 538). It would be better to test that the absolute value of the currently read ADC value and the target value is within some constant.
\(\small|ADC value- test value|< delta\)

 

If you want to lock the system into the alarm state executing just the alarm state code, then just loop it without a break.
something along the lines of this:

alert_state(){
while(1){
code goes here
}
}

This way, once it enters the alert state, it never does any other function, just stuck in that loop, until the processor is reset.

 

Why do the conversion? It is additional processing and possibly a loss in precision. If the conversion results in a greater range, then error in the accuracy is increased. If the conversion results in a smaller range, then there is a loss in precision.

For self-documentation purposes as someone has also mentioned, the use of define statements could make the program more readable, without a loss of precision or lack of accuracy.

In general I would agree if the only use was for a level detector but with the LCD I assume there will be at least some sort of electrical parameter readout and by using real voltage/current/resistance units for all constant and variable data operations in the program the readability (maybe fewer bugs) of the code will be improved. With some simple over-sampling (yes, additional processing) the actual precision of each millivolt reading might be improved over a single ADC level reading.

 

In general I would agree if the only use was for a level detector but with the LCD I assume there will be at least some sort of electrical parameter readout and by using real voltage/current/resistance units for all constant and variable data operations in the program the readability (maybe fewer bugs) of the code will be improved.

Ok. I think we agree. I did mention that in the case of interfacing with other libraries scaling would make sense. But personally in the scenario you propose, I'd only scale in a display function. I'd use the raw values elsewhere. to address readability and maintainabilty, I would and have liberally used defines, for ALL constants. To me "200mv" is clearer than "538". But you know that.

 

Another reason is that he's using C18 with no (not ANSI compliant) integer promotion by default. You have to be very careful using native 16bit integers.
http://www.xargs.com/pic/c-faq.html#bytestoint
http://www.xargs.com/pic/c-faq.html#preproc

There's nothing wrong with C18 if you know it's quirks and the one Linux version they released 3.40 (you can find it on the web in public domain) has full optimizations, never expires and works with all Linux versions of MPLABX. For a new project I wouldn't use it but I have lots of old programs that do.

 

Ok. I think we agree. I did mention that in the case of interfacing with other libraries scaling would make sense. But personally in the scenario you propose, I'd only scale in a display function. I'd use the raw values elsewhere. to address readability and maintainabilty, I would and have liberally used defines, for ALL constants. To me "200mv" is clearer than "538". But you know that.

It's a personal preference but I like to limit the use of raw unscaled data that represents scientific units of measure unless it's needed at the binary interface level.

 

Hey guys,

I got it working basing my new code on djsfantasi example, I added a few nested loops inside the IF statements and I have just run the code on my target board, I have been testing it and seems to work surprisingly good!

Here is the code (I will just paste the main, therefore the rest is practically intact):

/*MAIN PROGRAM*/
void main(void)
{
    ADCON1=0x0E;    //ADC_CH0 as analog input
    TRISC=0x00;     //PORTC as digital outputs

    Delay10KTCYx(255);
   
    initialize_LCD();   //Initialize LCD

    INTCONbits.GIE=0; //Disable all interrupts

    config1 = ADC_FOSC_16 & ADC_RIGHT_JUST & ADC_16_TAD ;
    config2 = ADC_CH0 & ADC_INT_OFF & ADC_VREFPLUS_VDD & ADC_VREFMINUS_VSS ;
    portconfig = ADC_1ANA;

    while(1)
    {
         OpenADC(config1,config2,portconfig);   //Open the ADC converter
         ConvertADC();                          //Start converting
         while(BusyADC());                      //Wait while ADC is converting
         adc_result = ReadADC();                //Read the ADC result and store it in variable
         CloseADC();                            //Close the ADC converter

            if(adc_result<=295 && adc_result>=0.20) //ADC result for alarm condition, line current aprox. 25mA
            {
                exceeded_alarm = 1;     //Set alarm threshold exceeded flag to 1
               
                if(exceeded_alarm != triggered_alarm)   //If exceeded alarm is != to trigger alarm...
                {
                    while(exceeded_alarm == 1 )     //While the exceeded alarm flag is 1...
                    {
                        state_alarm();     //Execute function state_alarm();

                        if (adc_result>=531)   //ADC result for open circuit, line current = 0mA
                        {
                             state_opencircuit();   //Execute the open circuit function
                        }

                        else if(adc_result<0.20)     //ADC result for short-circuit, line current > 100mA
                        {
                             state_shortcircuit();  //Execute the short circuit function
                        }

                        triggered_alarm = 1;     //Set triggered flag to 1 so the function is not repeated
                    }
                }
            }

            else if(adc_result>295 && adc_result<531) //ADC result for normal condition, line current < 25mA
            {
                state_normal();     //Execute the normal state function
            }

            else if (adc_result>=531)   //ADC result for open circuit, line current = 0mA
            {
                exceeded_open = 1;      //Set open circuit threshold flag to 1
               
                if (exceeded_open != triggered_open)       //If exceeded open is != to triggered open...
                {
                    while(exceeded_open == 1)   //While the exceeded open flag is 1...
                    {
                        state_opencircuit();

                        if (adc_result<=295 && adc_result>=0.20)
                        {
                            state_alarm();      //Execute the alarm state function
                        }

                        else if (adc_result<0.20)
                        {
                            state_shortcircuit();      
                        }
                    }
                }
            }

             else if(adc_result<0.20)     //ADC result for short-circuit, line current > 100mA
             {
                 exceeded_short = 1;      //Set open circuit threshold flag to 1

                if (exceeded_short != triggered_short)       //If exceeded open is != to triggered open...
                {
                    while(exceeded_short == 1)   //While the exceeded open flag is 1...
                    {
                        state_shortcircuit();

                        if (adc_result<=295 && adc_result>=0.20)
                        {
                            state_alarm();      //Execute the alarm state function
                        }

                        else if (adc_result>=531)   //ADC result for open circuit, line current = 0mA
                        {
                            state_opencircuit();       //Execute the short cirrcuit function
                        }
                    }
                }
            }
    }
}

Thank all of you for your help!

 

Hey guys,

I got it working basing my new code on djsfantasi example, I added a few nested loops inside the IF statements and I have just run the code on my target board, I have been testing it and seems to work surprisingly good!

Thank all of you for your help!

Glad it's working. You might want to use the suggestions from this thread on ways to improve your codes operation.