WWST-6892, WWST-6896, WWST-6898, WWST-6904 - New DTH for Viconics/Schneider Room Controllers #45873
Conversation
Support for Viconics VT8650, Schneider Electric: SE8350 and SE8650. Reading more attributes after thermostat mode and fan speed changes. Changes in configuration.
KKlimczukS
changed the title
|
|
||
| def configurationCommands = [] | ||
| // todo: check if following binding is necessary here | ||
| configurationCommands += "zdo bind 0x${device.deviceNetworkId} 1 0xA 0x201 {${device.zigbeeId}} {}" |
There was a problem hiding this comment.
I think zigbee.addBinding(_CLUSTER_) might also work here.
There was a problem hiding this comment.
Since binding is added in zigbee.configureReporting() method those binding commands probably will not be needed here, but i will take another look at this (remove those commands first and check if all works correctly)
There was a problem hiding this comment.
I think @PKacprowiczS is correct.
There was a problem hiding this comment.
Yes, zigbee.configureReporting has a call to zigbee.addBinding as part of it. So in this case, clusters 0x201 and 0x405 will be handled with all of the zigbee.configureReporting(THERMOSTAT_CLUSTER and zigbee.configureReporting(RELATIVE_HUMIDITY_CLUSTER calls below (in fact, 13 times for the thermostat cluster). So these calls are superfluous.
There was a problem hiding this comment.
Yes, I agree. As expected, I removed those zdo bind commands.
| eventMap = [:] | ||
| def descMap = zigbee.parseDescriptionAsMap(description) | ||
|
|
||
| if ((descMap.clusterInt == THERMOSTAT_CLUSTER && descMap.attrId)) { |
There was a problem hiding this comment.
nit: unnecessary double brackets
| private void createChildThermostat() { | ||
| log.debug "Creating child thermostat to handle unoccupied cooling/heating setpoints" | ||
| def label = "Unoccupied setpoints" | ||
| def childName = "${device.displayName} " + label |
There was a problem hiding this comment.
nit: maybe just "${device.displayName} ${label}"?
| log.debug "installed" | ||
| sendEvent(name: "checkInterval", value: 2 * 60 * 60 + 2 * 60, displayed: false, data: [protocol: "zigbee", hubHardwareId: device.hub.hardwareID, offlinePingable: "1"]) | ||
|
|
||
| if (isViconicsVT8350()|| isSchneiderSE8350()) { |
There was a problem hiding this comment.
nit: space between isViconicsVT8350() and ||
There was a problem hiding this comment.
fixed, thank You.
| capability "Occupancy Sensor" | ||
| capability "Temperature Measurement" | ||
| capability "Relative Humidity Measurement" | ||
| capability "Thermostat" |
There was a problem hiding this comment.
I think you should just leave this deprecated capability off
There was a problem hiding this comment.
I'm not sure about it. When I got rid of that line, all other plugin elements that are related to the Thermostat weren't displayed.
There was a problem hiding this comment.
some metadata is defined by referencing the Thermostat attributes, but others, such as Z-Wave Battery Thermostat, specifically reference the discrete capabilities. You might just need to poke at the metadata bit.
There was a problem hiding this comment.
@greens i will give it a try ( merge request with UI metadata wasn't created yet)
There was a problem hiding this comment.
Yeah, if the metadata references /capability/thermostat/X where X is some number then it is referring to the Thermostat capability. If you drop this and use metadata that references the individual capabilities it should work.
There was a problem hiding this comment.
Thank You @dkirker, I removed those references from UI metadata and it works now.
|
|
||
| def installed() { | ||
| log.debug "installed" | ||
| sendEvent(name: "checkInterval", value: 2 * 60 * 60 + 2 * 60, displayed: false, data: [protocol: "zigbee", hubHardwareId: device.hub.hardwareID, offlinePingable: "1"]) |
There was a problem hiding this comment.
should not be offlinePingable
| "02":"Override", | ||
| "03":"Standby" |
There was a problem hiding this comment.
These last two are not legal values for the capability.
| if (description?.startsWith("humidity: ")) { | ||
| // Viconics VT8350 humidity reports are parsed as floating point numbers (range 0 - 1%) | ||
| def humidityVal = (description - "humidity: " - "%").trim() | ||
| if (humidityVal.isNumber()) { | ||
| humidityVal = new BigDecimal(humidityVal) * 100 | ||
| } | ||
| eventMap.name = "humidity" | ||
| eventMap.value = humidityVal | ||
| eventMap.unit = "%" | ||
| } |
There was a problem hiding this comment.
Can you just handle this the way you're handling the other events? i.e. using the results of parseDescriptionAsMap?
There was a problem hiding this comment.
yes, it will be done as you suggest.
| if (descMap.clusterInt == THERMOSTAT_CLUSTER && descMap.attrId) { | ||
| def attributeInt = zigbee.convertHexToInt(descMap.attrId) | ||
|
|
||
| if (attributeInt == OCCUPANCY || attributeInt == CUSTOM_EFFECTIVE_OCCUPANCY) { |
There was a problem hiding this comment.
I think you should be using a switch statement here. If you're clever about placement of breaks, you should be able to condense this code significantly.
There was a problem hiding this comment.
good point, fixed
| } else if (isViconicsVT8650() || isSchneiderSE8650()) { | ||
| switch (mode) { | ||
| case "on": | ||
| getThermostatFanModeCommands(CUSTOM_FAN_MODE_ON) |
There was a problem hiding this comment.
I think you should avoid a switch statement here and delegate this lookup of the fan mode value to the getThermostatFaneModeCommands function you defined.
There was a problem hiding this comment.
getThermostatFaneModeCommands receives constant hex values, but a map will be in the middle - that way switch statement won't be used here.
| delayBetween([ | ||
| zigbee.writeAttribute(THERMOSTAT_CLUSTER, CUSTOM_THERMOSTAT_MODE, DataType.ENUM8, THERMOSTAT_MODE_OFF), | ||
| zigbee.readAttribute(THERMOSTAT_CLUSTER, CUSTOM_THERMOSTAT_MODE), | ||
| zigbee.readAttribute(THERMOSTAT_CLUSTER, CUSTOM_THERMOSTAT_OPERATING_STATE) | ||
| ], 500) |
There was a problem hiding this comment.
seems like you could use the same pattern for these 4 that you did for the fan modes.
|
|
||
| def ping() { | ||
| log.debug "ping" | ||
| refresh() |
There was a problem hiding this comment.
the ping method should not be querying all of these attributes. Just temperature is fine.
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, CUSTOM_FAN_MODE, DataType.ENUM8, 1, 60, 1) | ||
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, CUSTOM_FAN_SPEED, DataType.ENUM8, 1, 60, 1) | ||
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, CUSTOM_THERMOSTAT_OPERATING_STATE, DataType.ENUM8, 1, 60, 1) | ||
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, OCCUPANCY, DataType.ENUM8, 1, 60, null) |
There was a problem hiding this comment.
nit: spacing difference here
| } | ||
| } | ||
|
|
||
| def mapFanSpeedSliderValue(rawValue) { |
There was a problem hiding this comment.
you could just make this an actual map
| } | ||
|
|
||
| def configure() { | ||
| parent.configureChild() |
devicetypes/smartthings/child-thermostat-setpoints.src/child-thermostat-setpoints.groovy
Show resolved
Hide resolved
...smartthings/viconics-schneider-room-controller.src/viconics-schneider-room-controller.groovy
Show resolved
Hide resolved
| break | ||
| default: | ||
| log.debug "UNHANDLED ATTRIBUTE, descMap.inspect(): ${descMap.inspect()}" | ||
|
|
There was a problem hiding this comment.
You've got a blank line here
| celsius = (celsius as Double).round(2) | ||
|
|
||
| delayBetween([ | ||
| zigbee.writeAttribute(THERMOSTAT_CLUSTER, setpointAttr, DataType.INT16, zigbee.convertToHexString(celsius * 100)), |
There was a problem hiding this comment.
Let's bring this indenting to the left one so there isn't so much wasted space
| def getThermostatFanModeCommands(mode) { | ||
| if (mode) { | ||
| delayBetween([ | ||
| zigbee.writeAttribute(THERMOSTAT_CLUSTER, CUSTOM_FAN_MODE, DataType.ENUM8, mode), |
There was a problem hiding this comment.
Same with the indenting here
| speed = speed - 1 | ||
| } | ||
| delayBetween([ | ||
| zigbee.writeAttribute(THERMOSTAT_CLUSTER, CUSTOM_FAN_SPEED, DataType.ENUM8, speed), |
| def setFanSpeed(speed) { | ||
| log.debug "setFanSpeed: ${speed}" | ||
|
|
||
| if (speed == 0 || speed >= 4) { //if by any chance user selects 0 or a value higher than 3, it fan will be set to AUTO |
There was a problem hiding this comment.
it fan will be set to AUTO -> the fan will be set to AUTO
So there is no off then?
There was a problem hiding this comment.
No, the only possible options are: Low, Med, High, Auto, On (user can also configure the device so the concrete "fan sequence" can be set manually : On-Auto, L-M-H, L-H, L-M-H-A and L-H-A).
| def getThermostatModeCommands(mode) { | ||
| if (mode) { | ||
| delayBetween([ | ||
| zigbee.writeAttribute(THERMOSTAT_CLUSTER, CUSTOM_THERMOSTAT_MODE, DataType.ENUM8, mode), |
There was a problem hiding this comment.
Same with the indenting here
|
|
||
| def configurationCommands = [] | ||
| // todo: check if following binding is necessary here | ||
| configurationCommands += "zdo bind 0x${device.deviceNetworkId} 1 0xA 0x201 {${device.zigbeeId}} {}" |
There was a problem hiding this comment.
Yes, zigbee.configureReporting has a call to zigbee.addBinding as part of it. So in this case, clusters 0x201 and 0x405 will be handled with all of the zigbee.configureReporting(THERMOSTAT_CLUSTER and zigbee.configureReporting(RELATIVE_HUMIDITY_CLUSTER calls below (in fact, 13 times for the thermostat cluster). So these calls are superfluous.
| if (temperatureScale == "C") { | ||
| celsius//Math.round(celsius) | ||
| } else { | ||
| celsiusToFahrenheit(celsius)//Math.round(celsiusToFahrenheit(celsius)) |
There was a problem hiding this comment.
I know in some other device handlers there is rounding logic. Are we not wanting to do that here? If not, let's remove the comments.
There was a problem hiding this comment.
@dkirker Sure.
Actually setpoints can be changed manually with a step = 0.5
Additionally - since the device displays his values on the screen with only one digit after the decimal point and reports its values with 2 digits after decimal point - it' looks really bad if You want to change setpoints on the ST UI (step = 0.5), for example: 18.72 <-19.22 -> 19.72
so those values will be rounded to the nearest half (for example: 19.22 >> 19.00; 19.32 >> 19.50)
We will also configure setpoints initially to rounded default values so the device would work on "rounded" numbers from the very beginning.
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, CUSTOM_HUMIDITY, DataType.UINT16, 1, 60, 10) | ||
| configurationCommands += zigbee.configureReporting(RELATIVE_HUMIDITY_CLUSTER, RELATIVE_HUMIDITY_MEASURED_VALUE, DataType.UINT16, 1, 60, 5) | ||
|
|
||
| delayBetween(getRefreshCommands()+configurationCommands) |
There was a problem hiding this comment.
Space around the + for readability.
…o nearest half. Code refactoring.
|
|
||
| private getTHERMOSTAT_FAN_MODE_ATTRIBUTE_ID_MAP() { | ||
| [ | ||
| "on": 0x00, // CUSTOM_FAN_MODE_ON |
There was a problem hiding this comment.
Can you use the enums you created in these maps? You should also be using them for the keys in the other maps, but you'll obviously have to cast to ints when retrieving the values, but I think if you're going to define all these magic numbers you should not have magic numbers in the code.
There was a problem hiding this comment.
Sure, but I had to add some new constants.
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, CUSTOM_HUMIDITY, DataType.UINT16, 1, 60, 10) | ||
| configurationCommands += zigbee.configureReporting(RELATIVE_HUMIDITY_CLUSTER, RELATIVE_HUMIDITY_MEASURED_VALUE, DataType.UINT16, 1, 60, 5) |
There was a problem hiding this comment.
I think you can remove the reporting configuration using the humidity cluster, since you don't handle it in the parse method.
There was a problem hiding this comment.
Sure, fixed (i kept those configuration commands for testing purposes).
| configurationCommands += setSetpoint(initialCoolingSetpoint, COOLING_SETPOINT_UNOCCUPIED) | ||
| configurationCommands += setSetpoint(initialHeatingSetpoint, HEATING_SETPOINT_UNOCCUPIED) | ||
|
|
||
| configurationCommands += zigbee.configureReporting(THERMOSTAT_CLUSTER, CUSTOM_THERMOSTAT_MODE, DataType.ENUM8, 1, 60, 1) //formerly THERMOSTAT MODE: 0x001C |
There was a problem hiding this comment.
I just realized that you're setting the max reporting interval for all of these attributes at a minute. That's way too low. That requires them to report every minute, even if there's no change. If your min reporting interval is 1 second and the delta is 1 they'll report every change anyway, so please raise the max reporting interval to at least an hour.
If the device is sending us like 20 updates every minute it will be an incredibly expensive device to run.
There was a problem hiding this comment.
@greens Good point (max reporting was set to short period for testing purposes).
| (THERMOSTAT_MODE_AUTO):"auto", | ||
| (THERMOSTAT_MODE_COOL):"cool", | ||
| (THERMOSTAT_MODE_HEAT):"heat", | ||
| "04":"heat" |
There was a problem hiding this comment.
I'm still not sure about that one. Viconics/Schneider uses this value (i saw it in their very old dths), but it's not mentioned in their manual.
I left this value intentionally (for testing purposes), but it will be removed probably.
There was a problem hiding this comment.
did you still want to remove it? It's still a string and I think all the other indices are not.
There was a problem hiding this comment.
I performed few tests and the device consumed 0x04 and 0x05 values, but after a while it switched to previous mode.
Additionally: values >= 0x06 were rejected (device didn't even report setting them).
Anyway, i removed "04" already.
| private getEFFECTIVE_OCCUPANCY_MAP() { | ||
| [ | ||
| (OCCUPANCY_OCCUPIED):"Occupied", | ||
| (OCCUPANCY_UNOCCUPIED):"Unoccupied" |
There was a problem hiding this comment.
I believe these are not capitalized in the capability.
|
|
||
| private getFAN_SPEED_SLIDER_MAP() { | ||
| [ | ||
| (FAN_SPEED_LOW): 1, |
There was a problem hiding this comment.
This map is not necessary. Just refactor the code in the one place that you use it.
There was a problem hiding this comment.
In that case, we can get rid of it and just use "value +1" instead (because the device reports values in the range 0-3, where 0 is LOW fan speed).
There was a problem hiding this comment.
Reported speeds are in range 0-2 (0 is LOW, not OFF), so it has to be incremented +1 to indicate (on the UI/slider) that the fan is working, if it stayed at 0 - it wouldn't be the best UX.
There was a problem hiding this comment.
ah yeah, it's passed in the event later
I was just looking at the if statement
either way, this map should go away
| def descMap = zigbee.parseDescriptionAsMap(description) | ||
|
|
||
| if (descMap.clusterInt == THERMOSTAT_CLUSTER && descMap.attrId) { | ||
| def attributeInt = zigbee.convertHexToInt(descMap.attrId) |
There was a problem hiding this comment.
That's because in most cases there are no attrInt in the map returned by zigbee.parseDescriptionAsMap(description)
so it had to be that way:
def attributeInt = zigbee.convertHexToInt(descMap.attrId)
There was a problem hiding this comment.
I'm looking at the code for parseDescriptionAsMap
if (outMap.containsKey("attrId")) {
outMap["attrInt"] = convertHexToInt(outMap["attrId"])
}
If this is what you're seeing, I want to see the case in which you are getting a message that is parsing out to have an attrId but not an attrInt.
There was a problem hiding this comment.
It was noticed mostly when OCCUPIED reports were processed, that's why i reverted that change.
When i checked it today on a side, it turned out it worked fine:
example code:
def description = "read attr - raw: 230A0A02010A0200001800, dni: 230A, endpoint: 0A, cluster: 0201, size: 10, attrId: 0002, result: success, encoding: 18, value: 00" def descMap = zigbee.parseDescriptionAsMap(description) log.debug "test descMap.inspect(): ${descMap}"
That was a strange behavior, because yesterday attInt was null in most cases for a while.
Anyway, i'm reverting those changes for now, but i will keep an eye on it.
...smartthings/viconics-schneider-room-controller.src/viconics-schneider-room-controller.groovy
Outdated
Show resolved
Hide resolved
…neider Room Controllers (SmartThingsCommunity#45873) * WWST-6898 - New DTH for Viconics Schneider Room Controller * Support for capabilities: occupancy, fan mode. Support for Viconics VT8650, Schneider Electric: SE8350 and SE8650. Reading more attributes after thermostat mode and fan speed changes. Changes in configuration. * Fixes spacing * Renames child device to Child Thermostat Setpoints * Renames child device to Child Thermostat Setpoints * Fixes processing occupancy reports. * small refactoring * code refactoring * reverted capability Thermostat * Removed configuration from child dth. * Configure setpoint values initially. Round incoming setpoint values to nearest half. Code refactoring. * Remove blank line * fix * fix * Change max reporting interval to 3600 seconds. Clean up the code. * Code refactoring. Removed unnecessary configuration commands. * Fix, code refactoring. * Fix * Fix * Removes unnecessary value from thermostat mode map. Fixes temperature reporting.
New DTH for Viconics/Schneider Room Controllers:
WWST-6898 - VT8350 - Viconics Technologies - Low Voltage Fan Coil Controller and Zone Controller
WWST-6892 - VT8650 - Viconics Technologies - Heat Pump and Indoor Air Quality Controller
WWST-6904 - SE8350 - Schneider Electric - Low Voltage Fan Coil Unit (FCU) and Zone Control
WWST-6896 - SE8650 - Schneider Electric - Roof Top Unit Controller
This is the first working version of the DTH for Viconics/Schneider Room Controllers.
Since:
@ZWozniakS @MWierzbinskaS
Please let me know if it works correctly with your devices.
@SmartThingsCommunity/srpol-pe-team
@tpmanley @greens @dkirker
Please, could You take a look at the code ?