SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
268 stars 452 forks source link

fp2: use old lunchbox/rest.lua #1711

Closed hdlee27 closed 1 week ago

hdlee27 commented 1 week ago

Check all that apply

Type of Change

Checklist

Description of Change

Summary of Completed Tests

github-actions[bot] commented 1 week ago

Channel deleted.

github-actions[bot] commented 1 week ago

Test Results

   64 files  ±0    400 suites  ±0   0s :stopwatch: ±0s 1 989 tests ±0  1 989 :white_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  3 429 runs  ±0  3 429 :white_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit a61b5fa1. ± Comparison against base commit 339ccb0e.

github-actions[bot] commented 1 week ago

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against a61b5fa1b77a57b494ede58479d12641d5f21ba4

ldeora commented 1 week ago

This one finally works for me. Doing some more testing (deleting and adding several times in a row, alternating between this and the current beta version) later today (may take hours).

Instructed some affected users to test this driver with their environment - waiting for feedback.

ldeora commented 1 week ago

Food for thought: What would happen if the current production driver suddenly worked after the driver from this PR worked, which I will test later. That would mean that the cache and/or the fields would survive a delete/add. I don't know how persistent the fields are stored. The fields contain everything important for the adding to work.

That would explain why some users don't have any issues at all. Users with the installed production version of the drivers who tried to add the device to ST the very first time. Not like those users who tried it before with the very first version from the PR!

Just thinking out loud...

ldeora commented 1 week ago

Sooo...

I tested it now back and forth:

Installed driver from this PR: works Installed driver from main, production, beta: doesn't work Installed driver from this PR again: works Installed driver from main, production, beta: doesn't work Installed driver from this PR again: works

Between driver changes, the exact same procedure: delete/add device in Aqara Home app, scan in ST.

This is the proof that the lunchbox update broke the driver!

Screenshot_20241022_183115_Samsung Internet

ldeora commented 1 week ago

Someone without this issue must test this before it gets merged. @seojune79 for example said, that every version works for him, so he should also test this version.

Then it should go into production asap, before users are giving up.

dljsjr commented 1 week ago

Thanks for the fix @hdlee27

And thanks for the testing @ldeora

Wanted to let you know that we reviewed the Hue and Sonos code that uses a vendored version of this library as well, and these changes were not made in those drivers. Only the Aqara driver. Just wanted to do our diligence since we suspect there's an issue here. We'll be expediting this to production.

ldeora commented 1 week ago

That's good to know! Thanks for the information.