Hopefully simple ISR issue

So I’m building a circuit to control a stepper motor for an outdoor shade. I have buttons wired up for: up, stop, and down.
When either the up or down buttons are pressed, a function rotates the motor in the correct direction for x number rotations, to fully up or fully down. I have the stop button wired to pin 2 on my Arduino pro mini (5v/16Mhz), and an attached interrupt so that when the stop button is pressed, the motor is halted where it is.
To achieve the halt, my ISR simply sets an active flag to false, and in my “rotate” function, I check for the active flag during every iteration of a loop, so that I can stop the loop immediately.

The problem I seem to be having is that when I press the up or down button, the active flag gets set to true, but then immediately is set to false. Even if I set it to true initially, and remove all references to it going to false, except in the ISR, it still happens, which makes me think the ISR is being triggered when it shouldn’t be.

Could someone please take a look at my (very simple) code and tell me why I’m an idiot?

Paul

int DOWN = 8;
int STOP = 2;
int UP = 6;

int DIR = 10;
int STEP = 11;
int ENABLE = 12;

int SPR = 200; // Steps per revolution
int REV = 10; // Number of revolutions

int counter = 0;
volatile bool active = false;

void stopRotation() {
    active = false;
}

void shadeUp() {
    active = true;
    digitalWrite(ENABLE, LOW); // Set Enable low
    digitalWrite(DIR, HIGH); // Set Dir high

    Serial.println("Shade going up");
    rotate();
}

void shadeDown() {
    active = true;
    digitalWrite(ENABLE, LOW); // Set Enable low
    digitalWrite(DIR, LOW); // Set Dir high

    Serial.println("Shade going down");
    rotate();
}

void rotate() {
    Serial.println("Active: " + String(active)); // This always reads as false (zero)
    for (int x = 0; x < SPR * REV; x++) {
        if (active) {
            digitalWrite(STEP, HIGH);
            delay(5);
            digitalWrite(STEP, LOW);
            delay(10);
    
            if (x % SPR == 0) {
                counter ++;
                Serial.print("Rotation ");
                Serial.println(counter);
            }
        } else {
            Serial.println("Stopping!");
            break;
        }
    }
    counter = 0;
}

void setup() {
    Serial.begin(9600);
    
    pinMode(DOWN, INPUT_PULLUP);
    pinMode(STOP, INPUT);
    pinMode(UP, INPUT_PULLUP);

    pinMode(DIR, OUTPUT);
    pinMode(STEP, OUTPUT);
    pinMode(ENABLE, OUTPUT);

    digitalWrite(DIR, LOW);
    digitalWrite(STEP, LOW);
    digitalWrite(ENABLE, LOW);

    attachInterrupt(digitalPinToInterrupt(STOP), stopRotation, RISING);
}

void loop() {
    if (digitalRead(DOWN) == LOW) {
        shadeDown();
    } else if (digitalRead(UP) == LOW) {
        shadeUp();
    } else {
        active = false;
        delay(50);
    }
}

There should be no need to use an ISR for a button that is pressed by a human - humans are verrryyy sllloooowwww.

Just check the button between steps.

Separately, it is not a good idea to use the String (capital S) class on an Arduino as it can cause memory corruption in the small memory on an Arduino. This can happen after the program has been running perfectly for some time. Just use cstrings - char arrays terminated with '\0' (NULL).

...R

For me, you sketch is working fine; there is no problem in the UP and DN sequence; the interrupt is also working well.


Figure-1: Normal UP/DN operation

udint.png
Figure-2: Interruption

Be sure that you have a pull-down resistor (2.2k - 4.7k) connected between DPin-2 and GND.

udint.png

To achieve the halt, my ISR simply sets an active flag to false, and in my "rotate" function, I check for the active flag during every iteration of a loop, so that I can stop the loop immediately

So it would work the same if you just checked the pin in that same function.

If this is an emergency stop then an emergency stop should NEVER rely on code. Emergency stops should be hard wired.

I also noticed that the stop button is the only one you don’t have the pull-up turned on for. Why not?

Delta_G:
I also noticed that the stop button is the only one you don’t have the pull-up turned on for. Why not?

The trigger level of the interrupting signal is set at RISING edge; therefore, the interrupt pin (DPin-2) must have external pull-down.

attachInterrupt(digitalPinToInterrupt(STOP), stopRotation, RISING);

Yeah. Just wondering why that one button is different.

OP could configure all the buttons in the same fashion by enabling the internal pull-up for the interrupt pin (DPin-2), connecting the other end of the interrupting device (the STOP button) at GND and then setting the trigger level (Mode) to FALLING or LOW.
upInt.png

upInt.png

Thank you all for your comments.
I have all the buttons tied to ground with 10k resistors, but not the inputs directly, so:

GND->10K->BUTTON<-DPINs …does that matter? I guess it might because until the buttons are pressed, they are dangling, yes?

The only reason the pullup is different on the STOP button is because that’s on an ISR pin and the others aren’t (this is on a Nano). I wasn’t sure whether that would need to be that way or not, and when I posted my code it was being tested as not input_pullup.

The only reason I don’t do the read in the loop, and use the ISR instead, is because of operational time. Setting a boolean flag, and reading that is far more efficient from a code POV than calling a read operation. That being said, this is not an emergency stop, so while my statement may be true, it might also be moot.

Again, thanks for the comments, I think I’ll remove the ISR altogether and just read the pin in the loop.

Yeah. You got your buttons wired wrong. For input pull-up it goes ground —button — pin. No resistor needed.

And the pin will read LOW when pressed.

The interrupt pin should be the same and should be set for FALLING

Actually, this shouldn’t need an interrupt. Just read the pin at the point where you’re currently checking the flag. The response time is the same. Button presses are slow slow slow at computer speed.

Thank you Delta

I notice that you are not doing button debouncing. This introduces that possibility that noise transients could be misinterpreted. On the whole, button inputs that are not debounced should not be trusted.

Look at Arduino Playground - LibraryList

As long as the buttons are debounced, I agree that the ISR is unnecessary. Since debouncing takes typically 10ms, it is as close to “instantaneous” as one could hope (again, Emergency Stop needs to be hardwired). There are over a dozen different debouncing libraries given, each with its own strengths and weaknesses. I prefer those that do not stop loop() while the debouncing is happening.
joe

The buttons in this context are purely for test purposes. The buttons that will actually be used come from a 3 gang wifi switch, to match the design of the gazebo where this will be used. Those buttons have all the hardware they'll need to be noiseless, built into their own architecture.
That way I have remote access to this circuitry as well as manual access.

Thx

For the OP's project, only the FIRST switch contact is the only one that needs to be effective. What he does need to take care of is not being able to press more than one switch at a time.

Paul