Bugfixes and new feature -- currently testing at home by jeffmaki · Pull Request #40 · sman591/homebridge-lg-thinq-ac

jeffmaki · 2020-08-15T22:26:44Z

Currently testing in my setup via my day-to-day usage. Feel free to join me? LMK if you have any thoughts/feedback?

Fix for #17: confirmed fixed for fahrenheit units, TBD for celsius
Fix for #9: duplicate of #17
Fix for #16: requires user to choose in Homebridge UI since HomeKit cannot support custom characteristics (e.g. a switch for this)
Fix for #18
Fix for #19
Plus, new feature: display filter cleaning status

Fix for sman591#17: confirmed fixed for fahrenheit units, TBD for celsius Fix for sman591#9: duplicate of sman591#17 Fix for sman591#16: requires user to choose in Homebridge UI since HomeKit cannot support custom characteristics (e.g. a switch for this) Fix for sman591#18 Fix for sman591#19 Plus, new feature: display filter cleaning status

Plus this file that should have been part of the last commit

sman591 · 2020-08-15T23:29:21Z

Hey, thanks for contributing! Just glanced through everything and this looks reasonable. I'll try it out locally and give it a full review later -- feel free to convert to a full PR when you're ready.

For the future, please split up each "major" contribution into its own PR. This should be ok for now, but it helps chunk up the review to each relevant change and benefits from automated changelog/release tooling

sman591

Tested locally, looking good! Thanks for tackling these

config.schema.json

src/characteristic/abstractCharacteristic.ts

src/characteristic/abstractSplitTemperatureThresholdCharacteristic.ts

sman591 · 2020-08-16T22:56:02Z

src/characteristic/targetHeaterCoolerStateCharacteristic.ts

+    this.device.updateCharacteristics(
+      v.newValue === this.characteristic.AUTO ? true : false,
+      this.platform.Characteristic.TargetHeaterCoolerState.UUID,
+    )


This is a clever way to accomplish what we need, but given a "cached" snapshot can be stale, this could inadvertently revert previous changes made in the UI since the last update interval.

Open to discussing a solution here, but it might be best in a separate PR. It seems like this is only necessary because changing the lockTemperature value has implications on other characteristics; I wonder if we'd want some way to subscribe to changes that happen because of it, or mark characteristics as dependent on this flag?

Would marking them dependent on each other do something in the UI? This is the best I could find given the way the Home app UI works. Regardless of the way we tackle this engineering-wise, do you know if we can make the UI more sensical in this case?

"Marking dependent" would be only an internal concept to this plugin; no native UI change. More as a way for characteristics to optionally subscribe to important changes such that we don't have to trigger a full cache-based refresh

Got it. Well, the "temperatureLocked" concept is trying to be that. So if we put aside the particular engineering approach to implement, how would you suggest we handle the UI if not a cache-based refresh?

sman591 · 2020-08-16T22:59:10Z

src/characteristic/abstractSplitTemperatureThresholdCharacteristic.ts

