Harman Luxury Change from REST HTTP API to WebSckets #1433

alon-tchelet · 2024-06-11T14:13:54Z

This PR is fixing pretty much all the issues previously experienced with the driver as the vast majority of issues came from random timeouts and the lack of lua sockets usage.

By moving all the communication between the device and the hub to websockets we now have a stable bidirectional connection based on lua sockets. The old inefficient and blocking REST API polling was removed and the device is now able to give live updates without being polled by the hub. This results in a much more relaxed and quieter network activity by the device and hub and a smoother user experience.

Note: This PR is replacing previous improvement attempt in now closed PR #1360

github-actions · 2024-06-11T14:15:21Z

Invitation URL:
https://bestow-regional.api.smartthings.com/invite/OzMg0o1AgD29

github-actions · 2024-06-11T14:15:46Z

Test Results

60 files 377 suites 0s ⏱️
1 821 tests 1 821 ✅ 0 💤 0 ❌
3 159 runs 3 159 ✅ 0 💤 0 ❌

Results for commit a3dc9a6.

♻️ This comment has been updated with latest results.

github-actions · 2024-06-11T14:15:48Z

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against a3dc9a6

cjswedes

The usage of websockets is really good for this driver, and I think it is worthwhile to pursue it. Unfortunately the lustre library used here is out of date; I assume you copied from bose which is also out of date and we intend to update some day. You should instead use the version that is made available in the lua libraries so that you are using the most recent version which has several bug fixes, and so you dont have to include any of the lustre files in the driver package. If you want an example of using the newer api, which isn't callback based, sonos is really the only example (sonos packages its own lustre but it is almost identical to what is in the lua libs with only a small change to the constructor api iirc).

Sonos has a Router which is a single cosock task that manages all sending and receiving for all the websockets used in the driver. You could follow that approach or another option is to have a single task for managing each websocket individually.

drivers/SmartThings/harman-luxury/src/api/apis.lua

drivers/SmartThings/harman-luxury/src/api/nsdk.lua

dljsjr · 2024-06-13T15:21:29Z

I would also advocate for using the cosock/select based lustre instead of the callback based one. As Carter mentioned, it's included in the Lua Libs, so you'd no longer need to vendor it, but you will need to adjust the API usage.

(sonos packages its own lustre but it is almost identical to what is in the lua libs with only a small change to the constructor api iirc)

As Carter mentioned, the patch to lustre in the Sonos driver is a minor tweak to the constructors. For Sonos, we need to be able to pass in our own TCP socket instead of having lustre create one implicitly. This is because the default lustre package doesn't work with wss:// out of the box, but the only thing needed for wss:// is passing in a TLS-wrapped lua socket. So the Sonos version of lustre modifies the constructor to allow for passing in our own socket. It's on our roadmap to upstream this patch so that it's available to all users, but there are some additional refactors needed to make the changes more idiomatic and ergonomic.

If that's too large of a refactor for your timelines on this PR, it's not a blocker. But it is something I would recommend you keep on your radar, in the interest of keeping the code size and memory usage for the driver at a minimum by using as much shared code as possible. It also makes sure that you don't have to back port bug fixes yourself if we improve the code in the lua libs.

dljsjr · 2024-06-13T15:26:57Z

drivers/SmartThings/harman-luxury/src/disco.lua

@@ -41,20 +32,12 @@ function Discovery.set_device_field(driver, device)
  local device_cache_value = driver.datastore.discovery_cache[device.device_network_id]

  -- persistent fields
