From 248e473abe5ddf46dbcbf37ff4abbe8bc946e303 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Sun, 9 Aug 2026 16:00:14 +0000 Subject: [PATCH] Fix AC temperature step quantization (PR #276, code review) Samsung local AC temperature writes always rounded to the nearest whole degree, dropping half-degree setpoints on boards that advertise a 0.5 step (CAC and TP1X FAC). Squashed from moridew's PR #276 with the review fixes applied: - The /temperatures/vs/0 fallback never matched: its increment lives inside the resource's items[] array, not at the top level (same shape _temps_vs_item() already unwraps for current/unit). The original fix only ever worked through /temperature/control/vs/0. - With no increment advertised anywhere (e.g. ARTIK051), writes went out unrounded instead of falling back to whole degrees the way climate.py's target_temperature_step already does. - A non-numeric payload now rejects the write (returns None) instead of posting {"temperature": null} -- coordinator.py's async_send_command already drops a write_fn result of None. - int/float normalization now happens once, in _quantize_temperature, instead of being duplicated (and skipped) per branch; a round(..., 2) guards against float division noise (e.g. 21.7 / 0.1). Tests rebuilt against real fixture resources (airconditioner_cac, airconditioner_artik051_krac_18k) instead of a fabricated flat resource shape no device produces. --- .../registry/capabilities/airconditioner.py | 66 ++++++++++++++++++- tests/test_airconditioner_capabilities.py | 53 +++++++++++++++ 2 files changed, 116 insertions(+), 3 deletions(-) diff --git a/custom_components/localthings/registry/capabilities/airconditioner.py b/custom_components/localthings/registry/capabilities/airconditioner.py index 4eac2be..9688776 100644 --- a/custom_components/localthings/registry/capabilities/airconditioner.py +++ b/custom_components/localthings/registry/capabilities/airconditioner.py @@ -491,7 +491,61 @@ def _humidity(rep): return None -def _climate_write(payload, rep, href=None): +def _quantize_temperature(value, step): + """Quantize a temperature to the device's advertised step size. + + Mirrors climate.py's `target_temperature_step`, which defaults to `1.0` + when no increment is advertised anywhere (`step=None` or `<=0` here) -- + so a board with no increment still gets whole-degree writes instead of + the raw value passed through unrounded. Returns `None` for a payload + that isn't numeric, so the caller can reject the write instead of + building a body around a null (coordinator.py's async_send_command + already logs and drops a write_fn result of None). + + Normalizes an integral result to `int` here, once, so both write + branches serialize `24` rather than `24.0` -- `round(..., 2)` guards + against float noise in the division (e.g. `21.7 / 0.1`). + """ + try: + numeric = float(value) + except (TypeError, ValueError): + return None + step_value = _num(step) or 1.0 + if step_value <= 0: + step_value = 1.0 + quantized = round(round(numeric / step_value) * step_value, 2) + return int(quantized) if quantized.is_integer() else quantized + + +def _temperature_step(resources): + """Device-reported temperature increment, read the same way and in the + same order as climate.py's `target_temperature_step`: OCF + `/temperature/control/vs/0`'s `increment` (or its vendor-prefixed + twin), falling back to vendor `/temperatures/vs/0`. The increment on + that second resource lives inside its `items[]` array like every other + per-item field, not at the resource's top level -- unwrapped via + `_temps_vs_item()`, the same helper `_temps_vs_current`/`_temps_vs_unit` + use above, rather than read off `resource` directly. + + `rep` (the bound entity's own resource, `/mode/vs/0` for ClimateDesc -- + see rep_fn=_first_mode below) never carries an increment, so it isn't a + candidate here; only the coordinator's `resources` snapshot is.""" + if not isinstance(resources, dict): + return None + control = resources.get(HREF_TEMP_CONTROL) + if isinstance(control, dict): + step = _num(control.get("increment")) or _num(control.get("x.com.samsung.da.increment")) + if step is not None: + return step + temps_vs = resources.get(HREF_TEMPS_VS) + if isinstance(temps_vs, dict): + step = _num(_temps_vs_item(temps_vs).get("x.com.samsung.da.increment")) + if step is not None: + return step + return None + + +def _climate_write(payload, rep, href=None, resources=None): """Maps a (kind, value) command from the climate platform to the (path_segs, body) for that one sub-write; `value` is already the raw device code. Power always goes to vendor `/power/vs/0` (OCF `/power/0` @@ -505,16 +559,22 @@ def _climate_write(payload, rep, href=None): if kind == "mode": return (["mode", "vs", "0"], {"x.com.samsung.da.modes": [value]}) if kind == "temperature_ocf": - return (["temperature", "desired", "0"], {"temperature": round(float(value))}) + quantized = _quantize_temperature(value, _temperature_step(resources)) + if quantized is None: + return None + return (["temperature", "desired", "0"], {"temperature": quantized}) if kind == "temperature": # Vendor items[] array; only one item observed on every AC dump, id '0'. + quantized = _quantize_temperature(value, _temperature_step(resources)) + if quantized is None: + return None return ( ["temperatures", "vs", "0"], { "x.com.samsung.da.items": [ { "x.com.samsung.da.id": "0", - "x.com.samsung.da.desired": str(round(float(value))), + "x.com.samsung.da.desired": str(quantized), } ] }, diff --git a/tests/test_airconditioner_capabilities.py b/tests/test_airconditioner_capabilities.py index fd9b8c3..92a0ca1 100644 --- a/tests/test_airconditioner_capabilities.py +++ b/tests/test_airconditioner_capabilities.py @@ -152,6 +152,59 @@ def test_climate_write_targets(): assert write(("bogus", 1), {}) is None +def test_climate_write_preserves_half_degree_temperature_steps(): + """CAC/TP1X FAC boards advertise `0.5` on both temperature resources. + The increment on /temperatures/vs/0 lives inside its items[] entry (the + same shape every fixture in the corpus uses), not at the resource's top + level -- a fabricated flat `{"/temperatures/vs/0": {"increment": ...}}` + resource would pass a step-reading bug like that silently.""" + climate_desc = next(e for e in airconditioner.CLIMATE.entities if isinstance(e, ClimateDesc)) + write = climate_desc.write_fn + resources = _load_device("airconditioner_cac") + assert write(("temperature_ocf", 24.5), {}, None, resources) == ( + ["temperature", "desired", "0"], + {"temperature": 24.5}, + ) + assert write(("temperature", 24.5), {}, None, resources) == ( + ["temperatures", "vs", "0"], + { + "x.com.samsung.da.items": [ + {"x.com.samsung.da.id": "0", "x.com.samsung.da.desired": "24.5"} + ] + }, + ) + + +def test_climate_write_rounds_to_whole_degree_with_no_advertised_increment(): + """ARTIK051 boards have neither /temperature/control/vs/0 nor an + increment field on /temperatures/vs/0's item -- target_temperature_step + (climate.py) defaults to 1.0 there, so the write path must match rather + than pass the raw value through unrounded.""" + climate_desc = next(e for e in airconditioner.CLIMATE.entities if isinstance(e, ClimateDesc)) + write = climate_desc.write_fn + resources = _load_device("airconditioner_artik051_krac_18k") + assert airconditioner._temperature_step(resources) is None + assert write(("temperature", 23.6), {}, None, resources) == ( + ["temperatures", "vs", "0"], + { + "x.com.samsung.da.items": [ + {"x.com.samsung.da.id": "0", "x.com.samsung.da.desired": "24"} + ] + }, + ) + + +def test_climate_write_rejects_non_numeric_temperature(): + """A non-numeric payload must reject the write (return None) rather than + build a body with `{"temperature": None}` -- coordinator.py's + async_send_command logs and drops a write_fn result of None instead of + POSTing it.""" + climate_desc = next(e for e in airconditioner.CLIMATE.entities if isinstance(e, ClimateDesc)) + write = climate_desc.write_fn + assert write(("temperature_ocf", "not-a-number"), {}) is None + assert write(("temperature", None), {}) is None + + def test_climate_consumed_hrefs_declared_as_coverage(): """The climate-consumed and ambiguous hrefs are declared in the AC registry (as no-entity coverage caps) so they don't leak as gaps -- but produce no