1
\$\begingroup\$

I'm making a program for the msp430 using energia launchpad, the first code that I had is the following:

// display flag
boolean flag_display = false;

void setup() {
  // define ISR to activate the display
  attachInterrupt(PUSH2, display_ISR, CHANGE);
}

void display_ISR() {
  clean_leds();
  flag_display = !flag_display;
}

void loop() {
  if(flag_display == true) {

    // prints the number 45 on the display
    pick_digit(1);
    pick_number(4);
    delay(8);
    pick_digit(2);
    pick_number(5);
    delay(8);

  }
}

Basically the program presents the number 45 on the display every time the button PUSH2 is pressed and cleans the display every time the button is un-pressed, the problem is that sometimes (when the button is un-pressed and the program is still on the first delay) the number 5 in the second digit stays present, so I used the following to bypass this problem:

// display flag
boolean flag_display = false;

// delay flag from the display
boolean flag_delay = true;

void setup() {
  // define ISR to activate the display
  attachInterrupt(PUSH2, display_ISR, CHANGE);
}

void display_ISR() {
  flag_display = !flag_display;
}

void loop() {
  if(flag_display == true) {

    // prints the number 45 on the display
    pick_digit(1);
    pick_number(4);
    delay(8);
    pick_digit(2);
    pick_number(5);
    delay(8);

    // if the code runs till the end, un-flag the delay flag
    flag_delay = false;

  }

  // Keeps cleaning the leds until the delayed code is over processing
  if(flag_display == false && flag_delay == false) {
    clean_leds();
    flag_delay = !flag_delay;
  }
}

But I read that over using flags in your program is a bad programming practice and my solution doesn't seem elegant nor power efficient, since the line:

flag_delay = false;

Is going to keep being processed every time the display is on, and the function clean_leds() is going to keep being processed until the delayed code is over. Is there a more efficient way of using the ISRs and flags in this particular case? Thank you.

The clean_leds() function is has follows:

void clean_leds()
{
  digitalWrite(P1_7, LOW);
  digitalWrite(P1_6, LOW);
  digitalWrite(P2_5, LOW);
  digitalWrite(P2_4, LOW);
  digitalWrite(P2_3, LOW);
  digitalWrite(P2_2, LOW);
  digitalWrite(P2_1, LOW);
}
\$\endgroup\$
  • \$\begingroup\$ What does clean_leds() actually do? I can't find docs on it. \$\endgroup\$ – jonk Mar 22 '14 at 20:53
  • \$\begingroup\$ It's a function, created by me, that simply cleans the LEDs from the display. \$\endgroup\$ – Rui Lima Mar 23 '14 at 1:32
  • \$\begingroup\$ I'm not sure what that does. And it may matter. I think toggling a flag in an interrupt routine that is called on both edges of the pushbutton may be okay. But if so, I don't think the loop should change it. Often, an interrupt routine sets a flag that is later cleared by code that handles it in loop() as part of an if statement. But you may not need that in your case. I guess I wanted to see that code, as well, before composing some answer. \$\endgroup\$ – jonk Mar 23 '14 at 2:14
  • 1
    \$\begingroup\$ Changed the question to show the clean_leds() function code. \$\endgroup\$ – Rui Lima Mar 23 '14 at 11:03
2
\$\begingroup\$

I haven't worked with this processor yet but I am 100% sure there is some way to track the push button transition states

i.e un_pressed_button(low) - > pressed_button (high) transition and the vice versa

If you do not want to use the flag you need to check for the

pressed_button(high) -> un_pressed_button(low) transition on the push-button

sudo code would be like,

if(current_state_of_button == LOW && previous_state_of_button == HIGH)
{
    clean_leds();
}

you will have to keep track of the states with two variables. OR setup two ISRS that fire on the two possible transitions. Hope this helps

\$\endgroup\$
  • \$\begingroup\$ The flag_display variable already keeps track when the button is pressed/un-pressed. What bothers me is that I need a second flag (flag_delay) to prevent the code pick_digit(2) and pick_number(5) to be processed after the button is un-pressed (because, in the first example, if by chance I un-press the button between the 8 milliseconds of that delay timeframe that's what happens). \$\endgroup\$ – Rui Lima Mar 23 '14 at 11:57
  • 1
    \$\begingroup\$ Oh that is simple you can just disable the interrupts while performing a critical section (section you don't want to be interrupted by push_button release action) To disable interrupts: cli(); // disable global interrupts and to enable them: sei(); // enable interrupts \$\endgroup\$ – Dexobox Mar 23 '14 at 18:06
  • \$\begingroup\$ the flow of your code is very messy here is a proposed flow, again i dont think that you understand the gravity of keeping two variables (current_state_of_button and previous_state)Its something which is requried if you want to get rid of the flag. The loop() should just scan the button for changes, if button_pressed ISR1 should fire , the routine for ISR1 should contain the code to pick_digit() and delay and this ISR cannot be interrupted you may disable the interrupts when entering the ISR. Once it is out of the ISR it should check if the prev_state == pressed and current_state==unpressed \$\endgroup\$ – Dexobox Mar 23 '14 at 18:14
  • \$\begingroup\$ and clean_lcd() \$\endgroup\$ – Dexobox Mar 23 '14 at 18:14
1
\$\begingroup\$

Here is a psudo code for what i am trying to explain

bool prev_state
bool current state

setup{
//attach ISR 
}
ISR1.subroutine{
cli();   //disable interrupts
//Print your digits to LCD
sei();   //enable interrupts
}


loop{

 if(prev_state == HIGH && current_state == LOW)
 {
    //if this routine takes time also you might want to disable the interrupts here too
clead_leds();
 }
\$\endgroup\$
1
\$\begingroup\$

There are a number of issues in your original code and I think a misunderstanding of ISRs.

Your original code can be fixed by removing the clean_leds() from the ISR and moving it to the loop routine. There is no need for flag_delay at all.

if(flag_display == true) 
{
   // prints the number 45 on the display
   pick_digit(1);
   pick_number(4);
   delay(8);
   pick_digit(2);
   pick_number(5);
   delay(8);
}
else
{
   clean_leds();
}

The ISR (interrupt service routine) will literally interrupt any existing code unless interrupts have been disabled (as explained by Dexobox).

This means that clean_leds() may run after the number 4 is displayed but before the number 5 leaving only '5' on the display rather than '45'. As a general rule if you are going to render '45' in the main loop then you should also be clearing it in the main loop. This reduces the possibility of race conditions and unintended behaviour.

Alternatively you can clean_leds() and draw the '45' in the ISR but given the drawing is using delays and may take some time I would not recommend it.

\$\endgroup\$
  • \$\begingroup\$ I agree, but wouldn't, in this case, the function clean_leds() continuously try to clean the LEDs even after the displays where off? Wouldn't that consume more power? \$\endgroup\$ – Rui Lima Mar 24 '14 at 8:45
  • 2
    \$\begingroup\$ It all depends what clean_leds() actually does but if you only want to run it once then perhaps a better alternative to boolean flags is a state machine enum or something similar. This way you can go between three states - 1: Display, 2: Clean, 3: Nothing. After calling your display or clean logic you just transition to the nothing state which could put your processor into some kind of lower power sleep mode or just spin in the main loop. \$\endgroup\$ – Lummo Mar 24 '14 at 9:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.