Closed samstreak closed 3 years ago
Thanks for the feedback - I'm more an automation guy than a programmer, but I have done some C work on arduino things. Anyway, I'll take a look at updating these this week and get back to you.
On Sat, Dec 12, 2020 at 6:50 PM Jason Cheatham notifications@github.com wrote:
@jason0x43 requested changes on this pull request.
Hi, and thanks for contributing! (I finally got some free time to catch up on things...) I have a few suggestions/requests, and then this can be merged. Please let me know if you have any questions or disagree with any of my suggestions.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541801071:
@@ -37,6 +37,15 @@ metadata { command 'subscribe' command 'unsubscribe' command 'resubscribe'
- command 'updateAddress',
- [
- [name:"IP Address", type: "STRING", description: "New IP address", constraints: []]
- ]
- command 'updatePort',
- [
- [name:"TCP Port", type: "STRING", description: "New TCP port", constraints: []]
- ]
- }
This is an extra } (syntax error)
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541801187:
@@ -179,3 +188,84 @@ private debugLog(message) { private syncTime() { parent.childSyncTime(device) } + + +def updatePort(port) {
The code for this function should ideally go in the parent app so that all the drivers can use it. This function would then just call parent.childUpdatePort(child).
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541801205:
- } else {
- hPort=i2h(iPort)
- def existingPort = device.getDataValue('port')
- infoLog("Existing Port: ${h2i(existingPort)}")
- if (iPort && iPort != h2i(existingPort)) {
- debugLog("Updating port from ${h2i(existingPort)} to ${port}")
- device.updateDataValue('port', hPort)
- }
- } +}
+def updateAddress(ip) {
The code for this function should go in the parent app so that all drivers can use it.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541801879:
- if (ip && ip != hexToIp(existingIp)) {
- debugLog("Updating IP from ${hexToIp(existingIp)} to ${ip}")
- device.updateDataValue('ip', hexIp)
- }
- } +}
+private strArrToIntArr(strArr) {
- int[] intArr = new int[strArr.length];
- for (int i = 0; i < strArr.length; i++) {
- intArr[i]=s2i(strArr[i])
- }
- intArr +}
+private h2i(hex) {
This already exists in the parent app so isn't needed here.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541802325:
@@ -37,6 +37,15 @@ metadata { command 'subscribe' command 'unsubscribe' command 'resubscribe'
- command 'updateAddress',
- [
- [name:"IP Address", type: "STRING", description: "New IP address", constraints: []]
- ]
- command 'updatePort',
- [
- [name:"TCP Port", type: "STRING", description: "New TCP port", constraints: []]
This seems like it should be a number.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541802410:
@@ -179,3 +188,84 @@ private debugLog(message) { private syncTime() { parent.childSyncTime(device) } + + +def updatePort(port) {
- iPort=s2i(port)
If the port is a number, it wouldn't need to be converted here. Also, iPort needs a def.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541802898:
@@ -179,3 +188,84 @@ private debugLog(message) { private syncTime() { parent.childSyncTime(device) } + + +def updatePort(port) {
- iPort=s2i(port)
- if (iPort < 1 || iPort > 65535 ) {
- infoLog("Invalid TCP port specified - ${port}")
- -1
- } else {
- hPort=i2h(iPort)
hPort should have a def, and spaces around =
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541803036:
+} + +private strArrToIntArr(strArr) {
- int[] intArr = new int[strArr.length];
- for (int i = 0; i < strArr.length; i++) {
- intArr[i]=s2i(strArr[i])
- }
- intArr +}
+private h2i(hex) {
- //Hex to Integer
- Integer.parseInt(hex,16) +}
+private s2i(strV) {
If port is a number, this is only used in one place, and that usage could just be replaced with the Integer.parseInt call.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541803139:
- intArr[i]=s2i(strArr[i])
- }
- intArr +}
+private h2i(hex) {
- //Hex to Integer
- Integer.parseInt(hex,16) +}
+private s2i(strV) {
- //String to Integer
- Integer.parseInt(strV, 10) +}
+private hexToIp(hex) {
This exists in the parent, so isn't needed here.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541803805:
+private s2i(strV) {
- //String to Integer
- Integer.parseInt(strV, 10) +}
+private hexToIp(hex) {
- //Hex IP String to dotted decimal IP string
- [
- h2i(hex[0..1]),
- h2i(hex[2..3]),
- h2i(hex[4..5]),
- h2i(hex[6..7])
- ].join('.') +}
+private intArrToHex(arrAddress) {
Rather than defining an intArrToHex function, import the HexUtils class (add import hubitat.helper.HexUtils after the metadata block) and just call HexUtils.intArrayToHexString directly.
In drivers/jason0x43-wemo_switch.groovy https://github.com/jason0x43/hubitat/pull/11#discussion_r541804084:
+private hexToIp(hex) {
- //Hex IP String to dotted decimal IP string
- [
- h2i(hex[0..1]),
- h2i(hex[2..3]),
- h2i(hex[4..5]),
- h2i(hex[6..7])
- ].join('.') +}
+private intArrToHex(arrAddress) {
- //integer array (split IP address) to Hex string
- hubitat.helper.HexUtils.intArrayToHexString(arrAddress) +}
+private i2h(i) {
Rather than defining an i2h function, import the HexUtils class and just call HexUtils.integerToHexString directly.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jason0x43/hubitat/pull/11#pullrequestreview-550870002, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGXTK7BIPX3ZWMCF323PAUTSUP6UZANCNFSM4TIFSXPA .
I added the updates and updated the drivers for the insight switch and dimmer - wasn't sure if it was needed for the maker as I think they no longer support that device, and the motion sensor is discontinued.
Looks good. I merged this, then pushed another commit that updated the timestamps for the modified drivers and app.
Moved my wifi to a different subnet which prevented the SSDP broadcasts from coming through to update the device IPs. Added functions (with helper functions) to allow manual update of dotted decimal IP address (no hexIP input needed from the user) or decimal TCP Port (49153 is default). Copied partial functions from Wemo Connect app for Hex <=> IP dotted decimal info the driver.
If this is only useful for me, that's fine, it doesn't need to be integrated into the main line of code.