-  device:set_field(const.STATUS, true, {
-    persist = true,
-  })
  device:set_field(const.IP, device_cache_value.ip, {


Does this ever get updated if the IP address of the speaker changes due to DHCP?

Good point. I forgot taking this into account when changing to WebSockets. I'll add some logic to handle this scenario as I switch to the lua library's lustre library

dljsjr · 2024-06-13T15:32:48Z

drivers/SmartThings/harman-luxury/src/hl_websocket.lua

+local HLWebsocket = {}
+HLWebsocket.__index = HLWebsocket
+
+--- hendles capabilities and sends the commands to the device


typo: hendles -> handles

alon-tchelet · 2024-06-14T12:37:07Z

The usage of websockets is really good for this driver, and I think it is worthwhile to pursue it. Unfortunately the lustre library used here is out of date; I assume you copied from bose which is also out of date and we intend to update some day. You should instead use the version that is made available in the lua libraries so that you are using the most recent version which has several bug fixes, and so you dont have to include any of the lustre files in the driver package. If you want an example of using the newer api, which isn't callback based, sonos is really the only example (sonos packages its own lustre but it is almost identical to what is in the lua libs with only a small change to the constructor api iirc).

I have indeed copied the Lutsre library from Bose, as I wrote in the respective commit message. But I'll take your advice and use the version from the latest library (lib v9_52x)

Sonos has a Router which is a single cosock task that manages all sending and receiving for all the websockets used in the driver. You could follow that approach or another option is to have a single task for managing each websocket individually.

Thanks for the directions

alon-tchelet · 2024-06-19T17:09:48Z

Hi @dljsjr and @cjswedes ,
I pushed the reworked version. Could you have a look?
I do have an issue with the keep_alive config of the websocket. I obviously need it to inspect if the device is still online, but it seems that when I provide a number value I get this error:

FATAL Harman Luxury  runtime error: [string "cosock.lua"]:313: [string "lustre/ws.lua"]:299: attempt to index a nil value (local 'recv')
stack traceback:
	[string "lustre/ws.lua"]:299: in method '_handle_recvs'
	[string "lustre/ws.lua"]:286: in method '_receive_loop'
	[string "lustre/ws.lua"]:178: in function <[string "lustre/ws.lua"]:177>
stack traceback:
	[C]: in function 'error'
	[string "cosock.lua"]:313: in field 'run'
	[string "st/driver.lua"]:1016: in method 'run'
	[string "init.lua"]:207: in main chunk

I don't understand what I'm missing that causes it to try to access a nil value.

PS, I will clean up the commit messages when the changes are accepted

dljsjr · 2024-06-25T14:36:13Z

drivers/SmartThings/harman-luxury/src/lustre/ws.lua

+      if self:_handle_select_err(loop_state, err) then
+        return
+      end
+    elseif self:_handle_recvs(loop_state, recv, 1) then


I'm the person that was helping you out on the forums :)

You can fix this issue by only including this file while preserving the directory structure. You don't need to include the entire library.

Okay. I'll modify it

dljsjr

Approved pending the change that @alon-tchelet is already making 🎉

alon-tchelet · 2024-06-25T15:52:11Z

Cleaned the commits. I will update when the self verification is complete

cjswedes · 2024-07-01T16:39:06Z

@alon-tchelet we'd like to get this merged by 11am CST 7/2. Can you squash your commits down to remove the fixup commits?

edit: we will squash and merge then if you haven't.

Harman decision applied on device and now propagated to SmartThings Signed-off-by: alon-tchelet <[email protected]>

Harman don't want it and don't plan to use it in the future Signed-off-by: alon-tchelet <[email protected]>

We are moving the device communications between the hub and the device to WebSockets. Until the ST lustre library WebSocket keep_alive ping bug is resolved, this lustre/ws.lua will be used instead. Signed-off-by: alon-tchelet <[email protected]>

Signed-off-by: alon-tchelet <[email protected]>

the removed APIs are no longer used after moving to WebSockets Signed-off-by: alon-tchelet <[email protected]>

this is done to further match the discovery behaviour to the example code and to return the handshake process control to the device. Signed-off-by: alon-tchelet <[email protected]>

The REST HTTP API polling was creating too much conjunction and was unreliable at time (especially in weaker networks). Hence, we changed the whole communication set up between the hub and the device to WebSockets. With the exception of fetching the device information at discovery, the rest of the communication is done through a WebSocket. We implemented a JSON protocol that tries to replicate the native SmartThings capabilities' table structures and the offline/online logic is now handled fully by the WebSocket connection status. Moreover, a lot of old and no longer used functions and namings have been modified or deleted altogether. Signed-off-by: alon-tchelet <[email protected]>

Signed-off-by: alon-tchelet <[email protected]>

alon-tchelet · 2024-07-02T09:07:07Z

Sorry, I did it locally and forgot pushing to remote. All good now.
Do you know how long will it be until this driver will get to end users?

cjswedes · 2024-07-02T14:14:37Z

If all goes well with the release. Itll go to the beta channel today, and the production channel next week Monday.

alon-tchelet mentioned this pull request Jun 11, 2024

Harman-Luxury Improve device update communications #1360

Closed

cjswedes reviewed Jun 13, 2024

View reviewed changes

dljsjr reviewed Jun 13, 2024

View reviewed changes

drivers/SmartThings/harman-luxury/src/api/apis.lua Show resolved Hide resolved

dljsjr reviewed Jun 13, 2024

View reviewed changes

drivers/SmartThings/harman-luxury/src/api/nsdk.lua Show resolved Hide resolved

dljsjr reviewed Jun 13, 2024

View reviewed changes

alon-tchelet force-pushed the harman-websocket branch from 77d1b36 to 058320e Compare June 25, 2024 12:02

dljsjr reviewed Jun 25, 2024

View reviewed changes

dljsjr approved these changes Jun 25, 2024

View reviewed changes

alon-tchelet force-pushed the harman-websocket branch 2 times, most recently from 4b9bcdd to afcf23a Compare June 25, 2024 15:50

cjswedes approved these changes Jun 26, 2024

View reviewed changes

alon-tchelet force-pushed the harman-websocket branch from afcf23a to 805e83a Compare July 2, 2024 09:04

alon-tchelet added 8 commits July 2, 2024 11:05

remove media presents for MA AVRs

cbac968

Harman decision applied on device and now propagated to SmartThings Signed-off-by: alon-tchelet <[email protected]>

remove keypad input support

72cc6a5

Harman don't want it and don't plan to use it in the future Signed-off-by: alon-tchelet <[email protected]>

add a specific timeout error handler in REST HTTP reply handler

7666603

Signed-off-by: alon-tchelet <[email protected]>

delete unused APIs

0a35f4b

the removed APIs are no longer used after moving to WebSockets Signed-off-by: alon-tchelet <[email protected]>

move credential generation to device side

d2467c1

this is done to further match the discovery behaviour to the example code and to return the handshake process control to the device. Signed-off-by: alon-tchelet <[email protected]>

fix handleReply function documentation

a3dc9a6

Signed-off-by: alon-tchelet <[email protected]>

alon-tchelet force-pushed the harman-websocket branch from 805e83a to ad42c0d Compare July 2, 2024 09:05

alon-tchelet force-pushed the harman-websocket branch from ad42c0d to a3dc9a6 Compare July 2, 2024 09:12

cjswedes merged commit 2edb6c6 into SmartThingsCommunity:main Jul 2, 2024
11 checks passed

alon-tchelet deleted the harman-websocket branch July 2, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harman Luxury Change from REST HTTP API to WebSckets #1433

Harman Luxury Change from REST HTTP API to WebSckets #1433

alon-tchelet commented Jun 11, 2024

github-actions bot commented Jun 11, 2024

github-actions bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

cjswedes left a comment

dljsjr commented Jun 13, 2024 •

edited

Loading

dljsjr Jun 13, 2024

alon-tchelet Jun 14, 2024

dljsjr Jun 13, 2024

alon-tchelet commented Jun 14, 2024

alon-tchelet commented Jun 19, 2024

dljsjr Jun 25, 2024

alon-tchelet Jun 25, 2024

dljsjr left a comment

alon-tchelet commented Jun 25, 2024

cjswedes commented Jul 1, 2024 •

edited

Loading

alon-tchelet commented Jul 2, 2024

cjswedes commented Jul 2, 2024

Harman Luxury Change from REST HTTP API to WebSckets #1433

Harman Luxury Change from REST HTTP API to WebSckets #1433

Conversation

alon-tchelet commented Jun 11, 2024

github-actions bot commented Jun 11, 2024

github-actions bot commented Jun 11, 2024 • edited Loading

Test Results

github-actions bot commented Jun 11, 2024 • edited Loading

cjswedes left a comment

Choose a reason for hiding this comment

dljsjr commented Jun 13, 2024 • edited Loading

dljsjr Jun 13, 2024

Choose a reason for hiding this comment

alon-tchelet Jun 14, 2024

Choose a reason for hiding this comment

dljsjr Jun 13, 2024

Choose a reason for hiding this comment

alon-tchelet commented Jun 14, 2024

alon-tchelet commented Jun 19, 2024

dljsjr Jun 25, 2024

Choose a reason for hiding this comment

alon-tchelet Jun 25, 2024

Choose a reason for hiding this comment

dljsjr left a comment

Choose a reason for hiding this comment

alon-tchelet commented Jun 25, 2024

cjswedes commented Jul 1, 2024 • edited Loading

alon-tchelet commented Jul 2, 2024

cjswedes commented Jul 2, 2024

github-actions bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

dljsjr commented Jun 13, 2024 •

edited

Loading

cjswedes commented Jul 1, 2024 •

edited

Loading