homebridge / HAP-NodeJS

Node.js implementation of the HomeKit Accessory Protocol (HAP)
Apache License 2.0
2.69k stars 630 forks source link

Transitioning node-persist to node-persist 3.0 #629

Closed jvmahon closed 11 months ago

jvmahon commented 5 years ago

This is a follow-up to an off-topic discussion originally started in https://github.com/KhaosT/HAP-NodeJS/issues/586 concerning transitioning node-persist from the legacy version to a more modern version. The issue with doing so as raised there was that node-persist provides no simple transitioning tools to move the database from the legacy data storage to updated version.

I am working on this type of data transitioning for another project and so I'm posting the basic technique here in case it is ever decided to transition HAP-nodeJS to an updated version.

In my case, I decided the "best" way to transition the data was to be able to run both the old (0.0.12) and new 3.0 node-persist side-by-side and run the data through a simple loop reading the old data using node-persist 0.0.12 and writing to a new database using node-persist 3.0. Another node-persist user gave me some advice on how to code this. See https://github.com/simonlast/node-persist/issues/87

This technique requires that you have node persist 0.0.12 and 3.0 loaded at the same time. To enable this, I published 0.0.12 under a new npm package name node-persist-legacy. See https://github.com/jvmahon/node-persist-legacy. Then its just a matter of looping over the old data and re-writing it using the node-persist 3.0 calls.

Some further details on how the updated code would work . . .

In order to transition data, I decided that what I'd do is to store the updated persist 3.0 database in a folder "Persist3" which is a subfolder located in the "normal" persist database folder.

In the "main" body of code, only node persist 3.0 calls are used.

On startup, the program checks if either (1) the "Persist3" folder exists or (2) if neither the "Persist3" folder exists nor any legacy database exists. If either of these is true, then the data was already transitioned or there is no older data to worry about, so the data is already in a state where the persist 3.0 calls in the code are ready to be used.

If, however, there exists "old" node-persist 0.0.12 (or earlier) database, but there isn't a Persist3 folder, then this is the first-run with node-persist 3 and the legacy data needs to be copied to the new database. So, in this case, you run through a simple database copying loop as mentioned above (and further detailed in: https://github.com/jvmahon/node-persist-legacy) and copy the data.

hassankhan commented 5 years ago

Could it be an idea to use the XDG Base Directory Specification? Essentially this would mean new node-persist configuration would live under ~/.config/hap-nodejs rather than in a folder inside the project.

KhaosT commented 5 years ago

@hassankhan technically whomever import hap-nodejs needs to provide the path for us to write the config, and the default fallback for the bundled persist library was the "current" location, so a lot of time when you run it directly, it generates the persist folder in the project itself 😅

If we are really thinking about migration, it might be worth just migrate to a SQLite db so we can get more flexibility down the road...

jvmahon commented 5 years ago

The idea of migrating to SQLite is interesting. On a related issue - one of the things I've been thinking about is the initial start-up speed of iOS Home when interfacing with Hap-NodeJS / Homebridge. I have over a hundred devices and, at times, there are long "updating" delays in iOS when starting Home. I was wondering if there is a lot of delay occurring due to synchronous operations (e.g., in their interactions with the iPhone or Apple TV) which might be lowered if there was more asynchronous processing; That's a reason for considering the updated node-persists it seems to favor an async model (as well as considering SQLite) - but I'm wondering if moving to more asynchronous transactions generally is what's needed. I.e., is it time to raise the level of the required nodeJS to say the 10 or 12 level so contributors could consider moving more functions to an asynchronous model? Not sure if this makes sense, but am interested in your thoughts.

NorthernMan54 commented 5 years ago

From my experience the delays with “updating” are caused by plugins pulling status from devices in real time versus utilizing the status cache in HAP-NodeJS and having plugins and devices push their status to HAP-NodeJS and populate the cache on state changes. My design principal for plugins is that they should never implement the ‘get’ event, and should use the updateValue method for providing current status on state changes. Please see pull request #539 for details.

Also, when you open the Home app, HomeKit sends a request for status of all items on the page, and HAP-NodeJS waits for all the ‘get’ events to return before returning the results to HomeKit. As the request is sent to each HAP-NodeJS instance separately, you can improve the “updating” performance by putting all the quick responders in one instance, and the slow ones in a separate instance. This should change the home app to appear more responsive.

On Aug 24, 2019, at 6:43 AM, jvmahon notifications@github.com wrote:

The idea of migrating to SQLite is interesting. On a related issue - one of the things I've been thinking about is the initial start-up speed of iOS Home when interfacing with Hap-NodeJS / Homebridge. I have over a hundred devices and, at times, there are long "updating" delays in iOS when starting Home. I was wondering if there is a lot of delay occurring due to synchronous operations (e.g., in their interactions with the iPhone or Apple TV) which might be lowered if there was more asynchronous processing; That's a reason for considering the updated node-persists it seems to favor an async model (as well as considering SQLite) - but I'm wondering if moving to more asynchronous transactions generally is what's needed. I.e., is it time to raise the level of the required nodeJS to say the 10 or 12 level so contributors could consider moving more functions to an asynchronous model? Not sure if this makes sense, but am interested in your thoughts.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/KhaosT/HAP-NodeJS/issues/629?email_source=notifications&email_token=AEXEFGFR4EFZTXFDORQWM6DQGEGD5A5CNFSM4GJMZI6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5B5UXA#issuecomment-524540508, or mute the thread https://github.com/notifications/unsubscribe-auth/AEXEFGAKILRDE6VVU257HQTQGEGD5ANCNFSM4GJMZI6A.

