search

d8adrvn / smart_sprinkler

Smart Sprinkler Controller Using Arduino and SmartThings
http://build.smartthings.com/projects/smartsprinkler/
Apache License 2.0
62 stars 55 forks source link

Device shows all zones queued even if they have 0 time #17

Closed mattnichols closed 10 years ago

mattnichols commented 10 years ago

Need to fix Arduino code to ignore queue requests when the time is 0.

d8adrvn commented 10 years ago

Now that you mention this, it does seem odd to queue with no time. I have always seen this and assumed its fine. I was worried if it did not queue, then someone who forgot to put >0 in for time, would not understand why their tile was not queuing. At least now, they know its queuing and running for just a brief "flash".

mattnichols commented 10 years ago

I have liked seeing all the zones queue up for the same reason you mentioned, but I think that is because I know how it works under the hood. A regular user may be confused if they only have 2 zones and they see all 8 queued up to turn on. Or, they may be concerned that the app is not working if they set up a schedule to only have one zone run and they see all of them queued up when the timer fires. I guess the question is... which is the least surprising to the regular user? What do you think? Should we change this?

It does require a change to the Arduino code.

On Tue, Jun 17, 2014 at 9:57 PM, Stan notifications@github.com wrote:

Now that you mention this, it does seem odd to queue with no time. I have always seen this and assumed its fine. I was worried if it did not queue, then someone who forgot to put >0 in for time, would not understand why their tile was not queuing. At least now, they know its queuing and running for just a brief "flash".

— Reply to this email directly or view it on GitHub https://github.com/d8adrvn/smart_sprinkler/issues/17#issuecomment-46393658 .

d8adrvn commented 10 years ago

Hi Matt,

I see your point. Also, as you consider that folks may have multiple smart apps instances running, foe example one for lawn and one for drip, it will be confusing whether the apps are triggering the correct zones.

-Stan

On Jun 18, 2014, at 10:37 AM, Matt Nichols notifications@github.com wrote:

I have liked seeing all the zones queue up for the same reason you mentioned, but I think that is because I know how it works under the hood. A regular user may be confused if they only have 2 zones and they see all 8 queued up to turn on. Or, they may be concerned that the app is not working if they set up a schedule to only have one zone run and they see all of them queued up when the timer fires. I guess the question is... which is the least surprising to the regular user? What do you think? Should we change this?

It does require a change to the Arduino code.

On Tue, Jun 17, 2014 at 9:57 PM, Stan notifications@github.com wrote:

Now that you mention this, it does seem odd to queue with no time. I have always seen this and assumed its fine. I was worried if it did not queue, then someone who forgot to put >0 in for time, would not understand why their tile was not queuing. At least now, they know its queuing and running for just a brief "flash".

— Reply to this email directly or view it on GitHub https://github.com/d8adrvn/smart_sprinkler/issues/17#issuecomment-46393658 .

— Reply to this email directly or view it on GitHub.

mattnichols commented 10 years ago

Agreed. I think we are in a good state now. We can include this enhancement in the next release. What do you think?

On Jun 18, 2014, at 6:10 PM, Stan notifications@github.com wrote:

Hi Matt,

I see your point. Also, as you consider that folks may have multiple smart apps instances running, foe example one for lawn and one for drip, it will be confusing whether the apps are triggering the correct zones.

-Stan

On Jun 18, 2014, at 10:37 AM, Matt Nichols notifications@github.com wrote:

I have liked seeing all the zones queue up for the same reason you mentioned, but I think that is because I know how it works under the hood. A regular user may be confused if they only have 2 zones and they see all 8 queued up to turn on. Or, they may be concerned that the app is not working if they set up a schedule to only have one zone run and they see all of them queued up when the timer fires. I guess the question is... which is the least surprising to the regular user? What do you think? Should we change this?

It does require a change to the Arduino code.

On Tue, Jun 17, 2014 at 9:57 PM, Stan notifications@github.com wrote:

Now that you mention this, it does seem odd to queue with no time. I have always seen this and assumed its fine. I was worried if it did not queue, then someone who forgot to put >0 in for time, would not understand why their tile was not queuing. At least now, they know its queuing and running for just a brief "flash".

— Reply to this email directly or view it on GitHub https://github.com/d8adrvn/smart_sprinkler/issues/17#issuecomment-46393658 .

— Reply to this email directly or view it on GitHub. — Reply to this email directly or view it on GitHub.

d8adrvn commented 10 years ago

HI Matt, actually, I added the code to the Arduino file last night. It was a real simple fix. So this issue can be closed.

mattnichols commented 10 years ago

Nice. Thanks!

On Jun 19, 2014, at 9:00 PM, Stan notifications@github.com wrote:

HI Matt, actually, I added the code to the Arduino file last night. It was a real simple fix. So this issue can be closed.

— Reply to this email directly or view it on GitHub.

mattnichols commented 10 years ago

Stan, I looks like strcmp does not always return 1 when strings are not equal. I had zones with 30 minutes not turn on. I looks like strcmp returns 3 in that case. So, I changed the check to this:

strcmp(inValue[2],"0")!=0

This seems to work flawlessly. Can you confirm?

d8adrvn commented 10 years ago

Matt, good catch. I was not aware of the additional states. Your fix works except when value is "null". I have added a second comparator to check for the null case. Also fixed #19 by splitting the comparators