+  handleSet?(value: CharacteristicValue, callback: CharacteristicSetCallback) {
+    if (this.device.lockTemperature) {
+      // updating from cache when we're locked puts things back to where they were, essentially preventing edits
+      this.device.updateCharacteristics(true)


This could be a potentially stale snapshot. I'd prefer we ignore the request or augment the handleSet / callback usage for this characteristic alone

Okay, but then the user can change the temperature to something the AC won't accept, and the temperature will just revert back next refresh. Which could be 10 minutes from now. I didn't think that was great either...

I sort of think of it as one action -> one failure (ignored input) being better than (potentially) one action -> stale UI due to cache -> in-sync UI moments later. It's unfortunate that HomeKit doesn't support this set of modes appropriately but I think it could be more confusing to flash the user with a stale UI for this one case

Got it. You should try disabling the "lockTemperature" flag and run the plugin to see what happens without it--it's not great. Basically you still get what you could be construed as a cached update: the temperature stays where the user set it, then snaps back to what the AC is actually at next refresh, potentially 10 minutes later. So my thinking here is that the user thinks they set the temperature to something preferable, then later realize the update didn't work. This would happen 100% of the time, whereas the cache being stale would be only when the connection between Thinq and the plugin is down, which I assumed was less than that. So TL;DR: this approach shows the correct state of the AC more often than any alternative.

I agree the real fix would be on the HomeKit side, but alas, can't find any fix there. Know anyone at Apple to ask what they would do in this case? Maybe there's something we don't know here...

src/characteristic/abstractCharacteristic.ts

src/platformAccessory.ts

….g. on portable units

src/platformAccessory.ts

sman591 · 2020-09-08T00:54:44Z

src/platformAccessory.ts

        try {
-          characteristic.handleUpdatedSnapshot(device.result.snapshot)
+          characteristic.handleUpdatedSnapshot(
+            <Unpacked<GetDeviceResponse['result']['snapshot']>>device,


This is happening because let device = this.getDevice().snapshot is somehow returning unknown when it should be ... | unknown -- not sure why that is, but I've pasted a suggestion above that should resolve this

sman591 · 2020-09-08T00:55:18Z

src/platformAccessory.ts

+          characteristic.handleUpdatedSnapshot(
+            <Unpacked<GetDeviceResponse['result']['snapshot']>>device,
+          )


Suggested change

characteristic.handleUpdatedSnapshot(

<Unpacked<GetDeviceResponse['result']['snapshot']>>device,

)

characteristic.handleUpdatedSnapshot(snapshot)

src/platform.ts

src/characteristic/abstractCharacteristic.ts

sman591 · 2020-09-08T01:26:39Z

src/characteristic/abstractCharacteristic.ts

  }
+
+  deviceUsesFahrenheit(): boolean {
+    return this.device.getDevice().countryCode.startsWith('US')


I'm a bit confused -- is this necessary? My understanding was both the LG and HomeKit APIs operate in Celsius, and it's just a matter of what HomeKit/LG display on screen that differs

Is it necessary? Can't say with units that display celsius. So this was my attempt to limit my changes to only Fahrenheit units, and if we find it's needed with celsius units, we can change. But somebody else would have to test that (someone who has a celsius unit)

sman591 · 2020-09-08T01:29:09Z

src/characteristic/abstractCharacteristic.ts

+  */
+  getHomeKitCelsiusForLGAPICelsius(_celsius: number): number {
+    if (!this.deviceUsesFahrenheit()) {
+      return _celsius


Is there harm in always performing the conversion? I believe both Celsius and Fahrenheit users will see these discrepancies due to rounding differences, right?

Co-authored-by: Stuart Olivera <stuart@stuartolivera.com>

sman591 · 2020-10-26T04:18:15Z

src/characteristic/abstractCharacteristic.ts

        callback(error)
+
+        // put UI back to where it was before
+        callback(error, this.cachedState)


Could you merge this with line 148?

Suggested change

callback(error)

// put UI back to where it was before

callback(error, this.cachedState)

// put UI back to where it was before

callback(error, this.cachedState)

bluefangs · 2021-04-19T07:35:19Z

@jeffmaki - Is this something that is still WIP / ongoing testing?

bluefangs · 2021-04-19T16:29:33Z

I ran this branch to poke out differences. Noticed these in the logs.

[4/19/2021, 9:56:18 PM] [LgThinqAirConditioner] Error during interval update Error: connect ETIMEDOUT 13.124.239.180:46030
[4/19/2021, 9:56:23 PM] [LgThinqAirConditioner] Error during interval update Error: connect ETIMEDOUT 13.124.239.180:46030
[4/19/2021, 9:56:23 PM] [LgThinqAirConditioner] Error during interval update Error: connect ETIMEDOUT 13.124.239.180:46030
[4/19/2021, 9:56:31 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com
[4/19/2021, 9:56:32 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com
[4/19/2021, 9:56:33 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com
[4/19/2021, 9:56:33 PM] [LgThinqAirConditioner] Error during interval update Error: connect ETIMEDOUT 52.79.134.204:46030
[4/19/2021, 9:56:33 PM] [LgThinqAirConditioner] Error during interval update Error: connect ETIMEDOUT 52.79.134.204:46030
[4/19/2021, 9:56:34 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com
[4/19/2021, 9:56:36 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com
[4/19/2021, 9:56:37 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com
[4/19/2021, 9:56:38 PM] [LgThinqAirConditioner] Error during interval update Error: getaddrinfo ENOTFOUND kic-service.lgthinq.com

part of #40

@jeffmaki

# [1.12.0](v1.11.0...v1.12.0) (2021-04-20) ### Features * add filter life & condition (thanks [@jeffmaki](https://github.com/jeffmaki)!) ([6c94d0f](6c94d0f)), closes [#40](#40)

part of #40 fixes #18

@jeffmaki

## [1.13.2](v1.13.1...v1.13.2) (2021-04-20) ### Bug Fixes * emit status on Homebridge boot (thanks [@jeffmaki](https://github.com/jeffmaki)!) ([97bfbc5](97bfbc5)), closes [#40](#40) [#18](#18)

jbyerline · 2021-08-26T06:11:42Z

Any word on getting this merged? I have a LG AC with Eco-Mode and would love the addition? Any known bugs since its been a year? @jeffmaki @sman591

jeffmaki added 3 commits August 15, 2020 17:52

Add missing file

9d27afb

Plus this file that should have been part of the last commit

Fix filename problem for case sensitive filesystems

e8c7e59

sman591 suggested changes Aug 16, 2020

View reviewed changes

jeffmaki added 3 commits August 17, 2020 10:12

Changes in response to code review

1645f85

ESLint changes (forgot to enable this in VC last commit)

aee8074

Fix bug where eco mode was selected when not available on the unit, e…

d847daa

….g. on portable units

sman591 suggested changes Sep 8, 2020

View reviewed changes

jeffmaki and others added 4 commits September 8, 2020 22:19

Apply suggestions from code review

055a511

Co-authored-by: Stuart Olivera <stuart@stuartolivera.com>

lint fix after code review

036e2a0

One more fix after code review for lint

c4270e7

Update platformAccessory.ts

304e6da

sman591 reviewed Oct 26, 2020

View reviewed changes

jeffmaki added 2 commits November 10, 2020 14:24

Changes from code review

825261e

Remove duplicate call

40450ac

sman591 mentioned this pull request Apr 20, 2021

Reorder the mapping / support Model: MS-Q12SWDZ #70

Open

sman591 added a commit that referenced this pull request Apr 20, 2021

feat: add filter life & condition (thanks @jeffmaki!)

6c94d0f

part of #40

sman591 added a commit that referenced this pull request Apr 20, 2021

fix: emit status on Homebridge boot (thanks @jeffmaki!)

97bfbc5

part of #40 fixes #18

Conversation

jeffmaki commented Aug 15, 2020

Uh oh!

sman591 commented Aug 15, 2020

Uh oh!

sman591 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluefangs commented Apr 19, 2021

Uh oh!

bluefangs commented Apr 19, 2021

Uh oh!

jbyerline commented Aug 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sman591 left a comment •

edited

Loading