Pages: 1 [2] 3   Go Down
Author Topic: Audomatic Garage Door Close  (Read 3064 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

while(PIR1, PIR2 == HIGH);
All I can say is that this works.

...

I'm saying is that it does or at least seems to as far as I can tell. If it is in fact wrong then it is wrong in form and not functionality.

No, it doesn't work, for two reasons, both of which have been explained. But presumably your testing has not been sufficient to expose the bug yet. I suggest it would benefit you to be far less eager to dismiss people's criticisms of your code.

Ok, I get that. Which is why I've explained ad nauseum that my basis for defending the way it's written is because as far as I can tell it works. That's all I'm saying, I'm not trying to dismiss anyone, I'm not trying to start a fight, or pee on anyone's fun-fire, I'm just saying "hum, odd because in all the testing I've done this seems to work swimmingly."

See: https://vimeo.com/50017640
Logged

UK
Offline Offline
Shannon Member
****
Karma: 222
Posts: 12534
-
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I'm just saying "hum, odd because in all the testing I've done this seems to work swimmingly."

I'm just saying that code doesn't achieve what you expect. I'd suggest that you could remove that line code and get the same behaviour. So, either what you're trying to do there is not required, or you haven't tested it sufficiently to realise that it's not doing what you intend.
Logged

I only provide help via the forum - please do not contact me for private consultancy.

Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I'm just saying "hum, odd because in all the testing I've done this seems to work swimmingly."

I'm just saying that code doesn't achieve what you expect. I'd suggest that you could remove that line code and get the same behaviour. So, either what you're trying to do there is not required, or you haven't tested it sufficiently to realise that it's not doing what you intend.

Noted.
Logged

California
Offline Offline
Faraday Member
**
Karma: 88
Posts: 3354
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Ok, I get that. Which is why I've explained ad nauseum that my basis for defending the way it's written is because as far as I can tell it works. That's all I'm saying, I'm not trying to dismiss anyone, I'm not trying to start a fight, or pee on anyone's fun-fire, I'm just saying "hum, odd because in all the testing I've done this seems to work swimmingly."

Which means 1 or 2 things:

1. You haven't sufficiently tested enough to expose the error.
2. The code is redundant/pointless and is handled in a different portion of the code.

If it works, that's fine for you. The problem is you're on the forum, asking for people for help with your code. People here don't want to weed through pointless code, or code that will cause issues as you make changes to your program. That's why it's pointed out.

The problem with GoTo is it's hard to follow/debug. So again, if it works for you, great. If not, you have to take into account how others feel about the code that you've written if you're asking for your help, because GoTo complicates things for those trying to read the code.

As far as your original question, I haven't read your code, but the fact that GoTo is used (delay(), and while statement could also cause issues), may make the normal way of doing this not work, but that's something you can experiment yourself:

Code:
if something that causes you to set a condition that will blink the led occurs
   set the condition
   note the time
end if

if the blinking condition is set and it's been 5 minutes
  turn the LED on
  note the time
end if

if it's been on long enough
  turn the led off
end if

Take a look at the Blink Without Delay example for how to handle the "note the time" and "it's been..." portions of the psuedocode.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Ok, I get that. Which is why I've explained ad nauseum that my basis for defending the way it's written is because as far as I can tell it works. That's all I'm saying, I'm not trying to dismiss anyone, I'm not trying to start a fight, or pee on anyone's fun-fire, I'm just saying "hum, odd because in all the testing I've done this seems to work swimmingly."

Which means 1 or 2 things:

1. You haven't sufficiently tested enough to expose the error.
2. The code is redundant/pointless and is handled in a different portion of the code.

If it works, that's fine for you. The problem is you're on the forum, asking for people for help with your code. People here don't want to weed through pointless code, or code that will cause issues as you make changes to your program. That's why it's pointed out.

The problem with GoTo is it's hard to follow/debug. So again, if it works for you, great. If not, you have to take into account how others feel about the code that you've written if you're asking for your help, because GoTo complicates things for those trying to read the code.

As far as your original question, I haven't read your code, but the fact that GoTo is used (delay(), and while statement could also cause issues), may make the normal way of doing this not work, but that's something you can experiment yourself:

Code:
if something that causes you to set a condition that will blink the led occurs
   set the condition
   note the time
end if

if the blinking condition is set and it's been 5 minutes
  turn the LED on
  note the time
end if

if it's been on long enough
  turn the led off
end if

Take a look at the Blink Without Delay example for how to handle the "note the time" and "it's been..." portions of the psuedocode.

If it helps, I've looked at the blink without delay example and I can successfully get the override LED to blink during the override. I cannot however get a beep out of the speaker.
Logged

New Jersey
Offline Offline
Faraday Member
**
Karma: 65
Posts: 3638
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Can you get anything out of the speaker elsewhere in your code? Such as the doorclose1 section?
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Can you get anything out of the speaker elsewhere in your code? Such as the doorclose1 section?

Yes, elsewhere it seems to work fine. I can get it to beep during the override too, a steady beep (I think I can even get it to do an alternating beep). The issue I'm having is getting it to only beep every 5 minutes (for example) while the override is active.

I don't suspect, but I could be wrong, that the goto is causing any issues with this simply because during the override the goto isn't read... if it helps to eliminate the goto then I will... I'm just at a loss as to how. Would a sub-routine outside of the for(loop) work? Something like:

Code:
if (digitalRead(OverrideSW) && digitalRead(DoorOpenedSW) || CloseAttemptsVal == CloseAttempts) {
    OverrideState = true;
  }

  if (OverrideState == true) {
    override()
  }


With the sub-routine being:

Code:

void override(){

digitalWrite(OverrideLED, HIGH);
    DoorOpenTime = 0;
    DoorState = false;
    DoorClose1 = false;
    DoorClose2 = false;
    DoorReset = false;
  }
}


