Ok Ive tried everything I can think of and can not get this to work correctly. This is for a paintball marker loader control when the marker is shot. I can always get the loader to turn on for a infinite period of time but it wont shut off when it should. Ive tried multiple approaches and nothing seems to work. The current code is below. Ive tried to add notes as best I could for others to follow. Help with this would be highly appreciated. Below is the best semi working method Ive gotten. it turns off and on with the trigger pulls but still runs without stopping on occasion. a simple delay will not work as it lags the firing process.
const int noid2 = SOLENOID2_PIN;
int noid2state = LOW;
unsigned long previousMillis = 0;
const long interval = 100;
Op_HDwell; // amount of time loader should run bassed off settings in the register 0-3000 millis
//TRIGGER_PIN, LOW = TRIGGER IS DEPRESSED
//TRIGGER_PIN, HIGH = TRIGGER IS RELEASED
void activateSolenoid2(){ // noid two pin is to activate the relay for the loader
unsigned long currentMillis = millis();
digitalWrite(SOLENOID2_PIN, HIGH);
if (currentMillis - previousMillis >= interval){
previousMillis = currentMillis;
digitalWrite(SOLENOID2_PIN, LOW);
}
}
void killnoid2(){
digitalRead(SOLENOID2_PIN);
if ((SOLENOID2_PIN == HIGH)> Op_HDwell){
digitalWrite(SOLENOID2_PIN, LOW);
}
}
const int noid2 = SOLENOID2_PIN;
int noid2state = LOW;
unsigned long previousMillis = 0;
const long interval = 100;
Op_HDwell; // amount of time loader should run bassed off settings in the register 0-3000 millis
//TRIGGER_PIN, LOW = TRIGGER IS DEPRESSED
//TRIGGER_PIN, HIGH = TRIGGER IS RELEASED
void activateSolenoid2(){ // noid two pin is to activate the relay for the loader
unsigned long currentMillis = millis();
digitalWrite(SOLENOID2_PIN, HIGH);
if (currentMillis - previousMillis >= interval){
previousMillis = currentMillis;
digitalWrite(SOLENOID2_PIN, LOW);
}
}
void killnoid2(){
digitalRead(SOLENOID2_PIN);
if ((SOLENOID2_PIN == HIGH)> Op_HDwell){
digitalWrite(SOLENOID2_PIN, LOW);
}
}
Post the entire, well formatted, code in code tags, please, that code is not complete.
This const int noid2 = SOLENOID2_PIN; makes a constant out of a already declare constant, why? On an Uno trying to minimize memory use is a good thing.
Sorry as you can see this is my first post here. If I were to post the whole code for just this .h file its 600 lines long. whos is gonna know where to look so I just posted the segment Im having issue with. The const int noid2 = SOLENOID2_PIN; was something I was trying based off an ensample I had found that did not pan out.
If the sketch is too large, then you can attach it.
The "Quick Reply" does not show the attach posibility, but the "REPLY" button does.
If something went wrong, then you can use More/Modify in the lower-right to Modify your post and fix things.
Please explain in your own words what this is:
if ((SOLENOID2_PIN == HIGH) > Op_HDwell)
In my own words: check the condition, that if a fixed number by a define is equal to HIGH. Compare the output of that condition with the variable or object OP_HDwell, then... I really don't know. The compiler probably doesn't know either.
When we say: "use millis() instead of delay()", then we mean that you need to rewrite the whole sketch from scratch. Using millis() is as if you are looking at the clock. The Arduino loop() should be executed as often as possible, and only do something if it is time to do something. Never wait in while-loops, never wait for millis(), keep the loop() running as often as possible.
If it helps, image that the loop() makes a buzzing sound is and like a spinning top. Your job is to keep it going.
if ((SOLENOID2_PIN == HIGH) > Op_HDwell) {
digitalWrite(SOLENOID2_PIN, LOW);
This was simply to state that if the pin was high longer then Op_HDwell the turn the pin low. I think I see what your saying but it was the only way I could actually get the pin to go low at all, but still does not function as intended.
Suppose you have #define SOLENOID2_PIN 5
Then the condition will be: if( (5 == HIGH) ...
That is always 'false'. So you have: if ( false > Op_HDwell)
That is not what you want.
phantom01231:
Ok Ive tried everything I can think of and can not get this to work correctly.
When is 'activateSolenoid2()' called? When is 'killnoid2()' called?
I think the answers should be "Once each time the trigger is pressed." and "Every time through loop().", in which case you want something like:
unsigned long previousMillis = 0;
const unsigned long interval = 100;
// Op_HDwell; // amount of time loader should run bassed off settings in the register 0-3000 millis
//TRIGGER_PIN, LOW = TRIGGER IS DEPRESSED
//TRIGGER_PIN, HIGH = TRIGGER IS RELEASED
void activateSolenoid2() // noid two pin is to activate the relay for the loader
{
digitalWrite(SOLENOID2_PIN, HIGH);
previousMillis = millis(); // Start the timer
}
void killnoid2()
{
if ((digitalRead(SOLENOID2_PIN) == HIGH) && millis() - previousMillis > Op_HDwell)
{
// It has been 'Op_HDwell' since the solenoid was last turned on
digitalWrite(SOLENOID2_PIN, LOW);
}
}
johnwasser:
When is 'activateSolenoid2()' called? When is 'killnoid2()' called?
I think the answers should be "Once each time the trigger is pressed." and "Every time through loop().", in which case you want something like:
unsigned long previousMillis = 0;
const unsigned long interval = 100;
// Op_HDwell; // amount of time loader should run bassed off settings in the register 0-3000 millis
//TRIGGER_PIN, LOW = TRIGGER IS DEPRESSED
//TRIGGER_PIN, HIGH = TRIGGER IS RELEASED
void activateSolenoid2() // noid two pin is to activate the relay for the loader
{
digitalWrite(SOLENOID2_PIN, HIGH);
previousMillis = millis(); // Start the timer
}
void killnoid2()
{
if ((digitalRead(SOLENOID2_PIN) == HIGH) && millis() - previousMillis > Op_HDwell)
{
// It has been 'Op_HDwell' since the solenoid was last turned on
digitalWrite(SOLENOID2_PIN, LOW);
}
}
They are called a little farther up in the code. which I think about it they may need to be moved but thus far up until this point everything has worked like a champ minus a few changes I have made and need to fix. The code needs a serious clean up for sure to delete some old stuff that is now just commented out.
Regardless if the sketch is working or not, you should not declare variables in a *.h file.
Please remove all the variables from the globals.h. Really, all of them. Seriously, every variable.
Use 'unsigned long' with millis() to avoid trouble.
I did not check why you use 'float' with millis(), it might be okay.
Don't do this: if (millis() > (Op_CurPullMS + 0 + (int)(1000.0 / (float)RAMP_MIN_BPS)))
If you compare the current millis() with something in the future, then you have a rollover problem after 50 days.
I can try to answer your question, but I am not sure where to look in the code.
If something needs to turn off after some time, then it has to be at the main level of the loop().
The state of the sketch could have changed, other parts can be active. All of that should be independent of the millis-timer to turn it off.
I make a timestamp of millis: previousMillis = millis()
Then I enable the millis-timer with: enable = true
Somewhere else, at the main level in the loop(), that millis-timer runs until it is time to stop.
if( enabled) // software timer is active ?
{
if( currentMillis - previousMillis >= interval)
{
digitalWrite( ledPin, LOW); // turn off led
enabled = false; // stop software timer
}
}
Koepel:
Regardless if the sketch is working or not, you should not declare variables in a *.h file.
Please remove all the variables from the globals.h. Really, all of them. Seriously, every variable.
Use 'unsigned long' with millis() to avoid trouble.
I did not check why you use 'float' with millis(), it might be okay.
Don't do this: if (millis() > (Op_CurPullMS + 0 + (int)(1000.0 / (float)RAMP_MIN_BPS)))
If you compare the current millis() with something in the future, then you have a rollover problem after 50 days.
I can try to answer your question, but I am not sure where to look in the code.
If something needs to turn off after some time, then it has to be at the main level of the loop().
The state of the sketch could have changed, other parts can be active. All of that should be independent of the millis-timer to turn it off.
I make a timestamp of millis: previousMillis = millis()
Then I enable the millis-timer with: enable = true
Somewhere else, at the main level in the loop(), that millis-timer runs until it is time to stop.
johnwasser:
When is 'activateSolenoid2()' called? When is 'killnoid2()' called?
I think the answers should be "Once each time the trigger is pressed." and "Every time through loop().", in which case you want something like:
unsigned long previousMillis = 0;
const unsigned long interval = 100;
// Op_HDwell; // amount of time loader should run bassed off settings in the register 0-3000 millis
//TRIGGER_PIN, LOW = TRIGGER IS DEPRESSED
//TRIGGER_PIN, HIGH = TRIGGER IS RELEASED
void activateSolenoid2() // noid two pin is to activate the relay for the loader
{
digitalWrite(SOLENOID2_PIN, HIGH);
previousMillis = millis(); // Start the timer
}
void killnoid2()
{
if ((digitalRead(SOLENOID2_PIN) == HIGH) && millis() - previousMillis > Op_HDwell)
{
// It has been 'Op_HDwell' since the solenoid was last turned on
digitalWrite(SOLENOID2_PIN, LOW);
}
}
phantom01231:
that segment is the only part that deals with the loader so I doubt there would be a mistake elsewhere causing it
OK. If the rest of the code is perfect, can you explain why the sketch is not doing what you want?
Note: I looked at the rest of the code. The compiler found a number of lines that are technically legal but don't make sense. You seem to be putting the unsigned long value of millis() into a 'float' and then not using it anywhere. Perhaps for some of those you intended to use a global but declared a new local instead? You have a couple of places where it looks like you used '=' instead of '==' in an 'if' statement. You have statements that don't do anything.
While I was looking I noticed that "killnoid2()" was only called once, immediately after "activateSolenoid2()". If you only check the timer once, immediately after the timer is started, it is unlikely you will ever detect the timer ending. That is why you should call that function frequently... like every time through loop().
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino: In function 'void FM_HandleFireMode()':
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:170:23: warning: statement has no effect [-Wunused-value]
Op_RampStarted;
^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:192:35: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
if (Op_TriggerWasPulled = false) {
~~~~~~~~~~~~~~~~~~~~^~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:172:15: warning: unused variable 'nextPullMinMs' [-Wunused-variable]
float nextPullMinMs = millis();
^~~~~~~~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:238:23: warning: statement has no effect [-Wunused-value]
Op_RampStarted;
^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:260:35: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
if (Op_TriggerWasPulled = false) {
~~~~~~~~~~~~~~~~~~~~^~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:240:15: warning: unused variable 'nextPullMinMs' [-Wunused-variable]
float nextPullMinMs = millis();
^~~~~~~~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:269:7: warning: statement has no effect [-Wunused-value]
!Op_RampStarted;
^~~~~~~~~~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino: In function 'bool FM_ProcessShot()':
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:383:7: warning: variable 'fireRateOver5BPS' set but not used [-Wunused-but-set-variable]
int fireRateOver5BPS = false;
^~~~~~~~~~~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:415:9: warning: variable 'Op_HDwell' set but not used [-Wunused-but-set-variable]
float Op_HDwell = millis();
^~~~~~~~~
Sketch uses 13960 bytes (43%) of program storage space. Maximum is 32256 bytes.
Global variables use 1488 bytes (72%) of dynamic memory, leaving 560 bytes for local variables. Maximum is 2048 bytes.
johnwasser:
OK. If the rest of the code is perfect, can you explain why the sketch is not doing what you want?
Note: I looked at the rest of the code. The compiler found a number of lines that are technically legal but don't make sense. You seem to be putting the unsigned long value of millis() into a 'float' and then not using it anywhere. Perhaps for some of those you intended to use a global but declared a new local instead? You have a couple of places where it looks like you used '=' instead of '==' in an 'if' statement. You have statements that don't do anything.
While I was looking I noticed that "killnoid2()" was only called once, immediately after "activateSolenoid2()". If you only check the timer once, immediately after the timer is started, it is unlikely you will ever detect the timer ending. That is why you should call that function frequently... like every time through loop().
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino: In function 'void FM_HandleFireMode()':
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:170:23: warning: statement has no effect [-Wunused-value]
Op_RampStarted;
^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:192:35: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
if (Op_TriggerWasPulled = false) { ^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:172:15: warning: unused variable 'nextPullMinMs' [-Wunused-variable]
float nextPullMinMs = millis();
^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:238:23: warning: statement has no effect [-Wunused-value]
Op_RampStarted;
^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:260:35: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
if (Op_TriggerWasPulled = false) { ^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:240:15: warning: unused variable 'nextPullMinMs' [-Wunused-variable]
float nextPullMinMs = millis();
^
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:269:7: warning: statement has no effect [-Wunused-value]
!Op_RampStarted;
^~~~~~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino: In function 'bool FM_ProcessShot()':
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:383:7: warning: variable 'fireRateOver5BPS' set but not used [-Wunused-but-set-variable]
int fireRateOver5BPS = false;
^~~~~~~~~~~~~~~~
/Users/john/Downloads/Angel_PPE_Firmware_v2.2/Firing.ino:415:9: warning: variable 'Op_HDwell' set but not used [-Wunused-but-set-variable]
float Op_HDwell = millis();
^~~~~~~~~
Sketch uses 13960 bytes (43%) of program storage space. Maximum is 32256 bytes.
Global variables use 1488 bytes (72%) of dynamic memory, leaving 560 bytes for local variables. Maximum is 2048 bytes.
That actually makes sense calling on it more to check it. I'm in the process now of going thru and changing the millis() that are floats to unsigned longs as Im not sure what the purpose of that was. The gentleman that started the code did somethings that did not make much sense. About 90% of this code has been altered at this point and there is still much to go. but the code does work amazingly enough. I did not catch that some of the lines had no effect on anything. Thank you for pointing that out. I will look into that as well. I will report back with some results.
phantom01231:
I did not catch that some of the lines had no effect on anything. Thank you for pointing that out.
Go to "Preferences..." and set "Compiler warnings:" to "All". The warnings show up in the text box below the sketch when you verify or upload.
If the code works you can remove any lines with these warnings:
warning: statement has no effect
warning: unused variable 'X'
warning: variable 'X' set but not used
"warning: suggest parentheses around assignment used as truth value"
Where you see this warning you have a '=' in an 'if' where '==' is much more common. If it was meant to be a comparison, change it to '=='. If it was meant to be an assignment AND test for non-zero, put the assignment (A = B;) in its own line ABOVE the 'if' and put a separate "A != 0" test in the 'if'. If you did not write that line it may be hard to determine the author's intent.
Okay got this one solved. Did as suggested and placed killnoid2(); in a few places inside the loop() and the loader finally started turning off! How ever it would not turn off as needed per the setting of OP_HDwell. Getting it to do so was just a little adjustment in the math for the timer. Thanks all for the help!