anaxyd / homey-airthings

GNU General Public License v3.0
4 stars 10 forks source link

Luminance support and general connection improvements #7

Open straficchio opened 4 years ago

straficchio commented 4 years ago

Hi,

I noticed that over long periods of time (days) the reads from wave devices tend to stop. When this happens, restarting the homey seems to be the only fix. I traced this back to an apparent condition where a BLE failure would not be correctly handled. Also, to reduce the interaction time I added synchronization to avoid too many connections to be open at the same time. This seems to improve connection reliability especially when other homey applications that use BLE are loaded (Beacon for example). Also I added decoding of the luminosity values on both wave and wave+. There is also a little bit of refactoring so that device type specific code is all in one unit and the rest is as general as possible.

Hope that at least some of these changes will turn out useful.

Cheers, Marco.

oletorv commented 4 years ago

Hi Marco !

Nice to see that you have some free time to help out :D

Good, I will look at it ASAP. The interesting part is that it seems to be some kind of scanning issue in the start of the Homey, for about 30 minutes on my here, where it does not find anything on BLE, is that the same on yours ?

The BLE failures, are they when the device is found and connected, but service discovery does not work ?

O.

straficchio commented 4 years ago

Hi Ole!

First off I think this has to do in general with improper synchronization and resource allocation/release in the Homey itself because I have seen instances of BLE becoming unresponsive and at the same time ZigBee dropping out completely. Also there may be differences between old homeys (like mine) and newer ones.

The scanning issue that you are mentioning happened to me too but typically only after the BLE became unstable from before. So for example devices would get stuck, I would first try to restart the airthings plugin without joy, then I would try to remove the devices and attempt to read them only to find out that they are not found anymore. At that point only a homey reboot would restore function.

By the way, for the purpose of my testing, I loaned a few waves from friends and I'm now running 7 units (1 mini, 2 wave first gen, 4 wave plus) with a tight polling rate (10 minutes). Before I would get a hang in a matter of 24-48 hours, and now I haven't seen a hang anymore. So at least did something :-)

The failures happen after connection but at pretty much any point afterwards. I have tried to avoid reenumeration of services and characteristics by holding those objects as properties of the device object but I could not find the minimum amount of data to store that would survive across reconnection. The one thing that works instead is to save the peripheral object and do an assertConnected on it instead of scanning explicitly. That saves time and reduces the stress on the BLE stack I think.

[edit] I almost forgot: this code is supposed to add the luminance capability on all the preexisting devices. It happened to me once that a couple of devices ended up without the luminance being correctly shown after the upgrade but from the logs everything seemed fine. Restarting just the airthings plugin fixed it.

Bye!

On 2/13/2020 12:37:42 AM, "Ole Andreas Torvmark" notifications@github.com wrote:

Hi Marco !

Nice to see that you have some free time to help out :D

Good, I will look at it ASAP. The interesting part is that it seems to be some kind of scanning issue in the start of the Homey, for about 30 minutes on my here, where it does not find anything on BLE, is that the same on yours ?

The BLE failures, are they when the device is found and connected, but service discovery does not work ?

O.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/anaxyd/homey-airthings/pull/7?email_source=notifications&email_token=ANP4RHWDUV3NR62OBOPV7LLRCSB4NA5CNFSM4KUF2MF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELSZZ3A#issuecomment-585473260, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANP4RHUF42CQIZLXTFKDFW3RCSB4NANCNFSM4KUF2MFQ.

rindlerblabla commented 4 years ago

See also https://github.com/anaxyd/homey-airthings/issues/4. My experience (have one wave plus and two minis) is that the connection get lost every time the Homey is restarted but that a search for new devices always make the devices reconnect.

anaxyd commented 4 years ago

@straficchio Thank you for you contribution! 👍 👍 Thats great to have luminance supported as a capability, I did not know that this existed. Also the syncronization issue is great to have a fix for.

I have been developing my own MiFlora app for Homey lately, because I find the official one to be lacking stability. This is also a BLE app, which also has issues with the Bluetooth stack in Homey, which causes the devices to not sync after a random amount of time. I have around 10 MiFlora sensors which will get synced, and have been researching the optimal way of doing this.

By sequencing a async/await call inside a for..of loop seems to do the trick. It will wait until the syncronization of device 1 is done before proceeding to device 2 in a great way, by using vanilla async Javascript instead of a library. I believe that is what the Semaphore library you have added does?

I also think that the assertConnected() is a major component in fixing the BLE Timeout issue, I have been reading about that function myself, but not tried it yet. I think I will try this in my MiFlora app as well. Great work!

I think we can use a lot of your code in a next release, but I don't think we can merge all of it because I want to use as little callback-based code as possible. I suggest that we pick most of your code into a new release, and optimize the syncronization code to be based on async/await in a sequence. Will of course credit you in this release. What do you think?

straficchio commented 4 years ago

Hi!

The purpose of the semaphore is indeed to serialize ble access. Since the device objects are called all asynchronously I wouldn’t know how to achieve the same functionality with a for loop but I trust it’s possible if you have done so already. One advantage of the semaphore, admittedly vanishingly small, is that one can decide how many connections to run at the same time so that one slow connection doesn’t hang the process. Not a big deal if one samples at slow rate.

As for the rest of the changes, some were just to avoid code duplication and to put as much as possible of the binary parsing in one place. Functionally is the same as before.

The key changes are, as you said, the assertConnect and the addition of a disconnect in the catch construct.

At any rate, please cherry pick as you see fit. No problem with that. Just happy to help out!

Bye!

On 14 Feb 2020, at 20:57, anaxyd notifications@github.com wrote:

 @straficchio Thank you for you contribution! 👍 👍 Thats great to have luminance supported as a capability, I did not know that this existed. Also the syncronization issue is great to have a fix for.

I have been developing my own MiFlora app for Homey lately, because I find the official one to be lacking stability. This is also a BLE app, which also has issues with the Bluetooth stack in Homey, which causes the devices to not sync after a random amount of time. I have around 10 MiFlora sensors which will get synced, and have been researching the optimal way of doing this.

By sequencing a async/await call inside a for..of loop seems to do the trick. It will wait until the syncronization of device 1 is done before proceeding to device 2 in a great way, by using vanilla async Javascript instead of a library. I believe that is what the Semaphore library you have added does?

I also think that the assertConnected() is a major component in fixing the BLE Timeout issue, I have been reading about that function myself, but not tried it yet. I think I will try this in my MiFlora app as well. Great work!

I think we can use a lot of your code in a next release, but I don't think we can merge all of it because I want to use as little callback-based code as possible. I suggest that we pick most of your code into a new release, and optimize the syncronization code to be based on async/await in a sequence. Will of course credit you in this release. What do you think?

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