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

RFC - connection aging #292

Merged
merged 7 commits into from
Apr 19, 2021
Merged

Conversation

chas-iot
Copy link
Contributor

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:

  • general approach
  • hooked into the correct places?
  • names of periods?

there's been no issues reported in several months
needs review
- general approach
- hooked into the correct places?
- names of periods?
@chas-iot
Copy link
Contributor Author

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

@mrstegeman
Copy link
Contributor

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

@benfrancis
Copy link
Member

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
Copy link
Contributor Author

chas-iot commented Mar 3, 2021

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
Copy link
Member

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
Copy link
Contributor Author

chas-iot commented Mar 5, 2021

@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 🙂

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
Copy link
Member

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
Copy link
Contributor Author

chas-iot commented Mar 9, 2021

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.

Copy link
Member

@benfrancis benfrancis left a comment

Choose a reason for hiding this comment

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

@tim-hellhake wrote:

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

Of course all properties need to be human readable in the UI, but at the API level they need to be machine readable, which this is not.

I've filed an issue to create a new TimeAgoProperty (or similar) schema, but I agree that given this is turned off by default it's reasonable to come up with a compromise in the meantime. I would prefer it if this compromise was the "2021-03-05 15:08:52" format in the screenshot above, which falls somewhere between machine readable and human readable! Hard coding strings like "more than 2 days ago" in the adapter just feels wrong.

@chas-iot
Copy link
Contributor Author

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

Copy link
Member

@benfrancis benfrancis left a comment

Choose a reason for hiding this comment

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

Thank you :)

@chas-iot
Copy link
Contributor Author

@tim-hellhake @benfrancis is anything further needed?

@tim-hellhake
Copy link
Member

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

@tim-hellhake tim-hellhake merged commit 3d128bd into WebThingsIO:master Apr 19, 2021
tim-hellhake added a commit that referenced this pull request Apr 19, 2021
RFC - connection aging (#292)
Don't prefix tags with 'v'
@chas-iot
Copy link
Contributor Author

Thank-you

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

4 participants