Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WWST-1595 Zigbee Accessory Dimmer #3465

Merged
merged 12 commits into from Oct 2, 2018
Merged

Conversation

greens
Copy link
Contributor

@greens greens commented Sep 11, 2018

Primary intention is to work with the Aurora Wireless Wall Remote

Primary intention is to work with the Aurora Wireless Wall Remote
@greens
Copy link
Contributor Author

greens commented Sep 11, 2018

Here's the first draft.

Primary intention is to work with the Aurora Wireless Wall Remote
// def events = []
// for (currentLevel..target).step(step).each{
// events << createEvent(name: "level", value: currentLevel + it)
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aborted attempts at the faked intermediate step events

sendEvent(name: "switch", value: "off")
}
}
} else if (descMap.commandInt == 0x01) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're essentially turning the middle buttons (when held) into "go to max" and "go to min"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with nothing in-between

fine tuning can be handled by the single press events

if (device.currentValue("level") == 0) {
sendEvent(name: "level", value: STEP)
}
sendEvent(name: "switch", value: device.currentValue("switch") == "on" ? "off" : "on")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This device has explicit On and Off buttons, correct? If so I think it may be better to create the on event on receipt of the On command rather than toggling. Should also force each event to be a state change event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only problem is that then we have to maintain the state and there's no way to get that from the device. (and it's not indicated on the device anywhere)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this device have separate on and off buttons or is it just a single button that toggles between on/off?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a single button

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I commented above, we can use the command it sends us to know. If the command ID is 0x01 it's an on and 0x00 is an off. I think we could consider the argument that we should explicitly mark the event with isStateChange: true as well, but I don't feel too strongly on that.

} else if (descMap.data[0] == "01") {
log.debug "move down"
value = Math.max(currentLevel - STEP, 0)
sendEvent(name: "level", value: value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually don't create level events for a value of 0 because when they are turned on it is still 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a little futzing about to make sure that if you turn it on from level = 0, level goes up to 10, but avoiding that situation in the first place is probably the better choice.

}

def off() {
sendEvent(name: "switch", value: "off")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add isStateChange: true to all these events

def configure() {
log.debug "Configuring Reporting, IAS CIE, and Bindings."
def cmds = []
return zigbee.onOffConfig() +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these configs will do anything since the device doesn't have On/Off or Level cluster servers, just clients.

capability "Switch"
capability "Button"
capability "Switch Level"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to add Health Check might be able to setup attribute reporting on any implemented attribute in the Basic cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried doing read attributes of basic cluster, but wasn't getting a response.

@greens
Copy link
Contributor Author

greens commented Sep 25, 2018

Updated. Still can't get the device to report any attributes in the basic cluster, so I don't think this device can support health check.

@greens
Copy link
Contributor Author

greens commented Sep 26, 2018

Waiting for a final review from somebody.

import physicalgraph.zigbee.zcl.DataType

metadata {
definition (name: "ZigBee Accessory Dimmer", namespace: "smartthings", author: "SmartThings", ocfDeviceType: "x.com.st.d.remotecontroller", runLocally: false, minHubCoreVersion: '000.019.00012', executeCommandsLocally: false, mnmn: "SmartThings", vid: "generic-dimmer") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove minHubCoreVersion and executeCommandsLocally since it doesn't run locally.

}

def setLevel(value) {
if (value != 0) sendEvent(name: "switch", value: "on")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to set switch to off if level is 0

@greens
Copy link
Contributor Author

greens commented Sep 26, 2018

Done.

sendEvent(name: "level", value: STEP)
}
sendEvent(name: "switch", value: device.currentValue("switch") == "on" ? "off" : "on")
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this else can be removed


def setLevel(value) {
if (value != 0) sendEvent(name: "switch", value: "on")
else if (value == 0) sendEvent(name: "switch", value: "off")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could simplify to just else

def setLevel(value) {
if (value != 0) sendEvent(name: "switch", value: "on")
else if (value == 0) sendEvent(name: "switch", value: "off")
sendEvent(name: "level", value: value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually don't set level to 0


def installed() {
sendEvent(name: "switch", value: "off", isStateChange: false, displayed: false)
sendEvent(name: "level", value: 0, isStateChange: false, displayed: false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually the default level is 100

} else {
def descMap = zigbee.parseDescriptionAsMap(description)
if (descMap && descMap.clusterInt == 0x0006) {
if (descMap.commandInt == 0x01 || descMap.commandInt == 0x00) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it use both of these commands? Seems odd if it has one button that can generate two different commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but I think it's for talking to actual devices that are keeping on/off so it keeps track of state internally

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we want to track state separately or just use what the device gives us. If they have other bulbs that they are controlling directly from the remote it would be weird for us to be generating an "off" event when the bulbs are actually getting an on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, if it's tracking state on it's own I think we should use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended operation with Aurora's hub is to control other lights, I assume.

I don't know any way they'd be able to do this on our system, though. And there's no outward indication on the device or any way for us to determine it's internal state.

Tracking the device's on/off state, but just having a purely invented level value seems counterintuitive to me and also removes the ability to use the app as a "virtual" on/off switch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I see your point

device needs custom metadata for button automations in OneApp

def configure() {
// strangely, these are necessary to have the device report when its buttons are pressed
zigbee.onOffConfig() + zigbee.levelConfig() + zigbee.configureReporting(0x0005, 0x0001, DataType.UINT8, 1, 3600, null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing you could call zigbee. addBinding(0x0006), etc.... Since this device doesn't have reportable attributes it's only the binding that should matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, that was it

def configure() {
// strangely, these are necessary to have the device report when its buttons are pressed
zigbee.addBinding(0x0006) + zigbee.addBinding(0x0008) + zigbee.addBinding(0x0005)
// zigbee.onOffConfig() + zigbee.levelConfig() + zigbee.configureReporting(0x0005, 0x0001, DataType.UINT8, 1, 3600, null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this commented line. You could also change the above to use zigbee.ONOFF_CLUSTER, zigbee.LEVEL_CONTROL_CLUSTER for the first 2. We don't have a constant currently for the scenes cluster though.

def installed() {
sendEvent(name: "switch", value: "on", isStateChange: false, displayed: false)
sendEvent(name: "level", value: 100, isStateChange: false, displayed: false)
sendEvent(name: "button", value: "pressed", isStateChange: false, displayed: false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to create a numberOfButtons event too.

}

def configure() {
// strangely, these are necessary to have the device report when its buttons are pressed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation looks off with the line below

if (value == 0) {
sendEvent(name: "switch", value: "off")
} else {
sendEvent(name: "switch", value: "on")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation looks off

tpmanley
tpmanley previously approved these changes Oct 2, 2018
Copy link
Contributor

@tpmanley tpmanley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of final minor things and then it looks good

@greens greens merged commit e1ab3fe into SmartThingsCommunity:master Oct 2, 2018
@greens greens deleted the WWST-1595 branch October 2, 2018 17:40
antonbo pushed a commit to antonbo/SmartThingsPublic that referenced this pull request Dec 11, 2018
* WWST-1595 Zigbee Accessory Dimmer

Primary intention is to work with the Aurora Wireless Wall Remote
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants