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

updateCharacteristic value type needs to be updated to add support for errors #974

Closed
hbblebc opened this issue Sep 25, 2022 · 29 comments
Closed

Comments

@hbblebc
Copy link

hbblebc commented Sep 25, 2022

Analysis

updateCharacteristic only supports a CharacteristicValue for its value input, even though the underlying updateValue function called inside updateCharacteristic takes a CharacteristicValue, Error, or HapStatusError.

Expected Behavior

Updating the TS will get rid of errors and allow me to pass through an Error when my device is not responding.

Steps To Reproduce

...getService().updateCharacteristic(this.Characteristic.Brightness, new Error('Polling data out of sync'));

Logs

NA

Configuration

NA

Environment

  • OS: Ubuntu 22.04.1
  • Software: Homebridge 1.5.0
  • Node: 16.17.1
  • npm: 8.15.0

Process Supervisor

hb-service

Additional Context

No response

@hbblebc hbblebc added the bug label Sep 25, 2022
@hbblebc
Copy link
Author

hbblebc commented Sep 25, 2022

Additionally, I don't see any way to send an Error through characteristic.onGet(). If the device is not responding and the user attempts to query a characteristic of the device, what is the appropriate way to respond to that? The docs made me thing I just return an Error but maybe I am missing something.

@hbblebc
Copy link
Author

hbblebc commented Sep 25, 2022

I updated my onGet functions to throw a HAPStatus value instead of returning an Error, it looks like that is the better solution for that one, from what I can see in the code.

@Supereg
Copy link
Member

Supereg commented Sep 25, 2022

Throwing inside the onGet is the expected way to return an error. Ideally you should throw an HapStatusError (with a service communication failure) to mark expected errors.

@Supereg Supereg added enhancement and removed bug labels Sep 25, 2022
@adriancable
Copy link
Contributor

@Supereg - is this a useful way to handle errors though? (Is there a useful way to handle errors?) This will make all devices on the bridge go to 'No Response', and potentially stick there for a long while, longer than the error state persists.

I know you know this (but for others that don't) HomeKit doesn't provide any usable way for accessories to signal a transient 'unreachable' state. Such a thing did exist a while ago (prior to iOS 11) but Apple has removed it completely.

A while back I got some insight into this from someone 'in the know' - TL;DR, from a UX perspective HomeKit accessories should always work. Therefore, instead of Apple providing a means for an accessory to signal an error state, the accessory vendor should fix their product so an error state never occurs. This is one of many examples of Apple's well known 'nudge' approach to push developers into making better design decisions. I think it is a very good approach.

@hbblebc
Copy link
Author

hbblebc commented Sep 25, 2022

That seems like an idealized pipe dream. No matter how well the device works, it’s impossible to guarantee 100% uptime. That is odd for Apple to remove the system in place to handle that.

I guess if my device is offline, it doesn’t have the capability to tell the bridge it is offline.

In my specific case, the device has a privacy mode that when enabled, will make one of the sensors stop updating, so I set the sensor to be offline instead. It might be better to just set the sensor state to “not detected” instead though, so nothing gets triggered.

@adriancable
Copy link
Contributor

@hbblebc - the problem Apple is trying to solve is one of moral hazard. A vendor with product bugs that result in material downtime should fix those bugs. Apple should not be legitimizing downtime by giving product developers a way of signalling it to HomeKit.

This is especially true for a smart home product because, for example, downtime on a lock means you can't get into your house. Without downtime close to 0, the smart home will never really be viable, because most equivalent non-smart home devices (e.g. switches, locks etc.) have effectively 0 downtime. And also, with good engineering, you really can get close to 100% uptime for smart products that don't rely on external dependencies (e.g. a cloud service), which most smart home products do not. So, Apple is not asking the impossible. (We are a device vendor, our products have effectively 0 downtime, and we are nothing special, just careful.)

In your case, the correct action is to set the sensor state to 'not detected' when it does not, or cannot, detect motion.

@hbblebc
Copy link
Author

hbblebc commented Sep 25, 2022

That makes sense, I will update my package accordingly.

@adriancable
Copy link
Contributor

@hbblebc - one additional note that just came to mind. For your use case, where the status of a characteristic cannot update because of a user action (privacy mode in your case), as opposed to downtime, the right thing is to set Characteristic.StatusActive to false. Almost all HomeKit services support this as an optional characteristic, and it's displayed in the Apple Home app UI on the accessory's settings page.

@hbblebc
Copy link
Author

hbblebc commented Sep 25, 2022

Interesting, I will look into it. How does HomeKit treat a device with that set false?

@adriancable
Copy link
Contributor

HomeKit doesn’t treat it any differently, but the Apple Home app will show ‘Status Active - No’ in the accessory’s settings, so the user will know the device status may not be up to date.

@Supereg
Copy link
Member

Supereg commented Sep 26, 2022

@Supereg - is this a useful way to handle errors though? (Is there a useful way to handle errors?) This will make all devices on the bridge go to 'No Response', and potentially stick there for a long while, longer than the error state persists.

@adriancable You are perfectly right. I totally agree with the points you made.

Would you be down to create a wiki page with the points you illustrated here? Would love to capture this a central page to which we can link to from documentation! 🚀

@adriancable
Copy link
Contributor

@Supereg - time permitting, sure. If you can create a 'blank' Wiki page with the right structure and email me the link, I'd be happy to fill it in.

@Supereg
Copy link
Member

Supereg commented Sep 28, 2022

@adriancable
Copy link
Contributor

@Supereg - thanks, this is great. I have added some 'meat' to it.

@NorthernMan54
Copy link
Contributor

@adriancable Maybe I'm not understanding the issue or use case correctly, but the comment about all devices on a bridge being marked as 'Not Responding' when setting a device with an error state doesn't feel correct.

In my homebridge plugins, I follow this pattern to manage device data and availability - https://developers.homebridge.io/#/api/service#serviceupdatecharacteristic and I just did a quick retest with IOS16 and the behaviour has not changed. I can mark a single accessory as 'not responding' on a bridge.

IMG_8705

This was just a quick test with one of my Tasmota devices using the homebridge-tasmota plugin, and all the other devices in the screenshot are also Tasmota based.

@adriancable
Copy link
Contributor

adriancable commented Sep 28, 2022

@NorthernMan54 - thanks for your input! I have had a play with iOS 16 - a couple of observations. First you are right that 'No Response' on one accessory does not affect other devices on the bridge. I believe this is a change from prior versions of iOS, but unfortunately since all my devices are on iOS 16, I cannot test.

The critical issue here is that, after setting the 'No Response' state, contrary to the Homebridge documentation, you cannot generally revive the accessory at will by updating the characteristic to a non-error value. Specifically, once the Apple Home app sees an accessory go to 'No Response' it then ignores / does not check for any further updates for that accessory. You can verify this for yourself: after setting your Portable Fan to 'No Response', stay on that same screen on the Apple Home app and try to 'revive' the accessory by setting a valid value for the characteristic. You will see that the accessory stays on 'No Response', I believe indefinitely, or at least for a very long time after the error condition stops.

You can force it to re-check by leaving the Apple Home app, waiting a few seconds, and then going back. You will then see that the accessory 'revives'.

But because of this behaviour whereby accessories stick on 'No Response' indefinitely, this approach leads to a poor/confusing UX if you are trying to signal transient error conditions with an accessory.

@NorthernMan54
Copy link
Contributor

@adriancable The behaviour of only a single accessory on a bridge has been this way for a few versions, as I had written the Tasmota plugin quite a few years ago.

The critical issue here is that, after setting the 'No Response' state, contrary to the Homebridge documentation, you cannot generally revive the accessory at will by updating the characteristic to a non-error value. Specifically, once the Apple Home app sees an accessory go to 'No Response' it then ignores / does not check for any further updates for that accessory. You can verify this for yourself: after setting your Portable Fan to 'No Response', stay on that same screen on the Apple Home app and try to 'revive' the accessory by setting a valid value for the characteristic. You will see that the accessory stays on 'No Response', I believe indefinitely, or at least for a very long time after the error condition stops.

I'm 100% agreed that this behaviour of the home app in this use case is not the best for an end user. But personally I prefer the current behaviour versus not having any easily visible feedback that a device may not be working/responding. Realize that in use cases where the end user is not restarting the home app or changing rooms, the 'not responding' message could go stale ie iPad Dashboards

Do you have any visibility into how any vendor supplied bridges handle the same scenario ? Am thinking the Hue Hub may be a good candidate to explore the 'not responding' experience

@adriancable
Copy link
Contributor

@NorthernMan54 - I have a Philips Hue hub, although the bulbs are not very accessible. I will try, when I can, to unscrew one of the bulbs and see what happens.

I suspect in this case the bulb will go to 'No Response' (and stick there) and this is probably OK because, if the bulb is physically disconnected, it is obvious to the user that there is a 'physical' issue. What I don't think is right is when other conditions, which are not visible to the user, cause unexpected 'No Response' in the Apple Home UI. I'm not familiar with the Tasmota plug-in, unfortunately so I don't know under what circumstances 'No Response' can occur and whether that would be expected by the user or not.

@NorthernMan54
Copy link
Contributor

The No response in my Tasmota plugin was coded against device availability and only triggers when a device is reported unavailable. Which would be a similar experience to a hue bulb being removed

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 31, 2022
@Supereg Supereg removed the stale label Oct 31, 2022
@hbblebc
Copy link
Author

hbblebc commented Nov 1, 2022

Discussion aside, I think the original issue is still outstanding and can use correction. In the rare situation you do want to pass an error to updateCharacteristic, the typing won’t allow it.

@Supereg
Copy link
Member

Supereg commented Nov 1, 2022

I'm sorry that the issue got closed due to the discussion being stale. The issue is still on track to be included in the upcoming v0.11.0 release (see https://github.com/homebridge/HAP-NodeJS/projects/7#card-85843620)

@hbblebc
Copy link
Author

hbblebc commented Nov 1, 2022

Thanks for the update. I’m never a fan of auto-stale bots. Better to manually determine if the ticket needs to be addressed or not.

@Supereg
Copy link
Member

Supereg commented Nov 1, 2022

Just opened a PR to fix this: #981. Would be happy for a review from your side 😃 🚀

@Supereg
Copy link
Member

Supereg commented Nov 1, 2022

This issue was addressed in current beta release 0.11.0-beta.10

@Supereg Supereg closed this as completed Nov 1, 2022
@hbblebc
Copy link
Author

hbblebc commented Nov 1, 2022

Looks good to me. Thanks for getting that taken care of.

@mandy-0810
Copy link

Hi, Currently i'm using lightService.getCharacteristic to get the status of the device and lightService.setCharacteristic to control the device from Home app, But when the device is triggered manually using a physical switch, the state of switch is not updating in the Home app. When i control the device manually using physical switch i receive a device update status from my socket-io.client, but i'm not sure how to update the state on the Home app. Can someone suggest me the best way to make this work.

@NorthernMan54
Copy link
Contributor

The api is well documented on the homebridge side

https://developers.homebridge.io/#/api/service

@Supereg
Copy link
Member

Supereg commented Dec 15, 2022

Here are the respective parts from the hap-nodejs technical documentation.

Either Characteristic.updateValue(…)
https://developers.homebridge.io/HAP-NodeJS/classes/Characteristic.html#updateValue

Or Service.updateCharacteristic(…) which is basically the same
https://developers.homebridge.io/HAP-NodeJS/classes/Service.html#updateCharacteristic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants