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

59 files 376 suites 0s ⏱️
1 808 tests 1 808 ✅ 0 💤 0 ❌
3 143 runs 3 143 ✅ 0 💤 0 ❌

Results for commit afcf23a.

♻️ 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 afcf23a

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 🎉

Harman decision applied on device and now propagated to SmartThings Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

Harman don't want it and don't plan to use it in the future Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

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 <alon.tchelet@streamunlimited.com>

Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

the removed APIs are no longer used after moving to WebSockets Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

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 <alon.tchelet@streamunlimited.com>

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 <alon.tchelet@streamunlimited.com>

Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

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

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

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 from 058320e to 4b9bcdd Compare June 25, 2024 14:51

alon-tchelet added 10 commits June 25, 2024 16:53

remove media presents for MA AVRs

50682a3

Harman decision applied on device and now propagated to SmartThings Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

remove keypad input support

f71a602

Harman don't want it and don't plan to use it in the future Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

add a specific timeout error handler in REST HTTP reply handler

fa0c52f

Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

delete unused APIs

7006c0d

the removed APIs are no longer used after moving to WebSockets Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

move credential generation to device side

98f5990

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 <alon.tchelet@streamunlimited.com>

fixup! change communications from REST HTTP API to WebSockets

b9f02f6

Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

fixup2! change communications from REST HTTP API to WebSockets

84bed31

Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

fix handleReply function documentation

afcf23a

Signed-off-by: alon-tchelet <alon.tchelet@streamunlimited.com>

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

cjswedes approved these changes Jun 26, 2024

View reviewed changes

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

Harman Luxury Change from REST HTTP API to WebSckets #1433

Are you sure you want to change the base?

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

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