[Code Review?] Tailwind Garage Door

First post and first ever driver. I would love it if any of you would be willing to take a look at my driver and tell me what I'm doing horribly wrong. I'm not a developer and any comments on what I'm doing here would be fantastic. Thanks!

https://github.com/kaimyn/Tailwind_Hubitat

3 Likes

I was literally working on the same project last night, I think mines a bit cruder though. First time writing a hubitat driver too.
Gelix/HubitatTailwind (github.com)

Nice! I liked the idea of using the GarageDoorControl capability so that Hubitat would recognize the device as a garage door which led me down the entire child device path.

I didn't realize that you could poll from directly in the driver.. that might be nice, although polling once a minute seems like it would only be useful if it's left open and not as a trigger. I have my RM set to poll every 2 seconds right now. Any idea if that's going to destroy my Hubitat?

I'm definitely no expert on the hubitats either, it might be a bad practice to do what I'm doing. As for polling via 2s rule. You could use the runtime stats to compare to other devices/apps to see how that stacks up though? My gut instinct is it's a little too frequent, if not just for the tailwind controller. The 1 minute I have on mine is a bit too slow, so probably something around 5 - 10s?

I don't have this device and can't test the driver. But from just poking around the code, I can comment on a few things if your curious to see what someone else thinks:

attribute "Door 1 Status", "text"

An attribute type of "text" is not valid; you probably want "string". It's also conventional to "camel case" attribute names in Hubitat, starting with a lowercase letter and avoiding spaces, so something like "door1Status" instead of "Door 1 Status", but I'm not aware of any technical reason this would matter (except maybe the automatically-generated .currentX property, but that is the same as .currentValue("X") and I generally prefer to avoid the former anyway).

asynchttpPost("parseCmdResponse", params, cmd)

You are passing data (cmd) to your callback method but not using it there. This paramater is optional, so you can just omit it or pass null if you want to. Nothing functional to be lost or gained here except maybe the efficiency of not throwing things around that you don't need to. In the event that you used CoCoHue as the inspiration for these async HTTP method calls, CoCoHue does use this data in most cases (to generate events based on the data sent to the Bridge if it seems to have received it correctly), so that's why I do it in most cases there. :slight_smile:

//wait 15 seconds and force a refresh of the status in case the door failed to open or close
//there must be a better way of doing this
pauseExecution(15000)
getParent().refresh()

It looks weird, but I'm not sure it's necessarily better or worse. :slight_smile: However, personally, I'd use runIn() here. You'll need to provide a method name that exists in this driver to run, so maybe something like:

runIn(15, "doParentRefresh")

void doParentRefresh() {
  getParent().refresh()
}

sendEvent(name: "Door 1 Status", value: child1Status, displayed: true)

The displayed parameter is not used on Hubitat (you may see it on code copied from SmartThings), so it can be omitted. Optionally, you may wish to provide a friendly descriptionText attribute like "Door 1 is open". Finally, by convention, you might also wish to provide a preference that writes these events to the logs if the preference is enabled, which is by default in Hubitat's built-in drivers: log.info "Door 1 is open", in this example (but the descriptionText for the event, hence this preference's typical name: "Enable descriptionText logging.")

Along the same lines, stock Hubitat driver also include a preference for debug logging. By convention, this normally shows data coming into the hub from the device and can help a user troubleshoot something with you if you're curious what the actual data they're working with is (e.g., is not not coming in as expected, or is the driver just not parsing it into events correctly for some reason). I also find it helpful to log when commands are run as part of this (e.g., a simple log.debug "open()" at the top of the open() method if debug logging is enabled), but not all drivers do this, and your preferences may vary. Also by convention, debug logging is enabled by default when devices are first paired and automatically disabled 30 minutes after any time it gets enabled, including this. Hubitat's example drivers in their HubitatPublic repo include examples of this if you're curious.

These are just generally comments that mostly reflect my preferences and Hubitat convention, by the way--I don't see anything objectively wrong here, just wanted to offer some thoughts since you asked. :slight_smile: Nicely done, especially if you are new to the platform!

6 Likes

I'd echo @bertabcd1234 comments, and nice work on creating your first driver.

With logging I also like to keep my devices relatively quiet most of the time, so would also encourage you to include preferences to enable / disable informational and debug logging. Not sure it is the most efficient practice, but I introduced three additional methods in my drivers, debugLog, infoLog, errorLog, each take in the log message, check the appropriate preference and record the log if the preference is turned on.

