Add nine Korean LG appliances, an ac_common base, and persistent driver stores - #111
Add nine Korean LG appliances, an ac_common base, and persistent driver stores#111tunarabuuu wants to merge 11 commits into
Conversation
b851db3 to
935fc0d
Compare
Was this If that code was never upstream, then we really needn't/shouldn't include cleanup for it upstream. |
|
As in #110, please split this giant commit (14k lines changed) into smaller one, one commit per logical change as this is basically non-reviewable in this form. |
935fc0d to
278d5aa
Compare
|
You're right — it was never upstream code. Those diagnostic entities only ever existed on my own add-on's development builds (and the drivers here graduated out of that setup), so the cleanup belongs on my side, not here. Removed in the latest push: |
|
Done — split into 13 commits: four infrastructure pieces first (discovery types and the 0xA7 acceptance, both from #64; the two JSON stores; the bridge threading), then one commit per appliance, each carrying its driver, its tests, and its own registry entry. Every intermediate commit type-checks on its own, and the final tree is byte-identical to the previous push — 502 tests passing. |
278d5aa to
f918ed0
Compare
| * Rename a device's components into Korean. | ||
| * | ||
| * The appliances here are Korean-market units: their own panel, the LG app and the cloud | ||
| * integration all use Korean, so a driver that labels half its entities in English leaves the |
There was a problem hiding this comment.
Can't this be done in HA instead? I know one can re-name devices and their individual components in HA.
There was a problem hiding this comment.
It can, but only for the display names, and one entity at a time. These appliances publish a lot of entities (the WashTower pair alone exposes ~230), so "rename it in HA" means a per-installation manual pass that resets on a fresh install or a re-discovery.
The bigger half can't be renamed in HA at all: the state values and select options these drivers publish are Korean labels decoded from the appliance's own enums (the same strings the unit's panel and the LG app show — that's what the tests pin). HA renaming covers an entity's name, not the values flowing through it, so without applyKoreanNames the device reads half-Korean/half-English no matter what you do in the UI.
So the helper only makes the last bit — component display names — consistent with what the rest of the device already speaks. That said, if you'd rather have English defaults with Korean as an option (or a config switch), I'm happy to restructure it that way.
I don't see I also think that introducing |
Originates in PR anszom#64 by BluSyn; carried here for the humidifier and dehumidifier drivers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dehumidifier family frames with 0xA7 where RAC/WIN use 0x87. From PR anszom#64 by BluSyn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Usage history accumulates locally in a small JSON store keyed by device id so daily counters survive restarts; without a storage path drivers run stateless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rides as an optional constructor parameter and reaches any driver exposing a setter for it; rethink-cloud constructs it from the bridge storage path when one is configured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f918ed0 to
33344a5
Compare
|
Good call — You're right on both counts: I hadn't converted So I removed Rebased to 11 commits (one per appliance + the shared pieces), each type-checks on its own; 485 tests passing. |
| * | ||
| * Only the two enums differ, so that is all this overrides. | ||
| * | ||
| * Labels are Korean because this model is sold in Korea and its panel, the LG |
There was a problem hiding this comment.
The _WW suffix would suggest this is a World Wide device.
It may be sold with panel localized to particular market, but I wouldn't take it as a guarantee that device identifying as DHUM_231006_WW isn't sold in any other market with a different panel language.
There was a problem hiding this comment.
Same remark for other _WW devices in this PR.
| * Only the two enums differ, so that is all this overrides. | ||
| * | ||
| * Labels are Korean because this model is sold in Korea and its panel, the LG | ||
| * app, and the cloud integration all use these names — matching them keeps one |
There was a problem hiding this comment.
The official app and HA integration both separate translations from code.
The RTL8720cm generation of the 056905 dehumidifier; reuses the 056905 mode tables and sends fan writes as bare 0x1fa without the per-mode table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wire map taken from LG's own modelJSON tlv_* labels rather than guessed tags. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AA FF frame layout established with the LG decode oracle; every enum verified against a live unit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same decode oracle as the Styler; a 12-field AA..BB frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Capability-proven settings only, with usage history persisted through the purifier history store. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FAKPK21021 (washer, deviceType 221) and BDH_D39301_KR (dryer, 222) speak a dedicated 0xEB/0xEC frame encoding decoded in a shared washtower_common base. Complementary to the NA WashTower in anszom#102, which is a different generation with a different frame format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
33344a5 to
79375cb
Compare
|
Done — everything is English now, no Korean strings anywhere in these drivers. You're right that If a proper i18n layer (canonical keys + a translation table, like the app/HA integration do) is something you'd want, I'm happy to build that as a follow-up — but this at least gets the defaults right for any market. Re-pushed: still 11 commits (one per appliance + the shared pieces), each type-checks on its own, 485 tests passing. |
anszom
left a comment
There was a problem hiding this comment.
I'll submit partial feedback for now. I've tried to review this in one go but it would take much more time than I expected.
After you go through the partial comments, please split this into one PR per device-class (so, one for humidifers, one for washtowers, etc).
| /* | ||
| * LG PuriCare Water Purifier (ATOM-U STS T20 Cold/Hot/Purified/Steril), ThinQ model 1WPU4CIGCR__2, deviceType 103. | ||
| * | ||
| * THE COMMAND FRAME, AND WHY IT LOOKED LIKE THERE WASN'T ONE |
There was a problem hiding this comment.
That's quite a lot of prose, with references to out-of-repo files, and various terms which aren't immediately obvious (Config.supportControl, legacy control path, capability API - I don't know what this refers to, and it isn't necessary to understand the device-side protocol).
Please shorten/remove. Focus on what is needed to understand the protocol.
| const pad2 = (n: number) => String(n).padStart(2, '0') | ||
| const kstParts = (time: number) => { | ||
| const date = new Date(time + KST_OFFSET_MS) | ||
| return { | ||
| year: date.getUTCFullYear(), | ||
| month: date.getUTCMonth() + 1, | ||
| day: date.getUTCDate(), | ||
| hour: date.getUTCHours(), | ||
| minute: date.getUTCMinutes(), | ||
| } | ||
| } | ||
| const kstDay = (time: number) => { | ||
| const p = kstParts(time) | ||
| return `${p.year}-${pad2(p.month)}-${pad2(p.day)}` | ||
| } | ||
| const kstMonth = (time: number) => { | ||
| const p = kstParts(time) | ||
| return `${p.year}-${pad2(p.month)}` | ||
| } | ||
| const previousKstMonth = (time: number) => { | ||
| const p = kstParts(time) | ||
| return kstMonth(Date.UTC(p.year, p.month - 2, 15) - KST_OFFSET_MS) | ||
| } | ||
| const previousKstMonthStart = (time: number) => { | ||
| const p = kstParts(time) | ||
| return Date.UTC(p.year, p.month - 2, 1) - KST_OFFSET_MS | ||
| } | ||
| const nextKstMidnight = (time: number) => { | ||
| const shifted = new Date(time + KST_OFFSET_MS) | ||
| return Date.UTC(shifted.getUTCFullYear(), shifted.getUTCMonth(), shifted.getUTCDate() + 1) - KST_OFFSET_MS | ||
| } | ||
| const formatClock = (hour: number, minute: number) => { | ||
| const period = hour < 12 ? 'AM' : 'PM' | ||
| return `${hour % 12 || 12}:${pad2(minute)} ${period}` | ||
| } | ||
| const formatKst = (time: number | undefined) => { | ||
| // This bridge can only attest to completions it observed. An absent timestamp does not | ||
| // prove the appliance has never completed a cycle before collection began. | ||
| if (time == null) return 'Not yet observed' | ||
| const p = kstParts(time) | ||
| return `${p.year}-${pad2(p.month)}-${pad2(p.day)} ${pad2(p.hour)}:${pad2(p.minute)}` | ||
| } |
There was a problem hiding this comment.
A few comments here.
- Time formatting utilities should be moved to somewhere in util/...
- Is anything here strictly bound to the KST timezone?
- If not, then it would be both simpler & more general to simply operate in the user's local time.
| * The base class strips the AA/length prefix before handing the body on, but every offset | ||
| * above is stated against the WHOLE frame — that is how the probes recorded them, and | ||
| * keeping the two in the same coordinate system is what makes the map checkable. So the | ||
| * frame is taken here, unstripped. |
There was a problem hiding this comment.
You can simply subtract two and now you have offsets relative to the internal AABB payload. No reason to circumvent the regular code path used by all other devices.
| applyPurifierUsageBaseline(day: string, baseline: PurifierDailyUsage) { | ||
| if (!this.history || this.historyUnavailable || day !== kstDay(this.clock.now())) return false | ||
|
|
||
| const next = copyHistory(this.history) | ||
| if (next.usage.day !== day) next.usage = emptyUsage(day) | ||
|
|
||
| const pendingUsage = this.pendingUsage | ||
| const localUsage = { ...next.usage } | ||
| if (pendingUsage?.day === day) { | ||
| for (const key of Object.keys(pendingUsage.delta) as (keyof UsageDelta)[]) | ||
| localUsage[key] += pendingUsage.delta[key] | ||
| } | ||
| for (const key of Object.keys(baseline) as (keyof PurifierDailyUsage)[]) | ||
| next.usage[key] = Math.max(localUsage[key], baseline[key]) | ||
| next.usageComplete = true | ||
|
|
||
| // The live driver is the sole owner of both its memory snapshot and JSON file. | ||
| // Suppress saveHistory's normal pending-delta fold because it is already included | ||
| // in localUsage above; restore it if the atomic save fails. | ||
| this.pendingUsage = undefined | ||
| if (!this.saveHistory(next)) { | ||
| this.pendingUsage = pendingUsage | ||
| return false | ||
| } | ||
| this.history = next | ||
| this.publishHistory() | ||
| return true | ||
| } |
There was a problem hiding this comment.
this, along with some other parts appear to be used only in tests. What is the purpose?
|
|
||
| export default class Device extends AABBDevice { | ||
| private readonly legacyPlatformCleanup: DeviceDiscovery | ||
| private historyStore: PurifierHistoryStore | undefined |
There was a problem hiding this comment.
I was trying to avoid the need to store any persistent history in rethink - usually these things can be supported by HA, and rethink serves only as a simple protocol translator.
So far I think the purifier can be supported in a stateless manner too. HA can handle deltas/accumulated values (even with reset-to-zero, see for example energy consumption for the RAC_...). Any historical data should similarly be retained by HA instead of rethink.
| setProperty(prop: string, mqttValue: string) { | ||
| const write = (type: number, table: Record<number, string>, what: string) => { | ||
| const code = Object.entries(table).find(([, label]) => label === mqttValue)?.[0] | ||
| if (code === undefined) return console.warn(`Stick Vacuum: Unknown ${what} '${mqttValue}'`) |
| * | ||
| * WHERE THE BYTE OFFSETS COME FROM | ||
| * -------------------------------- | ||
| * This model's modelJSON has no tlv_* labels (unlike the clip/TLV humidifier and |
| * Offsets above are stated against the whole frame, the way the probes recorded them, so | ||
| * the frame is read unstripped rather than through the base class's body view. | ||
| */ | ||
| processData(buf: Buffer) { |
| return this.echo(prop, mqttValue.trim()) | ||
| } | ||
| default: | ||
| console.warn(`Styler: Item does not support writing ${prop}`) |
|
|
||
| /* LG official maps: FAKPK21021 WASHER_PROTOCOL_EX 8.3 / WASHER_CONVERT_EX 2.3; | ||
| * BDH_D39301_KR DRYER_PROTOCOL_EX 4.1 / DRYER_CONVERT_EX 7.8. */ | ||
| export const WASHER_MODEL: WashTowerModel = { |
There was a problem hiding this comment.
Lots of keywords that have no meaning if you don't know the context.
How was this table created?
|
I also should have stated this at the beginning: Thank you! I appreciate the effort, this will significantly expand our support base. I apologize if my initial comments sounded dismissive - I got a bit overwhelmed with the scope of the review. |
|
Thanks — this is exactly the kind of review I was hoping for. I'll do the split: one PR per device-class, and go through the comments on each as I carve them out. Concretely I'm planning:
On the recurring points, so you don't have to repeat them per PR:
I'll close this PR once the per-device ones are up, and link them here. Starting with the dehumidifiers. |
|
Split complete — closing this in favour of the six per-device-class PRs, each rebased on master with your review comments addressed:
The |
|
I would like to integrate your |
|
@3735943886 Yes, absolutely — please go ahead, and thank you for asking. I'd be glad to see To make it easy to pull from, I've pushed the code to a reference branch on my fork: tunarabuuu/rethink@ Two notes for integration:
Every enum in |
This adds support for nine Korean-market LG appliances I've been running daily through rethink, each verified against a live unit with bridge mode on (so every mapping had three witnesses: the value HA set, the tag the appliance reported, and
lge_thinq's decode of the same appliance), and each covered by decode-vector regression tests.Credit where it's due
DHUM_056905_WWbuilds directly on @BluSyn's #64 — that driver, thetlv_device0xA7 acceptance, and theHumidifierComponentdiscovery type all originate there (the in-code comments say so too). This PR's version extends it: properties beyond the panel's reach mapped from the model JSON, English labels, and aModeTablesexport that the newerDHUM_231006_WWgeneration reuses. If #64 lands first I'll gladly rebase on top of it — it deserves to land on its own merits.New device handlers
CST_170004_WWac_commonbaseDHUM_056905_WWDHUM_231006_WWHUM_056905_WWtlv_*labelsST_B_E4H01Y_APLHWWA9K_F21WPU4CIGCR__2FAKPK21021BDH_D39301_KRwashtower_commonbaseAbout
ac_common: in #101 you suggested splitting a shared AC base class instead of translating packets to RAC's expectations — this PR contains that split, andCST_170004_WWis its first user. @3735943886'sCST_570004_WWceiling cassette should be able to sit on the same base.The Korean WashTower models are complementary to #102's
WTL_FXU_BDV_NA_01(different generation, different frame format — these speak a dedicated 0xEB/0xEC encoding decoded inwashtower_common).Infrastructure the drivers stand on
TLVDevice: byte 6 may be0xA7as well as0x87— the dehumidifier family frames with0xA7(from Add support for LG dehumidifier DHUM_056905_WW #64).ReservationStore(absolute reservation deadlines survive restarts, used by the AC) and aPurifierHistoryStore(water-purifier usage history), both small JSON stores threaded through the bridge as optional constructor parameters. Without a bridge storage path the drivers run stateless.HumidifierComponent(from Add support for LG dehumidifier DHUM_056905_WW #64), plusmodes/preset_modesonClimateComponent.Verification
npm test— 502 passed, 0 failed (251 upstream + 251 new)tsc -p tsconfig.build.json --noEmit— cleanprettier --check— cleanIndependent of #110 (only trivially-mergeable overlap in
cloud/homeassistant.tstype declarations). Happy to split this into per-device PRs if that's easier to review.🤖 Generated with Claude Code