jvmahon commented 5 years ago

As the request is sent to each HAP-NodeJS instance separately, you can improve the “updating” performance by putting all the quick responders in one instance, and the slow ones in a separate instance. This should change the home app to appear more responsive.

Yes, that's what I've done using HomeBridge - dividing different HomeBridge plugins into different instances helps a lot. This seems to support the thought that if the programming model itself was able to do more asynchronously, it would also improve performance but without requiring users to figure out how to divide devices into different executing instances. I don't know if this is something that could be done reasonably easily with HAP-NodeJS, but was interested in hearing the author's (KHaosT's) and other's thoughts on it.

NorthernMan54 commented 5 years ago

One change that could be made to HAP-NodeJS is to change the interface to devices, and deprecate the GET event interface, and force all plugins to use updateValue interface to return status.

In regards to an asynchronous programming model, please keep in mind that the NodeJS and JavaScript programming is asynchronous for all external calls. And requests are passed as events to each characteristic in parallel. And the response to HomeKit waits for all to respond. This is the root of the ‘Updating’ issue.

If the characteristic does not implement the get event interface, the response to HomeKit is immediate based on the cache.

Would it make sense to score each plugin based on implementation, and usage of the get event interface. Gold would be ones that push status updates via updateValue in real time, silver would be ones that poll devices regularly and use updateValue, bronze would be ones that use the GET event to return device status.

On Aug 24, 2019, at 10:42 AM, jvmahon notifications@github.com wrote:

As the request is sent to each HAP-NodeJS instance separately, you can improve the “updating” performance by putting all the quick responders in one instance, and the slow ones in a separate instance. This should change the home app to appear more responsive.

Yes, that's what I've done using HomeBridge - dividing different HomeBridge plugins into different instances helps a lot. This seems to support the thought that if the programming model itself was able to do more asynchronously, it would also improve performance but without requiring users to figure out how to divide devices into different executing instances. I don't know if this is something that could be done reasonably easily with HAP-NodeJS, but was interested in hearing the author's (KHaosT's) and other's thoughts on it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

ebaauw commented 5 years ago

deprecate the GET event interface, and force all plugins to use updateValue

Please don’t - this will break Eve history.

Also in homebridge-zp, I actually use this deliberately for the LED and button lock states. Sonos doesn’t provide notifications for these, and I don’t want to poll the zone players just for these two attributes. The Sonos app also does a just-in-time retrieval before displaying these attributes.

Would it make sense to score each plugin based on implementation, and usage of the get event interface. Gold would be ones that push status updates via updateValue in real time, silver would be ones that poll devices regularly and use updateValue, bronze would be ones that use the GET event to return device status.

I don’t think you can score a plugin. The same plugin might employ multiple schemes. homebridge-zp would be bronze for the LED and button lock state, but gold for everything else. homebridge-hue is silver for the Hue bridge, which doesn’t provide any notifications, but gold for deCONZ, which does.

In itself, GET is not the issue (see the Eve history case), but doing asynchronous I/O before calling callback. Here you should distinguish between guarding the I/O with a timeout (of 1 to 2 seconds) vs an I/O that might take longer.

jdtsmith commented 4 years ago

Just found this fascinating discussion @NorthernMan54 + @ebaauw. To clarify, if I do not provide a characteristic.on('get',... then whenever HomeKit needs a new value to display for that characteristic, it will automatically take one that HAP has cached for it (presumably from some earlier updateValue)? I always thought I had to provide that cached value myself (with simple event callbacks like getStatus(callback) {callback(this.status)}).

Is a sensible low-latency "gold" pattern for plugins then something like:

I can see @ebaauw's concern though: if you have some service reflected in HomeKit that is "informational only" (thinks a switch reporting some internal state of your device), which is not something your plugin will act on if its state changes, there is no sense polling for that information.

Sounds like HAP needs to accept, in addition to actual values, promises to provide an updated value. Then just update them as they resolve.

ebaauw commented 4 years ago

Using promises instead of callbacks won’t change the issue. The HomeKit runtime does a synchronous GET to the HAP server, blocking the HomeKit application until the HAP server has sent a response. Whether that response is reported through a callback function or by resolving a promise doesn’t change the fact that the HomeKit app is blocked. You would want to send a response immediately (by returning the cached value) or guard the asynchronous device I/O with a timeout and send the cached value in case the device I/O takes too long.

NorthernMan54 commented 4 years ago

And if you are using the updateValue/cache approach the other consideration is how to tell the Home app when your device is not available. So there is this feature of the updateValue function

https://github.com/KhaosT/HAP-NodeJS/pull/556

I find the updateValue approach is the most responsive when the device send’s messages indicating that its state has changed rather than having to poll the device.

On Jan 21, 2020, at 1:16 PM, Erik Baauw notifications@github.com wrote:

 Using promises instead of callbacks won’t change the issue. The HomeKit runtime does a synchronous GET to the HAP server, blocking the HomeKit application until the HAP server has sent a response. Whether that response is reported through a callback function or by resolving a promise doesn’t change the fact that the HomeKit app is blocked. You would want to send a response immediately (by returning the cached value) or guard the asynchronous device I/O with a timeout and send the cached value in case the device I/O takes too long.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

jdtsmith commented 4 years ago

Thanks, both. I'm still unclear on the difference between setValue and updateValue. Is it only that updateValue doesn't generate a 'set' event? If so, I can't see why a plugin would ever call setValue on one of its own services, since recursive loops could ensue.... i.e. it seems setValue is best called exclusively by HAP/Homebridge themselves.

And to be clear, is it true I can actually entirely omit a 'get' handler for a service characteristic as long as I am updateValue'ing regularly from external events (or don't mind polling)? The advantage even of intermittent polling is it is unlikely to collide with the Homekit GET from HAP.

BTW, do either of you know a good way to see which plugins are stalling out the HAP response to Homekit when the dreaded "updating" plagues the Home App? Like a Homebridge profiling log level ("XXXXXXX took 3.9s responding to status request for Service YYYY's Characteristic ZZZZ"). That's a better approach I think than GOLD/SILVER/BRONZE or whatever: just a shame board of who's holding up the process the longest and for which services/characteristics. Reminds me of the Battery Usage by App screen on the iPhone. Users would naturally filter this info back to plug-in developers...

jdtsmith commented 4 years ago

Using promises instead of callbacks won’t change the issue. The HomeKit runtime does a synchronous GET to the HAP server, blocking the HomeKit application until the HAP server has sent a response.

I see. An obvious solution is just to cutoff waiting for all callbacks after some user-configurable delay, falling back to the old cached values. Some things might not be correct, but at least it wouldn't block the whole system...

jvmahon commented 4 years ago

Thanks, both. I'm still unclear on the difference between setValue and updateValue. Is it only that updateValue doesn't generate a 'set' event? If so, I can't see why a plugin would ever call setValue on one of its own services, since recursive loops could ensue.... i.e. it seems setValue is best called exclusively by HAP/Homebridge themselves.

And to be clear, is it true I can actually entirely omit a 'get' handler for a service characteristic as long as I am updateValue'ing regularly from external events (or don't mind polling)? The advantage even of intermittent polling is it is unlikely to collide with the Homekit GET from HAP.

BTW, do either of you know a good way to see which plugins are stalling out the HAP response to Homekit when the dreaded "updating" plagues the Home App? Like a Homebridge profiling log level ("XXXXXXX took 3.9s responding to status request for Service YYYY's Characteristic ZZZZ"). That's a better approach I think than GOLD/SILVER/BRONZE or whatever: just a shame board of who's holding up the process the longest and for which services/characteristics. Reminds me of the Battery Usage by App screen on the iPhone. Users would naturally filter this info back to plug-in developers...

I'm not 100% sure of this (this is from memory), but I think a difference between setValue and updateValue is that if you have a .on ('change', function()... ) set up, and you call .setValue, that will then cause a change to emit and your .on ('change', function()... ) will trigger. If you use .updateValue the .on ('change', function()... ) does not trigger.

Supereg commented 4 years ago

Both of them trigger a change event (given the value actually changed). Only setValue triggers the set event and is the method use by hap-nodejs itself to set the value coming from a HAP request (and it really doesn't make sense that plugins call this function).

jvmahon commented 4 years ago

hem t

That sounds right. Thanks for the correction.

NorthernMan54 commented 4 years ago

@jdtsmith @jvmahon Back on your plugin scoring comment, I think we should deprecate the 'get' event with the next major upgrade. It would force everyone to improve their plugin device status logic, and get rid of the "updating" issue permanently.

Am thinking that the update to the next version of HAP-NodeJS is likely to require rework of everyone's plugin, so it would be a good time to make the move.

ebaauw commented 4 years ago

I think we should deprecate the 'get' event with the next major upgrade.

Please don’t, as this will break the fakegato history. The issue isn’t the get event, the issue is doing I/O in the event handler before returning control.

jdtsmith commented 4 years ago

I think we should deprecate the 'get' event with the next major upgrade.

Please don’t, as this will break the fakegato history. The issue isn’t the get event, the issue is doing I/O in the event handler before returning control.

If you are not producing a status value through just-in-time I/O or network access, why could you not updateValue?

Supereg commented 4 years ago

I think we should deprecate the 'get' event with the next major upgrade.

This would render any control point characteristics unusable (and would probably be problematic for any other characteristic, which do not support events).

NorthernMan54 commented 4 years ago

Fair point everyone

On Jan 22, 2020, at 8:24 AM, Andi notifications@github.com wrote:

 I think we should deprecate the 'get' event with the next major upgrade.

This would render any control point characteristics unusable.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.