My approach is to keep the information logging in plain language directed towards the user (not suggesting yours isn't), then with debug logging always starting the log message with the method name so I know exactly where it came from in the code, and adding any additional information that will help me as the developer with understanding what was happening, such as variable values, etc. Obviously the debug logs can also assist the user, so you can provide enough info in them for both target audiences.

Something for later that I would suggest is incorporating your code in HPM, but no rush for that just yet, become more comfortable with Groovy on HE first and then move on to things like HPM.

Again, well done!!

Simon

4 Likes

@chaue.shen I went a little different route with my implementation, feel free to steal any ideas you want if you like what I did better. (my code needs cleanup! iteration, iteration.)

Thanks, I already stole your status code array instead of a switch or logical anding. =)

Thanks! I really appreciate the feedback

1 Like

Couple of things I noticed that might be of use to you. In preferences when you specify a "number" (not integer!) you can specify a range like this:
input name: "doorCount", type: "number", title: "Number of Doors", required: "True", range: "0..3"
Also, with your child devices, you might have trouble with more than one controller (unique names).

Also, I just learned from you that you can specify a defaultValue! lol (I was doing it manually in installed()).

So far so good for me, 5 controllers, 12 doors. Started using controller name preference rather than IP, I mean, I'm as nerdy as the rest, but if my parents are going to use it, IP means nothing! Next step is figuring out how to do the child device names in a more descriptive manner than "Door 1 ,2,3"

2 Likes

You're totally right about the name collision issue. I've gone ahead and did something similar to you (no ":") but also stopped using the DNI as the door identifier and appended -1, -2, and -3 to the original devices DNI so that I can be pretty confident that it's globally unique.

Also, I think Tailwind updated the firmware. Door 3 is now actually 3 instead of 4 in the api, so opening or closing it returns 3 and -3 instead of 4 and -4.

What firmware are you showing in the app? I'm on IQ3/9.87, and still seeing 4 and -4.
I think the way I wrote things (I don't really care what the value is, it is the 3rd value in the array for door 3) it shouldn't even matter after adding to the statusCode. I can support both that way, I like it!
statusCodes=[
[-1, -2, -4],
[1, -2, -4],
[-1, 2, -4],
[1, 2, -4],
[-1, -2, 4],
[1, -2, 4],
[-1, 2, 4],
[1, 2, 4],
[-1, -2, -3],
[1, -2, -3],
[-1, 2, -3],
[1, 2, -3],
[-1, -2, 3],
[1, -2, 3],
[-1, 2, 3],
[1, 2, 3]
]
retVal = statusCodes[status][door]

Where status is returned from POST, and door is the literal door number (1 or 2 or 3). I don't even check what retVal is, it's either positive or negative (open or closed).

def getDoorOpenClose(curStatus)
{
if(curStatus < 0){
return "closed"
}
else{
return "open"
}
}

Very weird. I'm also on IQ3/9.87, but I'm having to send 3 in the POST body in order to open door 3, which gives me a +/-3 response. Sending a 4 gives me a 408 status response. Kind of concerning...

You know, I haven't actually been testing a door #3 much, I'll look into that sometime on Monday. I have one of the #3 doors with a sensor, but no automated door opener. The other is the hay barn out on the farm.

edit: confirmed, sending 3 does nothing to control my door at the hay barn. It does respond with a 3 though lol.

image
Fancy pants names on the child devices now. Basically just changing the label to match a preference set on the main driver. No more Door 1, 2, 3 for me! I should probably spend more time on making my version actually functional again, I broke a few things trying to make it smarter with polling. 10 - 15 seconds is too much IMO. My goal is to poll once a minute, or 5 minutes (or longer), and if it receives an open/close command to spam refresh until it closes (or times out).

1 Like

Tailwind updated their firmare from 9.87 to 9.95 and now has broken the Hubitat device driver. Do you see any problems on your side or from anyone using your driver via the Hubitat Package Manager? I have reached out to Scott at Tailwind and have not heard back. I have also factory reset the Tailwind device. If you have any ideas to resolve this I would appreciate it.

Thanks.

Unfortunately, the torsion spring in my garage literally snapped so I'm not able to test things out. Let me know if you find something out.

I will work on it this week as time allows.

So it turns out that my tailwind won't update past 9.87. I'll keep my eye on things to see if an update comes through, but for now I can't really help.

Mine is updated, I can check tomorrow. The only thing I've noticed so far is that it is returning a different status of 10 (on paper API shows 7 being max). Based on that I suspect the API was changed.
edit:
Status does NOT change based on doors being open or closed, always returns status of 10. So something is either broken or was completely changed in the API. I do remember them mentioning security being implemented at some point, maybe that's what's going on. Either way, going to need some help from tailwind on this.