What's the correct loop structure?

[radhika]

Hello,
I have been working on a menu code from last many days.
The code is written in MPLAB IDE v8.92 and compiler is CCS C.
This menu code is built for PIC18F4520 with 3 button i.e. up, down, select.
I have compiled the code. But, I am facing certain problem.
WHAT I WANT : I wan't to dispaly;
1. ON
2. OFF
3. Back
on OLED SSD1306.
But, when I added this do{}while loop in the code, I only get :
1. ON
as the output but not 2. OFF and 3. Back
This the attached small part of the code:

Code: [Select]

void charging_AC() {
     
        SSD1306_ClearDisplay();
        cursor(starting_row);
        SSD1306_GotoXY(starting_col, starting_row);
        SSD1306_PutC("1. ON");
        delay_ms(10);

if (button_select = 1){
do
        {
           output_high(PIN_A0);
           } while (1);
button_select = 0;
}

        start_row = starting_row;
        start_row +=1;
        SSD1306_GotoXY(starting_col, start_row);
        SSD1306_PutC("2. OFF");
        delay_ms(10);
if (button_select = 1){
do{
           output_low(PIN_A0);
           } while (1);
button_select = 0;
}
           

        cursor(start_row);
        start_row +=1;
        SSD1306_GotoXY(starting_col, start_row);
        SSD1306_PutC("3. Back");
        delay_ms(100);
        row_Ind = 1;
  }

[oPossum]

This is an assignment, not a compare, and will always evaluate to true.

Code: [Select]

if (button_select = 1){


[rstofer]

So, we write tests for equality backwards to prevent the kind of mistake you have

if (1 == button_select) {
}

This will compile as a conditional expression but never as an assignment statement.
The compile will choke on something like:

if (1 = button_select) {
}

That statement

do {
  output_high(PIN_A0);
} while (1);

is never going to leave the loop.  while(1) means FOREVER or until you push the reset button.


Why wouldn't

if (1 == button_select) {
  output_high(PIN_A0);
}

work?

Reversing the test as shown above will work on most compilers.  I don't know if it works on CCS.

[agehall]

Doesn't MPLAB come with some sort of debugger? Running the code in a debugger or even a simulator would show you a lot of the problems right away.

[radhika]

So, we write tests for equality backwards to prevent the kind of mistake you have

if (1 == button_select) {
}

This will compile as a conditional expression but never as an assignment statement.
The compile will choke on something like:

if (1 = button_select) {
}

That statement

do {
  output_high(PIN_A0);
} while (1);

is never going to leave the loop.  while(1) means FOREVER or until you push the reset button.


Why wouldn't

if (1 == button_select) {
  output_high(PIN_A0);
}

work?

Reversing the test as shown above will work on most compilers.  I don't know if it works on CCS.

Hello,
Thanks for the suggestion.
I did some changes as you suggested. Please have a look on code.

Code: [Select]

void charging_AC() {
     
        SSD1306_ClearDisplay();
        cursor(starting_row);
        SSD1306_GotoXY(starting_col, starting_row);
        SSD1306_PutC("1. ON");
        delay_ms(10);
button_select ==0;
if (input_state(PIN_D2) == 0 && row_Ind ==1)
        {do{
           output_high(PIN_A0);
           }while(input_state(PIN_D2) != 0 && row_Ind !=1);
   }       


        start_row = starting_row;
        start_row +=1;
        SSD1306_GotoXY(starting_col, start_row);
        SSD1306_PutC("2. OFF");
        delay_ms(10);
button_select == 0;
if (input_state(PIN_D2) == 0 && row_Ind == 2)
{do{
           output_low(PIN_A0);
           } while(input_state(PIN_D2) != 0 && row_Ind !=2 );
}


           

        cursor(start_row);
        start_row +=1;
        SSD1306_GotoXY(starting_col, start_row);
        SSD1306_PutC("3. Back");
        delay_ms(100);
        row_Ind = 1;
  }
I got the required result.
BUT THEIR IS STILL AN ERROR:
The error is, My led is remain glow when the "ON" is selected and even when the "OFF" is seleted.
How do I switch off the led when OFF is selected and it remain ON when "ON" is selcted.
https://postimg.cc/xcVhftmm
https://postimg.cc/kRsQdJ8f

[rstofer]

I don't see how you break out of those do-while loops. Or maybe you are breaking out every pass.

row_Ind !=1 is used to stay in a do-while loop but it doesn't change while inside the loop.

Code: [Select]

if (input_state(PIN_D2) == 0 && row_Ind ==1)
        {do{
           output_high(PIN_A0);
           }while(input_state(PIN_D2) != 0 && row_Ind !=1);
   }

We know row_Ind == 1 or we wouldn't enter the loop.  So when we test row_Ind != 1, we know it is false (because it was just == 1) and the expression is false and the test fails so we leave the while loop immediately. 

I have no idea what row_Ind does but just because it is required to be == 1 to enter the loop doesn't mean you have to test it later in the loop.  You are looking for the button press to go away, what does row_Ind have to do with that?
 

[radhika]

I don't see how you break out of those do-while loops. Or maybe you are breaking out every pass.

row_Ind !=1 is used to stay in a do-while loop but it doesn't change while inside the loop.

Code: [Select]

if (input_state(PIN_D2) == 0 && row_Ind ==1)
        {do{
           output_high(PIN_A0);
           }while(input_state(PIN_D2) != 0 && row_Ind !=1);
   }

We know row_Ind == 1 or we wouldn't enter the loop.  So when we test row_Ind != 1, we know it is false (because it was just == 1) and the expression is false and the test fails so we leave the while loop immediately. 

I have no idea what row_Ind does but just because it is required to be == 1 to enter the loop doesn't mean you have to test it later in the loop.  You are looking for the button press to go away, what does row_Ind have to do with that?
 

Code: [Select]

void charging_AC() {
     
        SSD1306_ClearDisplay();
        cursor(starting_row);
        SSD1306_GotoXY(starting_col, starting_row);
        SSD1306_PutC("1. ON");
        delay_ms(10);
        start_row = starting_row;
        start_row +=1;
       
        if (input_state(PIN_D2) == 0 && output_low(PIN_A0))
           {
           output_high(PIN_A0);
           }
         delay_ms(500);
               

        SSD1306_GotoXY(starting_col, start_row);
        SSD1306_PutC("2. OFF");
        delay_ms(10);

           

        cursor(start_row);
        start_row +=1;


if (input_state(PIN_D2) == 0 && output_high(PIN_A0))
{
           output_low(PIN_A0);
           }
delay_ms(500);



        SSD1306_GotoXY(starting_col, start_row);I tried this changes. But i still don't got the required result. Suggest some changes or the code to make it work.

[radhika]

Any suggestion?

[donotdespisethesnake]

It seems you had a working function to display a menu, then added some bugs to it.

Don't try to mix different actions in the same function. I would go back to the version which displays the menu, before you changed it.

Then, explain why you are trying to change it. Are you trying to handle button presses?

[rstofer]

Code: [Select]

void charging_AC() {
     
        SSD1306_ClearDisplay();
        cursor(starting_row);
        SSD1306_GotoXY(starting_col, starting_row);
        SSD1306_PutC("1. ON");
        delay_ms(10);
        start_row = starting_row;
        start_row +=1;
       
        if (input_state(PIN_D2) == 0 && output_low(PIN_A0))
           {
           output_high(PIN_A0);
           }
         delay_ms(500);
               

        SSD1306_GotoXY(starting_col, start_row);
        SSD1306_PutC("2. OFF");
        delay_ms(10);

           

        cursor(start_row);
        start_row +=1;


if (input_state(PIN_D2) == 0 && output_high(PIN_A0))
{
           output_low(PIN_A0);
           }
delay_ms(500);



        SSD1306_GotoXY(starting_col, start_row);I tried this changes. But i still don't got the required result. Suggest some changes or the code to make it work.

Why do you make unnecessary tests?

if (input_state(PIN_D2) == 0 && output_high(PIN_A0))
{
           output_low(PIN_A0);
           }

just set the pin low if D2 == 0

Code: [Select]

if (input_state(PIN_D2) == 0)
{
           output_low(PIN_A0);
           }
Same story with

        if (input_state(PIN_D2) == 0 && output_low(PIN_A0))
           {
           output_high(PIN_A0);
           }

Sometimes, when you post code to a forum, you need to pull out the stuff that is irrelevant and probably known to work.  Just replace it with a comment like // code removed - displays "ON" on the screen - known to work

Your problem seems to be flow control, not driving the screen.  Don't test things that don't change or aren't actually required.

Code: [Select]

void charging_AC() {

// clear display and set to show "ON" - code removed, known to work
       
    if (input_state(PIN_D2) == 0)
    {
        output_high(PIN_A0);
    }
    delay_ms(500);

    // change display to show "OFF" - code removed, known to work

    if (input_state(PIN_D2) == 0)
    {
        output_low(PIN_A0);
    }
        delay_ms(500);
}
People reading through your code don't want to be overwhelmed by detail.  In your case, it's not too bad because it's pretty easy to skip over blocks of irrelevant code that is presumed to work.  We don't actually KNOW that it works but probably.

I would think the stripped down code would work.  To test it, I would write a little program with the bare minimum logic and use the LED output.  I don't know how it should work and I don't know why there is a 1 second delay.

When I look at the reduced code I notice:

• Every time I enter the function, "ON" will be displayed - unconditionally
• If D2 is low, A0 is set high
• The program stalls for 1/2 second
• "OFF" will be displayed - unconditionally
• If D2 is low (it could have changed during the delays) A0 is set low otherwise, it is left high until next time
• The program stalls for 1/2 second

If A0 is just a test indicator, I would set it low unconditionally before exit.  I don't want to make an unnecessary test or abandon a signal.  Do I really care if the operator released the button during the delays?

Don't do 'while' loops unless you know how to get out of them.  Don't make unnecessary tests.  If a thing could be done unconditionally, do it.  I suspect you don't want the display to go to "ON" and then unconditionally go to "OFF" 1/2 second later.

It helps to write down the exact sequence to be follow in some kind of spoken language "If this is true, I want this to happen" or some kind of pseudo code.  Get the flow correct and then worry about coding it in C.

I just don't see the point of the code:  First it displays "ON" then 1/2 second later is displays "OFF" - unconditionally.  First it sets A0 high but if the button is released early, it leaves it that way.

I really don't understand what you are trying to accomplish so I really can't provide any guidance other than dumb replies like "If this is true...".  It may be the case that your flow control is correct but I don't think so.

[rstofer]

Another way to simplify code is to pull invariant blocks out into functions like

Code: [Select]

void DisplayON(void)
{
        SSD1306_ClearDisplay();
        cursor(starting_row);
        SSD1306_GotoXY(starting_col, starting_row);
        SSD1306_PutC("1. ON");
        delay_ms(10);
        start_row = starting_row; // <==== this assumes start_row and starting_row are global
        start_row +=1;
 }


void charging_AC() {
        DisplayON();
... the other code ...
}
       
The idea is to break down blocks of code into smaller and simpler chunks.  Pretty soon the actual actions will emerge as I show in my reduced code version above.