Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch wsconnector to internal socket client #3

Merged
merged 8 commits into from
Jan 24, 2023
Merged

Conversation

AIIX
Copy link

@AIIX AIIX commented Jan 12, 2023

  • Introduces a custom socket asynchronous HomeAssistantClient that replaces HASS client implementation with minimal changes on connector side
  • Fixes all bugs and errors encountered with using the HASS Client
  • Removes the "use_websocket" config key as this is already discovered at runtime on instance setup and on startup for already configured instances
  • Moves all debug behind a "enable_debug" configuration flag to keep the PHAL client logs clean

@AIIX AIIX requested a review from NeonDaniel January 13, 2023 22:10
Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed changes and the look good, couple bugs from testing (local wss connection):

  • After turning off a device on or off in the UI, the new state isn't displayed until I go to a different view and come back (only noticed in grouped view, appeared to work in devices view)

Other less important notes:

  • Camera previews work, but playback does not
  • When I display grouped, the bottom bar is all ..., could this be made scrollable?
  • All of my rooms have s appended (i.e. "Hallways", "Kitchens", "Downstairss"

@AIIX
Copy link
Author

AIIX commented Jan 20, 2023

This PR additionally now adds support for QR Code based oauth authentication for quicker setups. Additionally bugs mentioned above in the initial review have also now been fixed

Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have 19 areas displayed on the home screen, but only 8 are scrollable on the bottom bar before the right scroll arrow becomes grayed out; it stops scrolling after 4 presses if that's relevant

@AIIX AIIX requested a review from NeonDaniel January 24, 2023 04:34
Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a dependency version change, otherwise working well

requirements.txt Outdated
@@ -1,7 +1,7 @@
ovos-plugin-manager>=0.0.1
ovos-utils~=0.0.27a3
ovos-utils~=0.0, >=0.0.28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no 0.0.28 release, this should probably be ovos-utils~=0.0.27?

@@ -1,7 +1,7 @@
ovos-plugin-manager>=0.0.1
ovos-utils~=0.0.27a3
ovos-utils>=0.0.27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be ~=0.0.27 to prevent updating to an incompatible release (i.e. 1.0.0) in the future

@AIIX AIIX merged commit 6f4124b into master Jan 24, 2023
@j1nx j1nx deleted the feat/socket_client branch May 23, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants