Hope this solves any issues you may have...
It does. But let's look at some of your comments.
So I was messing up and used pinMode(Relay, LOW) before and after fixing it, everything works out!
How did you fix this? By making code changes, right? So, we need to see them.
So you can see my code?
Your old code.
Not sure what you mean here either. Does it matter? Is this causing problems with my program?
Maybe it matters. Maybe it doesn't. If you have a function called ReadEncoder() and it uses no global variables, returns a value, and does exactly what the name says, and nothing else, then we can read through the code, determine if it is reasonable, and then forget it. When we later encounter a call to ReadEncoder(), we don't have to worry about what side effects and bizarre things that function is having on the rest of the code.
But, when we encounter a ReadEncoder() function that uses global variables and does several things, then each time a global variable appears in the code, we need to go see if ReadEncoder() is screwing with that variable. That is not conducive to quick debugging.
What does this mean?
It means that if you simply rename your function to admit that it is a grab bag of unrelated functionality, I'm not even going to make an attempt to read it, or help you.
I removed them after I set up my code and got it working,
But, your code isn't working, is it?
char tempString; //Used for sprintf
A bit on the large side, isn't it?
seconds_hold = seconds;
minutes_hold = minutes;
minutes = minutes_hold;
seconds = seconds_hold;
No comments to explain this bizarre code. First, it is not necessary to declare and initialize variables as two separate steps.
int seconds_hold = seconds;
int minutes_hold = minutes;
looks better and is less typing.
Setting seconds and minutes to what they currently contain makes no sense.
sprintf(tempString, "%02d%02d", minutes, seconds);
Serial7Segment.write(0x77); // Decimal, colon, apostrophe control command
Serial7Segment.write(1<<COLON); // Turns on colon, apostrophoe, and far-right decimal
Serial7Segment.print(tempString); //Send serial string out the soft serial port to the S7S
You do this a lot. Why not create a function to do it, once. Then, tempString could be a local variable. And, it would be clear that it was oversized.
if (digitalRead(dpInEncoderPress)== LOW)
mode = Run_Timer;
Why are you delaying?
case (Set_Timer): //reads encoder and sets time
Case values do not need parentheses around them.
if (digitalRead(MagSwitch) == LOW) //if magnetic switch is detected
// switches modes depending on button press
digitalWrite(Relay, LOW); //Relay off
case (Set_Timer): //reads encoder and sets time
case (Run_Timer): //counts down timer
digitalWrite(Relay, HIGH); //relay on
countdownTimer(); //run countdown script (above)
else if (digitalRead(MagSwitch) == HIGH) //if no magnetic switch detected
digitalWrite (Relay, LOW);
minutes = 0;
seconds = 0;
Serial7Segment.print("OFF "); //Send the hour and minutes to the display
mode = Set_Timer;
This is the heart of the program. The first comment is wrong. The Arduino can not tell if the magnetic switch is present or not. All it can tell is whether the pin that the switch is supposed to be attached to is HIGH or LOW.
Then the comment about the switch statement leaves a bit to be desired. Which switch press? There is only one statement in all of loop() that sets mode to a value. It is not clear where else mode gets set.
Basically, one of three functions gets called. ReadEncoder() to get the time to count down, countdownTimer() to count down time, and PauseTimer() to stop the counting down. Just a quick look at the previous sentence will reveal inconsistencies in your code. Some functions start with upper case letters. Some start with lower case letters. Why?
But the real question is where does the problem you are trying to resolve come in? When loop() runs, and determines that you want to just run the timer, is the time being counted down wrong? Is the time being counted down changing between ReadEncoder() and countdownTimer()?
countdownTimer() has some subtle issues.
if( (millis() - millisTimer) > 1000)
If the function is called exactly 1000 milliseconds after the last call, it will not trigger the if statement. You most likely want >= there, not >.
millisTimer += 1000; //Adjust the timer forward 1 second
But, more than 1000 milliseconds has elapsed. millisTimer should be set to millis(), not incremented by 1000. And even that isn't perfect. What should happen is that there should be a local variable set at the top of the function:
unsigned long now = millis();
Then, no further calls to millis() should be made during this function. Instead, the value in now should be used every time the clock is to be checked. Notice that millis() returns an unsigned long, not a long.
Constraining seconds at the start of the function is wrong. An unknown amount of time may be lost doing that. Enough to account for the change you are seeing? Probably not, but it is wrong to be doing it.
I don't see any reason for any of the delay() calls in the code. Perhaps you could explain why they are there.