Then whenever I need the override, instead of using goto I could just call the sub-routine override()?
Logged

New Jersey
Offline Offline
Faraday Member
**
Karma: 65
Posts: 3638
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I'd be inclined to refactor the thing into a state machine (search the forums). But for less work, when you set OverrideState to be true, store the current value of millis in a global unsigned long. In the section you run when OverrideState is true do something like this:

Code:
if(millis()-TimeOverrideWasInvoked > 30000UL)
  {
  tone(Speaker,100);
  TimeOverrideWasInvoked =millis();
  }
Edit: Need to reset TimeOverrideWasInvoked
« Last Edit: September 27, 2012, 02:44:13 pm by wildbill » Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I'd be inclined to refactor the thing into a state machine (search the forums). But for less work, when you set OverrideState to be true, store the current value of millis in a global unsigned long. In the section you run when OverrideState is true do something like this:

Code:
if(millis()-TimeOverrideWasInvoked > 30000UL)
  {
  Tone(Speaker,100);
  }

Ok, I'll try this out when I'm back in front of my Arduino
Logged

Central MN, USA
Offline Offline
Tesla Member
***
Karma: 72
Posts: 7171
Phi_prompt, phi_interfaces, phi-2 shields, phi-panels
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

2) I understand that that the use of the goto is frowned upon (though I did not know hoe vehement people were about it  smiley). I used it 'cause it worked and as mentioned here (http://arduino.cc/en/Reference/Goto) I did not use in "unrestrained." I'm open to suggestions on how to eliminate this.

That was popular before structured programming paradigm came along to save the day when code became hundreds of lines long that resembles a certain type of poorly cooked Italian noodle. You have to have well-defined entrance and exit points in your code for debugging. It's not an optional writing style. I'll use language analogy once again. Goto was like middle English smiley

http://en.wikipedia.org/wiki/Structured_programming

Object-oriented programming didn't come out of a fad either. It was absolutely necessary to manage even larger projects and make code stay independent and reusable like modules. Then more paradigm that I don't know for even larger and more flexible projects.
Logged


Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 289
Posts: 25697
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
There was some confusion because people were pointing out errors
The confusion was purely yours.

Quote
things that should not work
sp. "could not work"
Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 474
Posts: 18696
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

... it felt like your first post was dedicated to detailing how stupid I am.

I want you to understand that I look at about 20 to 30 posts a day with programming questions. Once I read "it doesn't work" and browse through the code and see some obvious problems, for example where you are comparing pin numbers instead of reading from those pins and comparing the results, I usually quote the problem code, with a hint about how to fix it, and then stop reading. I'm not being paid for this, you know.

I never called you "stupid".

If I had really wanted to make you look small I would have kept going and found all the other errors (if any).

I'm a believer in teaching people to fish rather than giving them a fish, so I don't usually try to simply hand back corrected code.

You might want to reconsider telling someone on a forum they don't understand "the basics" especially when you say:

Quote
I'm not a programmer ...

If you are not a programmer, how do you know I don't understand the basics?
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

2) I understand that that the use of the goto is frowned upon (though I did not know hoe vehement people were about it  smiley). I used it 'cause it worked and as mentioned here (http://arduino.cc/en/Reference/Goto) I did not use in "unrestrained." I'm open to suggestions on how to eliminate this.

That was popular before structured programming paradigm came along to save the day when code became hundreds of lines long that resembles a certain type of poorly cooked Italian noodle. You have to have well-defined entrance and exit points in your code for debugging. It's not an optional writing style. I'll use language analogy once again. Goto was like middle English smiley

