WebThingsIO / zigbee-adapter

Zigbee adapter add-on for WebThings Gateway
Mozilla Public License 2.0
46 stars 29 forks source link

RFC - connection aging #292

Closed chas-iot closed 3 years ago

chas-iot commented 3 years ago

Please review both the use case and the implementation

Use case: I want to see when each Zigbee device was last active so that I can see if it has become disconnected or non-responsive

Implementation questions:

chas-iot commented 3 years ago

Apologies, somehow the eslint integration in vscode was broken for me, leading to the unnecessary errors.

mrstegeman commented 3 years ago

@tim-hellhake @dhylands any issues with this? Seems reasonable to me.

benfrancis commented 3 years ago

Out of interest, does this bubble up to the web thing and add an additional property to the Thing Descriptions of all Zigbee devices which will then appear in the UI?

chas-iot commented 3 years ago

Out of interest, does this bubble up to the web thing and add an additional property to the Thing Descriptions of all Zigbee devices which will then appear in the UI?

yes image

benfrancis commented 3 years ago

Hmm, interesting. I understand the use case and can see why this is useful.

I'm not overly keen on cluttering the UI with an extra property for every Zigbee device. I think it's arguable whether this is really a property of the device, or of the Zigbee controller communicating with the device. Unfortunately there's not currently a good alternative approach available for this that I can think of. In the future I wonder if it might make sense for this to be part of the W3C's proposed Directory API - providing a standard way to express when a device was last seen by a directory/gateway, rather than as a property of the thing itself (which doesn't always make sense, like when a thing is hosting its own Thing Description). You might want to propose it there?

One thing I would definitely change about the implementation is to make the property a timestamp (either an ISO 8601 string or and integer representing milliseconds past the epoch), rather than a set of human readable strings hard coded inside the adapter, for a couple of reasons:

  1. The current strings are not a useful machine readable value for consumers of the Web Thing API (other than the gateway's own web interface)
  2. The strings are not localisable

The downside of changing it to a timestamp is that its default representation in the UI is not going to be very user friendly. We would ideally want to define a schema for a LastSeenProperty or TimeagoProperty which renders the date relative to the current time like "3 hours ago" in your example.

In conclusion, thank you for working on this, I agree it's useful. I don't love adding an extra property but accept it's the best solution currently available so for now I would just suggest changing the format in which the property is provided.

chas-iot commented 3 years ago

@benfrancis thanks for engaging on this.

I'm not overly keen on cluttering the UI with an extra property for every Zigbee device.

It's behind a configuration flag and not enabled by default. image Hrmmm... this description is not correct - I'll update it to something like "experimental: Show when each Zigbee device was last active'.

One thing I would definitely change about the implementation is to make the property a timestamp (either an ISO 8601 string or and integer representing milliseconds past the epoch)

I took a look at ISO 8601 Data elements and interchange formats – Information interchange – Representation of dates and times and noted that it is an interchange format, not immediately suited for easy human consumption. The issue is that the resulting string is way too long to display nicely in the gateway's web interface. In the PSI-SG adapter, I'm addressing this by just taking the date and time portions and inserting a newline. image I am likely the only user of this adapter as it is very Singapore specific :slightly_smiling_face:

Showing the time of the last update in milliseconds past the epoch is pretty meaningless to any but extreme nerds - I would have to look up any value to convert into something meaningful. I'd prefer to show the milliseconds since the last update (updated every 5 seconds, which may easily be altered). If the property is increasing, then the device has not been seen. If the property goes to a relatively small number (i.e. less than or equal 5000) then the device was just active on the Zigbee network.

We would ideally want to define a schema for a LastSeenProperty or TimeagoProperty which renders the date relative to the current time like "3 hours ago" in your example.

Another possibility (but very long-term approach) would be to add an extensible library of formatter functions that could take the computer interchange formats and be able to convert to something for human consumption. But that would involve several use-case questions, which would also intersect with the implementation effort. For example: configure in each adapter (which pushes the maintenance burden onto the adapter developer and effectively puts the display in the control of the owner of the installation) or to be able to click on each property in the gateway web interface and choose an appropriate formatter (lots of upfront effort by the core developers and maintainers).

tim-hellhake commented 3 years ago

I can't test it, but the code looks fine.

The current strings are not a useful machine readable value for consumers of the Web Thing API (other than the gateway's own web interface)

I think the whole point of the property is that it's human-readable.

I agree that a standardized time format combined with proper formatting in the gateway would be way better, but this seems to be just an experimental debug property. I would propose to merge the current version and improve it as soon as there is support for a proper capability in the gateway.

chas-iot commented 3 years ago

I played with changing to milliseconds since last seen, which was terrible to use. Tried seconds, which was slightly more usable but not good. So I have gone back to my first implementation with just an updated configuration description.

chas-iot commented 3 years ago

I've changed to show the time of the last device contact in the lightly modified ISO format, as requested.

chas-iot commented 3 years ago

@tim-hellhake @benfrancis is anything further needed?

tim-hellhake commented 3 years ago

@chas-iot Sorry i missed the notification for the latest changes

chas-iot commented 3 years ago

Thank-you