I'm trying to make a drawer that slides open and closed, powered by a continuous motion servo, activated by a button. When the button is pushed, the button should be deactivated and the servo should turn on, pushing the drawer open if a sensor detects the first magnet (i.e., the drawer started in the "closed" position) and then stopping and reactivating the button when another sensor detects the second magnet. If the drawer starts in the "open" position, it should do the opposite.
Problem is, I've never been able to get the code to do that. The current version causes the drawer to open slightly, only as long as I'm holding the button down. On the plus side, if I hold the button down long enough for the drawer to hit the "open" position, it can detect magnet #2 and reverse directions for a stutter or two before switching directions and opening again.
Any idea what's wrong with my code? Forgive me if this is a little messy, I've built and rebuilt it a few times.
#include <Servo.h>
Servo drawerMotor;
bool drawerActive = HIGH;
byte inWire = 7;
bool buttonPushed = LOW;
bool buttonPushed2 = LOW;
byte reactivate = 0;
bool drawerOpen = LOW;
byte magWireOne = 11;
byte magWireTwo = 12;
bool magnetOne = LOW;
bool magnetTwo = LOW;
void setup() {
drawerMotor.attach(9);
drawerMotor.write(90);
pinMode(magWireOne,INPUT);
pinMode(magWireTwo,INPUT);
pinMode(inWire,INPUT);
Serial.begin(9600);
}
void loop() {
// put your main code here, to run repeatedly:
magnetOne = digitalRead(magWireOne);
magnetTwo = digitalRead(magWireTwo);
buttonPushed2 = digitalRead(inWire);
if (buttonPushed2 == HIGH && drawerActive==HIGH) // I put this in to try and deactivate the button the moment it's pushed
{
buttonPushed = HIGH;
drawerActive=LOW;
Serial.println("Button Has Been Pushed");
}
if(buttonPushed==HIGH && magnetOne==HIGH) // Begins to open the drawer, if the button has been pushed and the drawer is in the closed position
{
buttonPushed = LOW;
Serial.println("Button pushed 1");
drawerMotor.write(45);
}
if(buttonPushed==HIGH && magnetTwo==HIGH) // Begins to close the drawer, if the button has been pushed and the drawer is in the open position
{
buttonPushed = LOW;
Serial.println("Button pushed 2");
drawerMotor.write(135);
}
if(drawerActive==LOW && magnetTwo==HIGH && drawerOpen==LOW) // Stops the drawer as it moves into the open position, and reactivates the button
{
Serial.println("Drawer Opened");
drawerMotor.write(90);
drawerActive = HIGH;
drawerOpen = HIGH;
}
if(drawerActive==LOW && magnetOne==HIGH && drawerOpen==HIGH) Stops the drawer as it moves into the closed position, and reactivates the button
{
Serial.println("Drawer Closed");
drawerMotor.write(90);
drawerActive = HIGH;
drawerOpen = LOW;
}
}
Hope you don't mind, cleaned your code up a bit. It just makes it soooo much easier when selected, copied and pasted into an editor to see what it really looks like.
#include <Servo.h>
Servo drawerMotor;
bool drawerActive = HIGH;
bool buttonPushed = LOW;
bool buttonPushed2 = LOW;
bool drawerOpen = LOW;
bool magnetOne = LOW;
bool magnetTwo = LOW;
byte inWire = 7;
byte reactivate = 0;
byte magWireOne = 11;
byte magWireTwo = 12;
void setup()
{
drawerMotor.attach(9);
drawerMotor.write(90);
pinMode(magWireOne,INPUT);
pinMode(magWireTwo,INPUT);
pinMode(inWire,INPUT);
Serial.begin(9600);
}
void loop()
{
// put your main code here, to run repeatedly:
magnetOne = digitalRead(magWireOne);
magnetTwo = digitalRead(magWireTwo);
buttonPushed2 = digitalRead(inWire);
if (buttonPushed2 == HIGH && drawerActive==HIGH) // I put this in to try and deactivate the button the moment it's pushed
{
buttonPushed = HIGH;
drawerActive=LOW;
Serial.println("Button Has Been Pushed");
}
if(buttonPushed==HIGH && magnetOne==HIGH) // Begins to open the drawer, if the button has been pushed and the drawer is in the closed position
{
buttonPushed = LOW;
Serial.println("Button pushed 1");
drawerMotor.write(45);
}
if(buttonPushed==HIGH && magnetTwo==HIGH) // Begins to close the drawer, if the button has been pushed and the drawer is in the open position
{
buttonPushed = LOW;
Serial.println("Button pushed 2");
drawerMotor.write(135);
}
if(drawerActive==LOW && magnetTwo==HIGH && drawerOpen==LOW) // Stops the drawer as it moves into the open position, and reactivates the button
{
Serial.println("Drawer Opened");
drawerMotor.write(90);
drawerActive = HIGH;
drawerOpen = HIGH;
}
if(drawerActive==LOW && magnetOne==HIGH && drawerOpen==HIGH) Stops the drawer as it moves into the closed position, and reactivates the button
{
Serial.println("Drawer Closed");
drawerMotor.write(90);
drawerActive = HIGH;
drawerOpen = LOW;
}
}
And the first thing that leaps out is that the last if() statement is not commented correctly (no //). You might want to try that first and see if it makes any difference.
Thanks for the reply (and thanks for cleaning up my Frankenstein code, too). The comments don't actually exist in the version I have in my editor, they were a hasty addition I threw in before I posted them here so people wouldn't have to figure my intent out for themselves.
MDCooke:
Problem is, I've never been able to get the code to do that. The current version causes the drawer to open slightly, only as long as I'm holding the button down.
Your program looks well organized but you are missing one concept - the state of the drawer "opening" or "closing"
You could have a variable (let's call it drawerMotion) and it can have 4 values O' 'o' 'C' 'c' for the 4 states Open, Closed and opening and closing.
When the inner magnet is detected the motor stops and drawerMotion changes to 'C' and in that state the button will cause the state to change to 'o' and the motor will be started. when the outer magnet is detected the motor stops and drawerMotion changes to 'O'
I put this together offline a few hours ago so it all appears as commented code. Sorry.
/*===============================================
Ok, lets do this step by step. I like state machines so I'm going to regard it as such.
I see four states: CLOSED, OPENING, OPEN and CLOSING. Throw a fifth one in and call it STANDBY where you can do housekeeping or put everything to sleep if you want.
Let's say that STANDBY is entered after either OPEN or CLOSED has been achieved. A button push will awaken MCU from STANDBY and progress to OPENING if current state is CLOSED or CLOSING if current state is OPEN. Current states are defined by mag1 (CLOSED) and mag2 (OPEN). OPENING is changed to OPEN upon signal from mag2, CLOSING changes to CLOSED upon signal from mag1. Seem about right?
I'm also going to make an assumption here that maybe if the drawer is in motion you don't want to have to wait until that motion is complete before doing other things. So if the drawer is in motion the button push will stop it. The next button push will start it again in the opposite direction. That provides the most flexibility as button push 1 == stop, button push 2 == reverse (backwards), button push 3 == stop, button push 4 reverse (forwards) and son on. So add a sixth state and call it STOPPED.
*/
// states
#define CLOSED 0
#define OPENING 1
#define OPEN 2
#define CLOSING 3
#define STANDBY 4
#define STOPPED 5
// State machines tend to be event-driven processes so lets identify our events. Button push, mag1, mag2 and one we'll just call timeout that can be used to place the device in STANDBY. That was easy.
// events
#define BUTTPUSH 0
#define MAGNET1 1
#define MAGNET2 2
#define TIMEOUT 3
// Lastly, rather than have the loop identify the pin values, lets turn them into little functions.
bool mag1() = {return digitalRead(magWireOne);}
bool mag2() = {return digitalRead(magWireTwo);}
bool butt() = {return digitalRead(inWire);}
// now we can get the value anytime we want (or not) and once assigned will remain so until we
// reassign. Add two more that expand on these:
bool isClosed() = {return (mag1())?TRUE:FALSE;}
bool isopen() = {return (mag2())?TRUE:FALSE;}
byte state;
byte previous_state;
void process_event(byte event_id)
{
switch (state)
{
case CLOSED // only action to OPENING with BUTTPUSH
if (event_id == BUTTPUSH)
{
openDrawer();
state = OPENING;
}
if (event_id == TIMEOUT) state = STANDBY;
previous_state = CLOSED
break;
case OPENING // only actions to stop with button or mag2 (OPEN)
if (event_id == BUTTPUSH)
{
stopDrawer();
state = STOPPED;
}
if (event_id == MAGNET2)
{
stopDrawer();
state = OPEN;
}
previous_state = OPENING;
break;
case OPEN // only action to closing with BUTTPUSH
if (event_id == BUTTPUSH)
{
closeDrawer();
state = CLOSING;
}
if (event_id == TIMEOUT) state = STANDBY;
previous_state = OPEN;
break;
case CLOSING // only actions to stop with button or mag1 (CLOSED)
if (event_id == BUTTPUSH)
{
stopDrawer();
state = STOPPED;
}
if (event_id == MAGNET1)
{
stopDrawer();
state = CLOSED;
}
previous_state = CLOSING;
break;
case STANDBY // only enter after timeout from open or closed
if (event_id == BUTTPUSH)
{
if (isclosed)
{
openDrawer;
state = OPENING;
}
else
{
closeDrawer;
state = CLOSING;
}
previous_state = STANDBY;
}
break;
case STOPPED: // only actions to move opposite to previous
if (event_id == BUTTPUSH)
{
if (previous_state == CLOSING)
{
openDrawer();
state = OPENING;
}
if (previous_state == OPENING)
{
closeDrawer();
state = CLOSING;
}
previous_state = STOPPED;
}
break;
}
}
void timeout(){}
void openDrawer(){}
void closeDrawer(){}
void stopDrawer(){}
void setup()
{
state = STANDBY;
}
void loop()
{
if (butt()) process_event(BUTTPUSH);
if (mag1()) process_event(MAGNET1);
if (mag2()) process_event(MAGNET2);
if (timeout() process_event(TIMEOUT);
}
/*
Right, so I just threw this together on the fly and it certainly hasn't been tested but I think you'll get the drift, and by the way, welcome to the world of state machines.
Now all you have to do is fill in the four litlle functions before setup with whatever moves you, or the drawer. Initialize the pins and such and give it a go. I'll have a look at the logic again in a bit but at first glance it seems okay. Maybe you'll check it?
=========================================*/
UNKNOWN is a valid concern. I think the OP should do a bit of the work though.
You could check both magnets and if neither indicated a fully opened or closed position conclude that it was somewhere in the middle. The first action might therefore be to close it fully. Or open it fully....
I think it is something to do with the need to create an ENUM as an entity whereas the #defines are just "text". It may have something to do with the order in which I think about things. I will probably be halfway through a problem before the advantage of using an ENUM occurs to me, and then it seems (to me) a bigger deal to create an ENUM rather than some #defines
Don't get me wrong. I think folk who use ENUMs are wonderful.
I think that we're approaching thin philosophical ice here and should probably avoid it. Personally, I prefer using const but switch() doesn't care what I like.
DKWatson:
I think that we're approaching thin philosophical ice here and should probably avoid it. Personally, I prefer using const but switch() doesn't care what I like.
Nothing philosophical about it, it is just good programming practice. Using an enum says the states are related. Plus you avoid name collisions with unknown defines (which are vast in Arduino land).
Plus, switch is a heckofalot smarter with enums...
enum State {
CLOSED,
OPENING,
OPEN,
CLOSING,
UNKNOWN,
}state = UNKNOWN;
void setup() {
switch (state) {
case CLOSED:
break;
case OPENING:
break;
case OPEN:
break;
case UNKNOWN:
break;
}
}
void loop() {
}
warning: enumeration value 'CLOSING' not handled in switch [-Wswitch]
switch (state)
BulldogLowell:
Using an enum says the states are related. Plus you avoid name collisions with unknown defines (which are vast in Arduino land).
Very good points.
I think you may have converted me to ENUMs.
One of the things that I have found irritating and confusing is the usual way ENUMs (and STRUCTs) are defined. In your example you have
enum State {
[.....]
}state = UNKNOWN;
where the name for the definition of the ENUM (for want of a better description) is called "State" and the name "state" is used for the instance of it. Also I find the inclusion of the instance name as part of the definition confusing.
I came across an ENUM tutorial in which the guy was suffixing the definition with _t so it was identified as a type - which would make your version
enum State_t {
and then an instance could be created with
State_t state = UNKNOWN;
However that still did not seem as clear as I would like and I hit on the idea of
enum drawerStateENUM {
and
drawerStateENUM state = UNKNOWN;
which nicely satisfies my need for clarity. And I can do the same thing with my STRUCTs.
Thank you. I think I have downed 2 birds with your one stone.
So am I right to think that someState and anotherState are then States in the same way that pinNumber is a byte?
States is a user defined data type and you can then use that data type when declaring variables.
Does that limit a States such as someState to only allowing CLOSED, OPENING, OPEN, CLOSING and UNKNOWN as values?
No. You can do silly things like giving a States variable a value that is not declared in the enum States anotherState = 123;but one of the reasons for using an enum in the first place is to use meaningful names but without needing to know their value, although you can assign a value to them if you want TO
LesserMole:
So am I right to think that someState and anotherState are then States in the same way that pinNumber is a byte?
I had some of the same uncertainty which is what led me to write Reply #12 (in the context of the code in Reply #10). It clarifies things for me, but it may just confuse others.
Don't let me interrupt the discussion, I just wanted to thank everyone for their help. Given my (very low) skill level I need to study DKWatson's code a bit before I'm comfortable playing around with it, but hey, learning is what I'm here for!
