tagyoureit / nodejs-poolController

An application to control pool equipment from various manufacturers.
GNU Affero General Public License v3.0
327 stars 97 forks source link

Add ability to set clock/time #14

Closed tagyoureit closed 7 years ago

tagyoureit commented 7 years ago

Moved from https://github.com/tagyoureit/nodejs-Pentair/issues/20

@arrmo Add a socket.emit for this (and socket.on in index.js)? Send the same format ... h:m (h is 24h mode)?

Thanks!

tagyoureit commented 7 years ago

This is ready for testing (3.1.7 dev branch). See README.md for setDateTime socket command. Available as both a socket and URI. The formats are slightly different. For the URI the format is /datetime/set/time/:hh/:mm/date/:dow/:dd/:mon/:yy/:dst. The parameters are the same as in README.md.

@arrmo Want to test and if it works implement in bootstrap UI?

arrmo commented 7 years ago

Absolutely! Will do.

arrmo commented 7 years ago

OK, I think I'm all set up to do this - just need to get the dang directories thing working first (then can push the code)!

arrmo commented 7 years ago

OK, just trying to call the API now, one question before I do. The interface is as below, setDateTime(hour, min, dow*, day, mon, yy, dst)

But ... do I have to set all of the values, or if I only set some will it work (e.g. only change date, or time, but not dow or dst)?

Thanks!

tagyoureit commented 7 years ago

The way it is setup now... yes. I thought about even making them separate calls but decided on the easiest route to get started.

arrmo commented 7 years ago

Yep, NP - I can set all of them ... the only one that worries me a bit is DST, not sure how to determine if that is on or not ... :-(

arrmo commented 7 years ago

Never mind! ... ;-). I see you pass it in time - so I'll gate this on getting time input once, play the same value back.

arrmo commented 7 years ago

OK, struggling a bit with the API - does this look right? lastDT = last reported time ('time' received), selectedDate = new date to set.

socket.emit('setDateTime', lastDT.getHours(), lastDT.getMinutes(), selectedDate.getDay(), selectedDate.getDate(), selectedDate.getMonth(), selectedDate.getFullYear(), autoDST);

Just want to make sure I'm sending the right info across.

arrmo commented 7 years ago

OK, just to confirm before I add this in (from npm) ... you OK with this for a time picker (for setting the current time)? https://www.npmjs.com/package/jquery-clockpicker

tagyoureit commented 7 years ago

I'm mobile right now so just go with the package if you like it. The API call might not be right. The DOW should be a # and year should only be two digits. But hard to tell without looking at code.

arrmo commented 7 years ago

No worries! Let me get date working first. That helps on the year ... and are they strings, or integers? I'm sending integers.

arrmo commented 7 years ago

Hmmm ... trying this from the web interface, here is the URL, http://localhost:3000/datetime/set/time/:12/:15/date/:0/:12/:2/:17/:0

And the output, {"text":"FAIL: hour (NaN) should be 0-23 and minute (NaN) should be 0-59. Received: NaN:NaNDay (NaN) should be 0-31, month (NaN) should be 0-12 and year (NaN) should be 0-99.Day of week (NaN) should be one of: [1,2,4,8,16,32,64] [Sunday->Saturday]dst (NaN) should be 0 or 1"}

Thoughts?

arrmo commented 7 years ago

OK, figured it out! Remove the colons (clean them out of the README file?), but also, Day of week (0) should be one of: [1,2,4,8,16,32,64]

=> not 0-7, likely me misunderstanding!

tagyoureit commented 7 years ago

Leave off the ':'. Check the tests for examples but looks like you are close. https://github.com/tagyoureit/nodejs-poolController/blob/3.1.x-DEV/specs/lib/comms/server-set.spec.js

arrmo commented 7 years ago

Yep, very close! URL working, just note the socket.io yet. Thanks!

arrmo commented 7 years ago

OK, working! Will push very soon .. please check it out, let me know what you think. Let's not close it yet, I'd like to use bootstrap-clockpicker, not jquery-clockpicker ... for theme reasons, but bootstrap is not in npm?

BTW, after a set, can you send out a time update, so all gets updated? I can do it also, but a "reply" may be a good thing?

arrmo commented 7 years ago

In PR #24.

tagyoureit commented 7 years ago

BTW, after a set, can you send out a time update, so all gets updated? I can do it also, but a "reply" may be a good thing?

Yes! This is easy. And a bigger project on my horizon is figuring out how to send responses back to the UI of actual changes (or failures). Right now I'm just sending back the confirmation that the request was received (and sent to the serial bus).

tagyoureit commented 7 years ago

Hi, tested this out a little bit... looks great and it works when I click through the UI, but did see a few small items. I also added some items (noted below) on top of your code so pull them down to test as well (https://github.com/tagyoureit/nodejs-poolController/tree/arrmo-3.1.x-DEV).

  1. It looks like you made the date a button, but it doesn't do anything... wasn't clear if that was supposed to be part of this PR. Is there a missing dependency in package.json?

  2. When the date first loads, it comes up with NaN as the date, but time comes up ok. **I may have fixed this with my updates to add 'get time' to the list of initial intellitouch commands to retrieve.

    screen shot 2017-02-13 at 1 56 35 pm
  3. When you display the time (for the user changing it), it selects the AM time when it should be selecting PM. EG, the current time is 1:30pm, but the time picker selects 1:30 (but it should pick 13:30).

    screen shot 2017-02-13 at 1 59 02 pm
  4. Maybe have a small 'x' or cancel button as it isn't clear you can just click outside the model window to close the dialogue.

  5. I added code to automatically retrieve the time from the controller as soon as we set it. This means you should probably not set it from the UI but let the app pass the new time back to the client. **Pull this change down in the commit mentioned above.

cheers!

arrmo commented 7 years ago

Thanks so much for checking this out - and appreciate the comments (I really want you to be critical, so we can get it all working right). BTW, not an issue, but how did you pull this in without accepting the PR? Just trying to learn ... :-). I think it may be you creating a new branch, git still hurts my head!

OK, that aside, my thoughts (and thanks for numbering, makes it easier to reply),

  1. Ummm, OK, lost on this one. Here is what I see when I click that button - but you don't see it working. That's very odd. Any errors in the console? Trying to understand ... :-(. image
  2. Excellent. I don't see that, but I'll pull in your code in any case! BTW, did pull in your code, so it will show up in the PR when I push again (happens automatically). Just so you know.
  3. Agree with you, thanks! OK, seems to be a bug with the input selected by the widget itself. I tried changing to clockpicker-seconds (really has bootstrap included like it should, updated more recently, and provides for AM/PM). Struggling with it ... do we try to spin off of our own? Really can't believe it's this hard to find a time selector (date is great, and part of jQuery UI!). Thoughts?
  4. Agreed! BTW, you can also hit escape ... :-). This also seems to be a limitation of the picker itself though. Really need to find a better time picker. Open to suggestions!
  5. You bet, done!

Comments? And again, thanks!

arrmo commented 7 years ago

Arrgh ... OK, back to the old time-picker, the new one has even more bugs. Dang it!

arrmo commented 7 years ago

Yep, the old one works ... if the input is in 24 hour time. Doesn't seem to accept AM/PM.

tagyoureit commented 7 years ago

On the checking out a PR... yes, basically I created a new branch. You can click on the "command line instructions" and it tells you how. ![Uploading Screen Shot 2017-02-13 at 5.13.06 PM.png…]()

(I did "blow out" my old branch accidentally in the process... so I'm no expert either... but se-la-vie.) Will check out the changes in a little bit.

Also, I just did another commit to the PR branch that I checked out... so, do you pull that and update your PR??? Or do I need to merge these changes? Now I'm confused. :-)

tagyoureit commented 7 years ago

So, after a little more testing... it works fine in Chrome (just found out). But nothing happens in Safari v10. So seems like it is cross-browser issue... Absolutely no errors in Safari at all.

arrmo commented 7 years ago

Bingo! OK, that makes sense - and I'm not completely lost ... ;-). Just checked Firefox and IE, both work fine. What the heck is Safari doing?

In any case, I think I figured out a workaround for the AM/PM thing, thought of it on the drive in today. Let's see if I get time tonight to test it out ... ;-)

arrmo commented 7 years ago

OK, new push coming - to get it working, I have to convert 12/24 hour (display vs. widget input). Check it out ... Chrome, not Safari ... LOL.

arrmo commented 7 years ago

OK, updated to use the bootstrap version - matches better, will work with themes later. Move to this? If so, remove jquery-clockpicker from npm list (and uninstall)?

If this looks good, then please accept PR #25, and then I'll start to move on to Schedule (that's up next, right?)

tagyoureit commented 7 years ago

Yes, looks great and matches the UI. I'll close this and remove jquery-clockpicker from the package.json. Good work!