http://en.wikipedia.org/wiki/Structured_programming

Object-oriented programming didn't come out of a fad either. It was absolutely necessary to manage even larger projects and make code stay independent and reusable like modules. Then more paradigm that I don't know for even larger and more flexible projects.

I get why you would not want to use goto, I just could not come up with another way... and it worked so.

Although, that's not entirely true I did consider another option. Currently where goto is used the code is:

Code:
if (DoorClose1 == true) {
    while (DoorCloseDelayVal != DoorCloseDelay){
      tone(Speaker,100);
      digitalWrite(DoorClosingLED, HIGH);
      delay(500);
      tone(Speaker,200);
      digitalWrite(DoorClosingLED, LOW);
      delay(500);
      DoorCloseDelayVal ++;
      
      if (digitalRead(OverrideSW) && digitalRead(DoorOpenedSW)) {
        [b]goto override;[/b]
      }
      
      if (CloseAttemptsVal == CloseAttempts) {
        [b]goto override;[/b]
      }
      
  }

I considered doing something like this:

Code:
if (DoorClose1 == true) {
    while (DoorCloseDelayVal != DoorCloseDelay){
      tone(Speaker,100);
      digitalWrite(DoorClosingLED, HIGH);
      delay(500);
      tone(Speaker,200);
      digitalWrite(DoorClosingLED, LOW);
      delay(500);
      DoorCloseDelayVal ++;
      
      if (digitalRead(OverrideSW) && digitalRead(DoorOpenedSW)) {
       digitalWrite(OverrideLED, HIGH);
DoorOpenTime = 0;
DoorState = false;
DoorClose1 = false;
DoorClose2 = false;
DoorReset = false;
      }
      
      if (CloseAttemptsVal == CloseAttempts) {
        digitalWrite(OverrideLED, HIGH);
DoorOpenTime = 0;
DoorState = false;
DoorClose1 = false;
DoorClose2 = false;
DoorReset = false;
      }
      
  }

But I thought since I already have it written why not just use the a goto to go to the override code... Obviously I was wrong.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 40
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

... it felt like your first post was dedicated to detailing how stupid I am.

I want you to understand that I look at about 20 to 30 posts a day with programming questions. Once I read "it doesn't work" and browse through the code and see some obvious problems, for example where you are comparing pin numbers instead of reading from those pins and comparing the results, I usually quote the problem code, with a hint about how to fix it, and then stop reading. I'm not being paid for this, you know.

I never called you "stupid".

If I had really wanted to make you look small I would have kept going and found all the other errors (if any).

I'm a believer in teaching people to fish rather than giving them a fish, so I don't usually try to simply hand back corrected code.

You might want to reconsider telling someone on a forum they don't understand "the basics" especially when you say:

Quote
I'm not a programmer ...

If you are not a programmer, how do you know I don't understand the basics?

Well, first and foremost I do appreciate your time. You've pushed me to try to come up with something other than the goto. And you pointed out the issue with how I was reading the state of the PINs.

That said, in case you didn't see my other posts - I never said "it doesn't work" In fact what I said in my initial post was that it does work but that I'm not sure how to go about adding this specific function (that being a beep every x # of min. during the override).

Again, I honestly thought that you were telling me that I should not be naming/defining PINs. I'm sorry about my confusion on this but that's where my "basic stuff" quip came into play because I was siting here thinking "every single example I've looked at does this..." Hopefully that clears up why I said that and for what it's worth I take it back. You'll also have to excuse my confusion when you (and a number of other people) insisted that what I have will not work because coming into this it all did (including PIR1, PIR2 == HIGH). Now it's been pointed out that I may just not be testing enough to catch the error but I assure you that as far as I know it works. I will however make the corrections you and others have suggested.

Lastly, I'm not expecting to be spoon fed. Guidance is nice though.

Hope that we can move past the initial confusion and that once I figure out how to get my code working without the goto you'll be able to stomach my code smiley
Logged

Central MN, USA
Offline Offline
Tesla Member
***
Karma: 72
Posts: 7171
Phi_prompt, phi_interfaces, phi-2 shields, phi-panels
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

About the goto alternative, you need to pick a good working logic and then decide, such as:

in the while loop, if certain condition is met, call an override function, which returns to the loop and the code continue to loop

or if with a different logic, you may decide to end the loop and then call the override function.

In any case, since the override code is pretty independent from other code, it can stay together as a function you can call.

It's best if you start with a flow diagram to specify how the program will run, and then write the code according to the flow diagram.
Logged


Pages: 1 [2] 3   Go Up
Jump to: