From cdd892a5267ab4e1a9437258d42de73422e5f7a6 Mon Sep 17 00:00:00 2001 From: Keith Burzinski Date: Wed, 26 Aug 2026 22:16:25 -0500 Subject: [PATCH 1/6] [provisioning] Shut down the Wi-Fi AP and captive portal when the window closes (#17466) Co-authored-by: J. Nick Koston --- .../captive_portal/captive_portal.cpp | 17 ++++++++++++ esphome/components/provisioning/__init__.py | 20 ++++++++++++++ esphome/components/wifi/__init__.py | 9 +++++-- esphome/components/wifi/wifi_component.cpp | 25 ++++++++++++++++- .../provisioning/test_provisioning.py | 27 +++++++++++++++++++ .../provisioning/test.esp32-idf.yaml | 9 +++++-- .../provisioning/test.esp8266-ard.yaml | 7 ++++- 7 files changed, 108 insertions(+), 6 deletions(-) diff --git a/esphome/components/captive_portal/captive_portal.cpp b/esphome/components/captive_portal/captive_portal.cpp index e80f9e669f..599422a46b 100644 --- a/esphome/components/captive_portal/captive_portal.cpp +++ b/esphome/components/captive_portal/captive_portal.cpp @@ -6,6 +6,9 @@ #include "esphome/core/string_ref.h" #include "esphome/components/wifi/scan_list.h" #include "esphome/components/wifi/wifi_component.h" +#ifdef USE_PROVISIONING +#include "esphome/components/provisioning/provisioning.h" +#endif #include "captive_index.h" namespace esphome::captive_portal { @@ -78,6 +81,20 @@ void CaptivePortal::handle_wifisave(AsyncWebServerRequest *request) { void CaptivePortal::setup() { // Disable loop by default - will be enabled when captive portal starts this->disable_loop(); +#ifdef USE_PROVISIONING + // The captive portal is a provisioning surface: once the provisioning window + // has closed, stop serving it. WiFi's own closed-callback shuts down the + // access point the portal runs on, and the gated fallback in WiFiComponent's + // loop() ensures neither is started again afterwards. + if (provisioning::global_provisioning_manager != nullptr) { + provisioning::global_provisioning_manager->add_on_closed_callback([this]() { + if (this->active_) { + ESP_LOGD(TAG, "Provisioning window closed; stopping captive portal"); + this->end(); + } + }); + } +#endif } void CaptivePortal::start() { this->base_->init(); diff --git a/esphome/components/provisioning/__init__.py b/esphome/components/provisioning/__init__.py index 9462bbb3b7..63faaf8eae 100644 --- a/esphome/components/provisioning/__init__.py +++ b/esphome/components/provisioning/__init__.py @@ -23,6 +23,8 @@ class ProvisioningData: sources: set[str] = field(default_factory=set) # Names of source components that have their credentials set in the config. hardcoded_credentials: set[str] = field(default_factory=set) + # True when WiFi is configured with an access point but no station credentials. + ap_without_sta: bool = False def _get_data() -> ProvisioningData: @@ -56,6 +58,17 @@ def report_hardcoded_credentials(name: str) -> None: _get_data().hardcoded_credentials.add(name) +def report_ap_without_sta() -> None: + """Record that WiFi runs an access point with no station credentials. + + The access point (and captive portal) shut down when the provisioning window + closes. On a device where that access point is the only network connection, + closing the window makes the device unreachable until it is power-cycled, so + `provisioning:` warns about this combination. + """ + _get_data().ap_without_sta = True + + CONFIG_SCHEMA = cv.Schema( { cv.GenerateID(): cv.declare_id(ProvisioningManager), @@ -89,6 +102,13 @@ def _final_validate(config: ConfigType) -> None: "hardcoding them makes the window pointless.", ", ".join(sorted(data.hardcoded_credentials)), ) + if data.ap_without_sta: + _LOGGER.warning( + "'provisioning' is configured with a WiFi access point and no station " + "credentials. The access point shuts down when the provisioning window " + "closes, so if it is the device's only network connection, the device " + "will be unreachable until it is power-cycled." + ) FINAL_VALIDATE_SCHEMA = _final_validate diff --git a/esphome/components/wifi/__init__.py b/esphome/components/wifi/__init__.py index 4bb6629da1..b8c6d774ac 100644 --- a/esphome/components/wifi/__init__.py +++ b/esphome/components/wifi/__init__.py @@ -445,10 +445,15 @@ def _report_provisioning_credentials(config): about this, since a device that uses a provisioning window should get its credentials on first connection instead. """ - if config.get(CONF_NETWORKS): - from esphome.components import provisioning + from esphome.components import provisioning + if config.get(CONF_NETWORKS): provisioning.report_hardcoded_credentials("wifi") + elif CONF_AP in config: + # An access point with no station credentials: the AP shuts down when the + # provisioning window closes, so `provisioning:` warns that the device may + # become unreachable until power-cycled. + provisioning.report_ap_without_sta() return config diff --git a/esphome/components/wifi/wifi_component.cpp b/esphome/components/wifi/wifi_component.cpp index d82929e5cb..82755f39f7 100644 --- a/esphome/components/wifi/wifi_component.cpp +++ b/esphome/components/wifi/wifi_component.cpp @@ -633,6 +633,21 @@ void WiFiComponent::setup() { this->configured_power_save_ = this->power_save_; #endif +#if defined(USE_PROVISIONING) && defined(USE_WIFI_AP) + // The access point is a provisioning surface: once the provisioning window has + // closed, shut it down (mirrors the teardown done on a successful connection). + // The captive portal registers its own closed-callback, and the fallback block + // in loop() is gated so neither is started again afterwards. + if (provisioning::global_provisioning_manager != nullptr) { + provisioning::global_provisioning_manager->add_on_closed_callback([this]() { + if (this->ap_setup_) { + ESP_LOGD(TAG, "Provisioning window closed; disabling AP"); + this->wifi_mode_({}, false); + } + }); + } +#endif + if (this->enable_on_boot_) { #ifdef USE_ESP32 this->wifi_lazy_init_(); @@ -854,7 +869,15 @@ void WiFiComponent::loop() { } #ifdef USE_WIFI_AP - if (this->has_ap() && !this->ap_setup_) { + bool provisioning_closed = false; +#ifdef USE_PROVISIONING + // Once the provisioning window has closed, don't bring up the fallback AP (or + // the captive portal on it) - the device must stay unprovisionable until it is + // power-cycled. + provisioning_closed = + provisioning::global_provisioning_manager != nullptr && provisioning::global_provisioning_manager->closed(); +#endif + if (this->has_ap() && !this->ap_setup_ && !provisioning_closed) { if (this->ap_timeout_ != 0 && (now - this->last_connected_ > this->ap_timeout_)) { ESP_LOGI(TAG, "Starting fallback AP"); this->setup_ap_config_(); diff --git a/tests/component_tests/provisioning/test_provisioning.py b/tests/component_tests/provisioning/test_provisioning.py index d3a3771bbc..83c31aeca6 100644 --- a/tests/component_tests/provisioning/test_provisioning.py +++ b/tests/component_tests/provisioning/test_provisioning.py @@ -11,6 +11,7 @@ from esphome.components.provisioning import ( CONFIG_SCHEMA, FINAL_VALIDATE_SCHEMA, register_source, + report_ap_without_sta, report_hardcoded_credentials, ) from esphome.const import CONF_TIMEOUT, PlatformFramework @@ -66,6 +67,32 @@ def test_provisioning_no_warning_without_hardcoded_credentials( assert "credentials" not in caplog.text +def test_provisioning_warns_on_ap_without_sta( + set_core_config: SetCoreConfigCallable, + caplog: pytest.LogCaptureFixture, +) -> None: + """An access point with no station credentials triggers a reachability warning.""" + set_core_config(PlatformFramework.ESP32_IDF) + register_source("network") + report_ap_without_sta() + with caplog.at_level(logging.WARNING): + FINAL_VALIDATE_SCHEMA({}) + assert "access point" in caplog.text + assert "unreachable" in caplog.text + + +def test_provisioning_no_warning_without_ap( + set_core_config: SetCoreConfigCallable, + caplog: pytest.LogCaptureFixture, +) -> None: + """No reachability warning when no AP-without-station setup is reported.""" + set_core_config(PlatformFramework.ESP32_IDF) + register_source("network") + with caplog.at_level(logging.WARNING): + FINAL_VALIDATE_SCHEMA({}) + assert "access point" not in caplog.text + + def test_provisioning_rejects_zero_timeout( set_core_config: SetCoreConfigCallable, ) -> None: diff --git a/tests/components/provisioning/test.esp32-idf.yaml b/tests/components/provisioning/test.esp32-idf.yaml index 24168881fc..baa3aa8f68 100644 --- a/tests/components/provisioning/test.esp32-idf.yaml +++ b/tests/components/provisioning/test.esp32-idf.yaml @@ -1,6 +1,7 @@ # Exercises the provisioning window: api registers as a provisioning source -# (encryption enabled, no key), the on_timeout automation, and the wifi + -# esp32_improv cross-component guards. improv_serial is intentionally NOT gated. +# (encryption enabled, no key), the on_timeout automation, and the wifi (AP + +# captive portal) and esp32_improv cross-component guards. improv_serial is +# intentionally NOT gated. provisioning: timeout: 1min on_timeout: @@ -13,6 +14,10 @@ api: wifi: ssid: MySSID password: password1 + ap: + ssid: MyAP + +captive_portal: improv_serial: diff --git a/tests/components/provisioning/test.esp8266-ard.yaml b/tests/components/provisioning/test.esp8266-ard.yaml index 4188c00bef..2666477658 100644 --- a/tests/components/provisioning/test.esp8266-ard.yaml +++ b/tests/components/provisioning/test.esp8266-ard.yaml @@ -1,5 +1,6 @@ # Provisioning window on ESP8266 (no BLE Improv): api as a provisioning source -# and the wifi reboot guard. improv_serial is present and intentionally NOT gated. +# and the wifi (AP + captive portal) guards. improv_serial is present and +# intentionally NOT gated. provisioning: timeout: 1min on_timeout: @@ -12,5 +13,9 @@ api: wifi: ssid: MySSID password: password1 + ap: + ssid: MyAP + +captive_portal: improv_serial: From 20b38ac8b0c14f2cc3a9fbca9c4ffdd198ac800b Mon Sep 17 00:00:00 2001 From: guillempages Date: Thu, 27 Aug 2026 06:37:20 +0200 Subject: [PATCH 2/6] [runtime_image] Add support for QOI images (#16945) --- esphome/components/runtime_image/__init__.py | 17 +- .../components/runtime_image/image_format.cpp | 3 + .../components/runtime_image/image_format.h | 6 +- .../components/runtime_image/qoi_decoder.cpp | 174 ++++++++++++++++++ .../components/runtime_image/qoi_decoder.h | 49 +++++ .../runtime_image/runtime_image.cpp | 7 + esphome/core/defines.h | 3 +- tests/components/online_image/common.yaml | 6 + tests/components/runtime_image/__init__.py | 3 +- .../runtime_image/test_decoder_reuse.cpp | 69 ++++++- .../runtime_image/test_mime_types.cpp | 12 +- 11 files changed, 338 insertions(+), 11 deletions(-) create mode 100644 esphome/components/runtime_image/qoi_decoder.cpp create mode 100644 esphome/components/runtime_image/qoi_decoder.h diff --git a/esphome/components/runtime_image/__init__.py b/esphome/components/runtime_image/__init__.py index a220503045..e27fcadbee 100644 --- a/esphome/components/runtime_image/__init__.py +++ b/esphome/components/runtime_image/__init__.py @@ -28,6 +28,7 @@ ImageDecoder = runtime_image_ns.class_("ImageDecoder") BmpDecoder = runtime_image_ns.class_("BmpDecoder", ImageDecoder) JpegDecoder = runtime_image_ns.class_("JpegDecoder", ImageDecoder) PngDecoder = runtime_image_ns.class_("PngDecoder", ImageDecoder) +QoiDecoder = runtime_image_ns.class_("QoiDecoder", ImageDecoder) # Runtime image class RuntimeImage = runtime_image_ns.class_( @@ -37,9 +38,10 @@ RuntimeImage = runtime_image_ns.class_( # Image format enum ImageFormat = runtime_image_ns.enum("ImageFormat") IMAGE_FORMAT_AUTO = ImageFormat.AUTO +IMAGE_FORMAT_BMP = ImageFormat.BMP IMAGE_FORMAT_JPEG = ImageFormat.JPEG IMAGE_FORMAT_PNG = ImageFormat.PNG -IMAGE_FORMAT_BMP = ImageFormat.BMP +IMAGE_FORMAT_QOI = ImageFormat.QOI # Export enum for decode errors DecodeError = runtime_image_ns.enum("DecodeError") @@ -115,14 +117,27 @@ class PNGFormat(Format): cg.add_library("pngle", "1.1.0") +class QOIFormat(Format): + """QOI format decoder configuration.""" + + def __init__(self): + super().__init__("QOI", QoiDecoder) + + def actions(self) -> None: + cg.add_define("USE_RUNTIME_IMAGE_QOI") + + # Decodable formats only; platforms that support runtime detection accept # "AUTO" in their own schema and get_format() resolves it _JPEG_FORMAT = JPEGFormat() + +# Registry of available formats IMAGE_FORMATS = { "BMP": BMPFormat(), "JPEG": _JPEG_FORMAT, "JPG": _JPEG_FORMAT, # Alias for JPEG "PNG": PNGFormat(), + "QOI": QOIFormat(), } FILTER_SOURCE_FILES = filter_source_files_from_defines( diff --git a/esphome/components/runtime_image/image_format.cpp b/esphome/components/runtime_image/image_format.cpp index 9575b30887..9db8490415 100644 --- a/esphome/components/runtime_image/image_format.cpp +++ b/esphome/components/runtime_image/image_format.cpp @@ -21,6 +21,9 @@ static constexpr MimeLookup MIME_LOOKUP_TABLE[] = { #ifdef USE_RUNTIME_IMAGE_PNG {"image/png", ImageFormat::PNG}, {"image/x-png", ImageFormat::PNG}, #endif +#ifdef USE_RUNTIME_IMAGE_QOI + {"image/qoi", ImageFormat::QOI}, {"image/x-qoi", ImageFormat::QOI}, +#endif }; const char *get_mime_type_for_format(ImageFormat format) { diff --git a/esphome/components/runtime_image/image_format.h b/esphome/components/runtime_image/image_format.h index aff0c026b9..72bd81a06e 100644 --- a/esphome/components/runtime_image/image_format.h +++ b/esphome/components/runtime_image/image_format.h @@ -11,12 +11,14 @@ enum ImageFormat { /** Format is supplied per decode, e.g. detected from the Content-Type header * by online_image; sniffing the image data is not implemented. */ AUTO, + /** BMP format. */ + BMP, /** JPEG format. */ JPEG, /** PNG format. */ PNG, - /** BMP format. */ - BMP, + /** QOI format. */ + QOI, }; /// Canonical MIME type for a format; "image/*" for AUTO/unknown diff --git a/esphome/components/runtime_image/qoi_decoder.cpp b/esphome/components/runtime_image/qoi_decoder.cpp new file mode 100644 index 0000000000..b0fabb5233 --- /dev/null +++ b/esphome/components/runtime_image/qoi_decoder.cpp @@ -0,0 +1,174 @@ +#include "qoi_decoder.h" + +#ifdef USE_RUNTIME_IMAGE_QOI + +#include "esphome/components/display/display.h" +#include "esphome/core/helpers.h" +#include "esphome/core/log.h" + +#include + +namespace esphome::runtime_image { + +static const char *const TAG = "image_decoder.qoi"; + +constexpr uint8_t QOI_OP_RGB = 0b11111110; +constexpr uint8_t QOI_OP_RGBA = 0b11111111; +constexpr uint8_t QOI_OP_INDEX = 0b00000000; // 00xxxxxx +constexpr uint8_t QOI_OP_DIFF = 0b01000000; // 01xxxxxx +constexpr uint8_t QOI_OP_LUMA = 0b10000000; // 10xxxxxx +constexpr uint8_t QOI_OP_RUN = 0b11000000; // 11xxxxxx + +constexpr uint8_t QOI_RGB_CHUNK_SIZE = 4; +constexpr uint8_t QOI_RGBA_CHUNK_SIZE = 5; +constexpr uint8_t QOI_LUMA_CHUNK_SIZE = 2; + +constexpr uint8_t QOI_MASK_OP = 0b11000000; +constexpr uint8_t QOI_MASK_VALUE = 0b00111111; + +constexpr size_t QOI_HEADER_SIZE = 14; +constexpr size_t QOI_COLOR_TABLE_SIZE = 64; + +inline size_t qoi_color_table_index(const Color &color) { + // QOI color hash function: (r * 3 + g * 5 + b * 7 + a * 11) % 64 + return (color.r * 3 + color.g * 5 + color.b * 7 + color.w * 11) & + 63; // modulo 64 is equivalent to bitwise AND with 63 (0b00111111) +} + +void QoiDecoder::reset() { + ImageDecoder::reset(); + this->current_index_ = 0; + this->paint_index_ = 0; + this->width_ = 0; + this->height_ = 0; + this->bits_per_pixel_ = 0; + this->last_pixel_ = Color(0, 0, 0, 255); + if (this->color_table_) { + std::fill_n(this->color_table_.get(), QOI_COLOR_TABLE_SIZE, Color()); + } +} + +int HOT QoiDecoder::decode(uint8_t *buffer, size_t size) { + size_t index = 0; + if (this->current_index_ == 0) { + if (size < QOI_HEADER_SIZE) { + return 0; // Need more data for file header + } + + /** QOI Header definition, for reference: + char magic[4]; // magic bytes "qoif" + uint32_t width; // image width in pixels (BE) + uint32_t height; // image height in pixels (BE) + uint8_t channels; // 3 = RGB, 4 = RGBA + uint8_t colorspace; // 0 = sRGB with linear alpha, 1 = all channels linear + */ + // Check if the file is a QOI image + if (buffer[0] != 'q' || buffer[1] != 'o' || buffer[2] != 'i' || buffer[3] != 'f') { + ESP_LOGE(TAG, "Not a QOI file"); + return DECODE_ERROR_INVALID_TYPE; + } + + this->width_ = encode_uint32(buffer[4], buffer[5], buffer[6], buffer[7]); + this->height_ = encode_uint32(buffer[8], buffer[9], buffer[10], buffer[11]); + if (this->width_ == 0 || this->height_ == 0) { + ESP_LOGE(TAG, "Invalid image dimensions: (%zux%zu)", this->width_, this->height_); + return DECODE_ERROR_INVALID_TYPE; + } + uint8_t channels = buffer[12]; + if (channels < 3 || channels > 4) { + ESP_LOGE(TAG, "Unsupported number of channels: %d", channels); + return DECODE_ERROR_UNSUPPORTED_FORMAT; + } + this->bits_per_pixel_ = channels * 8; + uint8_t colorspace = buffer[13]; + if (colorspace > 1) { + ESP_LOGE(TAG, "Unsupported colorspace value: %d", colorspace); + return DECODE_ERROR_UNSUPPORTED_FORMAT; + } + ESP_LOGD(TAG, "QOI image header: width=%zu, height=%zu, channels=%d, colorspace=%d", this->width_, this->height_, + channels, colorspace); + + if (!this->color_table_) { + this->color_table_ = std::make_unique(QOI_COLOR_TABLE_SIZE); + } + + if (!this->set_size(this->width_, this->height_)) { + return DECODE_ERROR_OUT_OF_MEMORY; + } + + this->current_index_ = QOI_HEADER_SIZE; + index = QOI_HEADER_SIZE; + } // Current_index == 0 + + Color color; + const size_t total_pixels = this->width_ * this->height_; + while (index < size && this->paint_index_ < total_pixels) { + color = this->last_pixel_; + uint8_t byte = buffer[index]; + if (byte == QOI_OP_RGB) { + if (size < index + QOI_RGB_CHUNK_SIZE) { + return index; // Need more data for RGB chunk + } + index++; + color.r = buffer[index++]; + color.g = buffer[index++]; + color.b = buffer[index++]; + this->draw(this->paint_index_ % this->width_, this->paint_index_ / this->width_, 1, 1, color); + this->paint_index_++; + } else if (byte == QOI_OP_RGBA) { + if (size < index + QOI_RGBA_CHUNK_SIZE) { + return index; // Need more data for RGBA chunk + } + index++; + color.r = buffer[index++]; + color.g = buffer[index++]; + color.b = buffer[index++]; + color.w = buffer[index++]; + this->draw(this->paint_index_ % this->width_, this->paint_index_ / this->width_, 1, 1, color); + this->paint_index_++; + } else if ((byte & QOI_MASK_OP) == QOI_OP_RUN) { + // QOI run chunk + size_t run_length = (byte & QOI_MASK_VALUE) + 1; // run length is encoded in the lower 6 bits, plus one + for (size_t i = 0; i < run_length; i++) { + // TODO: optimize by drawing runs of pixels at once instead of one by one + this->draw(this->paint_index_ % this->width_, this->paint_index_ / this->width_, 1, 1, color); + this->paint_index_++; + } + index++; + } else if ((byte & QOI_MASK_OP) == QOI_OP_LUMA) { + if (size < index + QOI_LUMA_CHUNK_SIZE) { + return index; // Need more data for LUMA chunk + } + index++; + uint8_t byte2 = buffer[index++]; + uint8_t delta_g = (byte & QOI_MASK_VALUE) - 32; + color.r += delta_g - 8 + ((byte2 >> 4) & 0x0f); + color.g += delta_g; + color.b += delta_g - 8 + (byte2 & 0x0f); + + this->draw(this->paint_index_ % this->width_, this->paint_index_ / this->width_, 1, 1, color); + this->paint_index_++; + } else if ((byte & QOI_MASK_OP) == QOI_OP_DIFF) { + color.r += ((byte >> 4) & 0x03) - 2; + color.g += ((byte >> 2) & 0x03) - 2; + color.b += (byte & 0x03) - 2; + + this->draw(this->paint_index_ % this->width_, this->paint_index_ / this->width_, 1, 1, color); + this->paint_index_++; + index++; + } else if ((byte & QOI_MASK_OP) == QOI_OP_INDEX) { + color = this->color_table_[byte]; + this->draw(this->paint_index_ % this->width_, this->paint_index_ / this->width_, 1, 1, color); + this->paint_index_++; + index++; + } + this->last_pixel_ = color; + this->color_table_[qoi_color_table_index(color)] = color; + } + this->decoded_bytes_ += size; + return size; +} + +} // namespace esphome::runtime_image + +#endif // USE_RUNTIME_IMAGE_QOI diff --git a/esphome/components/runtime_image/qoi_decoder.h b/esphome/components/runtime_image/qoi_decoder.h new file mode 100644 index 0000000000..c44afd83cf --- /dev/null +++ b/esphome/components/runtime_image/qoi_decoder.h @@ -0,0 +1,49 @@ +#pragma once + +#include "esphome/core/defines.h" +#ifdef USE_RUNTIME_IMAGE_QOI + +#include + +#include "image_decoder.h" +#include "runtime_image.h" + +namespace esphome::runtime_image { + +/** + * @brief Image decoder specialization for QOI images. + */ +class QoiDecoder : public ImageDecoder { + public: + /** + * @brief Construct a new QOI decoder object. + * + * @param image The RuntimeImage to decode the stream into. + */ + QoiDecoder(RuntimeImage *image) : ImageDecoder(image, QOI) {} + + void reset() override; + int HOT decode(uint8_t *buffer, size_t size) override; + + bool is_finished() const override { + if (this->bits_per_pixel_ == 0) { + // header not yet received, so dimensions not yet determined + return false; + } + // QOI is finished when we've decoded all pixel data + return this->paint_index_ >= static_cast(this->width_ * this->height_); + } + + protected: + std::unique_ptr color_table_; + size_t current_index_{0}; + size_t paint_index_{0}; + size_t width_{0}; + size_t height_{0}; + Color last_pixel_{0, 0, 0, 255}; // QOI spec defines initial previous pixel as opaque black + uint16_t bits_per_pixel_{0}; +}; + +} // namespace esphome::runtime_image + +#endif // USE_RUNTIME_IMAGE_QOI diff --git a/esphome/components/runtime_image/runtime_image.cpp b/esphome/components/runtime_image/runtime_image.cpp index f7417c2c8e..ef92d0d707 100644 --- a/esphome/components/runtime_image/runtime_image.cpp +++ b/esphome/components/runtime_image/runtime_image.cpp @@ -15,6 +15,9 @@ #ifdef USE_RUNTIME_IMAGE_PNG #include "png_decoder.h" #endif +#ifdef USE_RUNTIME_IMAGE_QOI +#include "qoi_decoder.h" +#endif namespace esphome::runtime_image { @@ -367,6 +370,10 @@ std::unique_ptr RuntimeImage::create_decoder_(ImageFormat format) #ifdef USE_RUNTIME_IMAGE_PNG case PNG: return make_unique(this); +#endif +#ifdef USE_RUNTIME_IMAGE_QOI + case QOI: + return make_unique(this); #endif case AUTO: ESP_LOGE(TAG, "Image format could not be determined; set `format:` explicitly in the configuration"); diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 5b73c43ccd..993b9dce75 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -233,8 +233,9 @@ #endif #define USE_RTTTL_FINISHED_PLAYBACK_CALLBACK #define USE_RUNTIME_IMAGE_BMP -#define USE_RUNTIME_IMAGE_PNG #define USE_RUNTIME_IMAGE_JPEG +#define USE_RUNTIME_IMAGE_PNG +#define USE_RUNTIME_IMAGE_QOI #define USE_RUNTIME_STATS #define USE_OTA #define USE_OTA_PASSWORD diff --git a/tests/components/online_image/common.yaml b/tests/components/online_image/common.yaml index 85901287c7..d8d04850cf 100644 --- a/tests/components/online_image/common.yaml +++ b/tests/components/online_image/common.yaml @@ -62,6 +62,12 @@ image: url: http://www.faqs.org/images/library.jpg format: AUTO type: RGB565 + - platform: online_image + id: online_qoi_image + url: https://www.example.org/image.qoi + format: QOI + type: RGB + transparency: alpha_channel # Check the set_url action esphome: diff --git a/tests/components/runtime_image/__init__.py b/tests/components/runtime_image/__init__.py index a8ff4bb68e..041db39128 100644 --- a/tests/components/runtime_image/__init__.py +++ b/tests/components/runtime_image/__init__.py @@ -9,7 +9,8 @@ def override_manifest(manifest: ComponentManifestOverride) -> None: # tests have two decoder types and every retained decoder is under test. async def to_code_testing(config: ConfigType) -> None: enable_format("BMP") - enable_format("PNG") enable_format("JPEG") + enable_format("PNG") + enable_format("QOI") manifest.to_code = to_code_testing diff --git a/tests/components/runtime_image/test_decoder_reuse.cpp b/tests/components/runtime_image/test_decoder_reuse.cpp index 9c2d00b747..e62e2a98d9 100644 --- a/tests/components/runtime_image/test_decoder_reuse.cpp +++ b/tests/components/runtime_image/test_decoder_reuse.cpp @@ -70,11 +70,36 @@ static const uint8_t PNG_RGB_EXPECTED[4][4][3] = { {{0x12, 0x34, 0x56}, {0x65, 0x43, 0x21}, {0xFE, 0xDC, 0xBA}, {0xAB, 0xCD, 0xEF}}, }; +// 3x3 QOI, exercising all possible chunk types +static const uint8_t QOI_RGBA[] = { + 0x71, 0x6F, 0x69, 0x66, // Header: 'qoif' + 0x00, 0x00, 0x00, 0x03, // Width: 3 + 0x00, 0x00, 0x00, 0x03, // Height: 3 + 0x04, // Channels: 4 (RGBA) + 0x00, // Colorspace: 0 (SRGB) + 0xC1, // 1. QOI_OP_RUN + 0x79, // 2. QOI_OP_DIFF + 0xAA, 0x79, // 3. QOI_OP_LUMA + 0xFE, 0xC8, 0x64, 0x32, // 4. QOI_OP_RGB + 0xFF, 0x78, 0x50, 0x28, + 0x64, // 5. QOI_OP_RGBA + 0x31, // 6. QOI_OP_INDEX + 0xC1, // 7. QOI_OP_RUN + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 // End Marker +}; + +static const uint8_t QOI_EXPECTED_RGBA[3][3][4] = { + {{0x00, 0x00, 0x00, 0xFF}, {0x00, 0x00, 0x00, 0xFF}, {0x01, 0x00, 0xFF, 0xFF}}, + {{0x0A, 0x0A, 0x0A, 0xFF}, {0xC8, 0x64, 0x32, 0xFF}, {0x78, 0x50, 0x28, 0x64}}, + {{0x01, 0x00, 0xFF, 0xFF}, {0x01, 0x00, 0xFF, 0xFF}, {0x01, 0x00, 0xFF, 0xFF}} + +}; + /// Exposes the protected decoder machinery so reuse and eviction can be observed directly. class TestableRuntimeImage : public RuntimeImage { public: - explicit TestableRuntimeImage(ImageFormat format) - : RuntimeImage(format, image::IMAGE_TYPE_RGB, image::TRANSPARENCY_OPAQUE, nullptr, false, 0, 0) {} + explicit TestableRuntimeImage(ImageFormat format, image::Transparency transparency = image::TRANSPARENCY_OPAQUE) + : RuntimeImage(format, image::IMAGE_TYPE_RGB, transparency, nullptr, false, 0, 0) {} ImageDecoder *decoder() { return this->decoder_.get(); } }; @@ -132,6 +157,19 @@ template static void expect_pixels(TestableRuntimeImage &img } } +template +static void expect_pixels_rgba(TestableRuntimeImage &img, const uint8_t (&expected)[H][W][4]) { + ASSERT_EQ(img.get_width(), static_cast(W)); + ASSERT_EQ(img.get_height(), static_cast(H)); + for (size_t y = 0; y < H; y++) { + for (size_t x = 0; x < W; x++) { + SCOPED_TRACE(::testing::Message() << "pixel (" << x << "," << y << ")"); + Color color = img.get_pixel(x, y); + EXPECT_THAT((std::array{color.r, color.g, color.b, color.w}), + ::testing::ElementsAreArray(expected[y][x])); + } + } +} TEST(RuntimeImageDecoder, DecoderStaysWarmAcrossDecodes) { TestableRuntimeImage img(BMP); @@ -337,6 +375,33 @@ TEST(RuntimeImageDecoder, JpegDecoderStaysWarmAcrossDecodes) { } #endif // USE_RUNTIME_IMAGE_JPEG +TEST(RuntimeImageDecoder, QoiDecoderStaysWarmAcrossDecodes) { + TestableRuntimeImage img(QOI, image::TRANSPARENCY_ALPHA_CHANNEL); + + ASSERT_TRUE(decode_all(img, QOI_RGBA, sizeof(QOI_RGBA))); + expect_pixels_rgba(img, QOI_EXPECTED_RGBA); + ImageDecoder *first = img.decoder(); + ASSERT_NE(first, nullptr); + + ASSERT_TRUE(decode_all(img, QOI_RGBA, sizeof(QOI_RGBA))); + expect_pixels_rgba(img, QOI_EXPECTED_RGBA); + EXPECT_EQ(img.decoder(), first) << "decoder must be reused, not reallocated"; +} + +TEST(RuntimeImageDecoder, QoiChunkedFeedDecodesLikeDownloadLoop) { + TestableRuntimeImage img(QOI, image::TRANSPARENCY_ALPHA_CHANNEL); + + ASSERT_TRUE(decode_chunked(img, QOI_RGBA, sizeof(QOI_RGBA), 10)); + expect_pixels_rgba(img, QOI_EXPECTED_RGBA); + ImageDecoder *first = img.decoder(); + + // Chunked again on the warm decoder: the cross-call resume state + // (current_index_ / paint_index_) must have been fully reset. + ASSERT_TRUE(decode_chunked(img, QOI_RGBA, sizeof(QOI_RGBA), 10)); + expect_pixels_rgba(img, QOI_EXPECTED_RGBA); + EXPECT_EQ(img.decoder(), first); +} + TEST(RuntimeImageDecoder, SessionFlagsTrackLifecycle) { TestableRuntimeImage img(BMP); std::vector buffer(BMP_24BPP, BMP_24BPP + sizeof(BMP_24BPP)); diff --git a/tests/components/runtime_image/test_mime_types.cpp b/tests/components/runtime_image/test_mime_types.cpp index de8bbc76be..22b825cff0 100644 --- a/tests/components/runtime_image/test_mime_types.cpp +++ b/tests/components/runtime_image/test_mime_types.cpp @@ -10,12 +10,14 @@ TEST(RuntimeImageMime, FormatForKnownMimeTypes) { EXPECT_EQ(get_format_for_mime_type("image/bmp"), BMP); EXPECT_EQ(get_format_for_mime_type("image/x-ms-bmp"), BMP); EXPECT_EQ(get_format_for_mime_type("image/x-bmp"), BMP); - EXPECT_EQ(get_format_for_mime_type("image/png"), PNG); - EXPECT_EQ(get_format_for_mime_type("image/x-png"), PNG); #ifdef USE_RUNTIME_IMAGE_JPEG EXPECT_EQ(get_format_for_mime_type("image/jpeg"), JPEG); EXPECT_EQ(get_format_for_mime_type("image/jpg"), JPEG); #endif // USE_RUNTIME_IMAGE_JPEG + EXPECT_EQ(get_format_for_mime_type("image/png"), PNG); + EXPECT_EQ(get_format_for_mime_type("image/x-png"), PNG); + EXPECT_EQ(get_format_for_mime_type("image/qoi"), QOI); + EXPECT_EQ(get_format_for_mime_type("image/x-qoi"), QOI); } TEST(RuntimeImageMime, FormatMatchingIsCaseInsensitive) { @@ -39,20 +41,22 @@ TEST(RuntimeImageMime, UnknownMimeTypeHasNoFormat) { TEST(RuntimeImageMime, MimeTypeForFormatRoundTrip) { EXPECT_STREQ(get_mime_type_for_format(BMP), "image/bmp"); - EXPECT_STREQ(get_mime_type_for_format(PNG), "image/png"); #ifdef USE_RUNTIME_IMAGE_JPEG EXPECT_STREQ(get_mime_type_for_format(JPEG), "image/jpeg"); #endif // USE_RUNTIME_IMAGE_JPEG + EXPECT_STREQ(get_mime_type_for_format(PNG), "image/png"); + EXPECT_STREQ(get_mime_type_for_format(QOI), "image/qoi"); // AUTO has no single MIME type and falls back to the wildcard EXPECT_STREQ(get_mime_type_for_format(AUTO), "image/*"); // Every decodable format must resolve back to itself through its MIME type for (ImageFormat format : { BMP, - PNG, #ifdef USE_RUNTIME_IMAGE_JPEG JPEG, #endif // USE_RUNTIME_IMAGE_JPEG + PNG, + QOI, }) { EXPECT_EQ(get_format_for_mime_type(get_mime_type_for_format(format)), format) << format; } From fb132b44211509e82c6c85d139f6d009662bce1d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 01:01:35 -0500 Subject: [PATCH 3/6] [improv_serial][esp32_improv] Use buffer based RPC response builder (#18793) --- .../esp32_improv/esp32_improv_component.cpp | 54 +++++---- .../esp32_improv/esp32_improv_component.h | 3 +- .../components/improv_base/improv_base.cpp | 18 +++ esphome/components/improv_base/improv_base.h | 6 + .../improv_serial/improv_serial_component.cpp | 110 ++++++++++++------ .../improv_serial/improv_serial_component.h | 23 +++- esphome/components/network/ip_address.h | 2 + esphome/core/defines.h | 5 +- tests/components/improv_base/benchmark.yaml | 6 + .../improv_base/rpc_response_builder_test.cpp | 102 ++++++++++++++++ .../improv_serial/common-uart0.yaml | 2 + .../fixtures/improv_serial_uart.yaml | 2 + tests/integration/test_improv_serial_uart.py | 15 ++- 13 files changed, 280 insertions(+), 68 deletions(-) create mode 100644 tests/components/improv_base/benchmark.yaml create mode 100644 tests/components/improv_base/rpc_response_builder_test.cpp diff --git a/esphome/components/esp32_improv/esp32_improv_component.cpp b/esphome/components/esp32_improv/esp32_improv_component.cpp index 6e3a4ef526..4756fba637 100644 --- a/esphome/components/esp32_improv/esp32_improv_component.cpp +++ b/esphome/components/esp32_improv/esp32_improv_component.cpp @@ -1,5 +1,7 @@ #include "esp32_improv_component.h" +#include + #include "esphome/components/bytebuffer/bytebuffer.h" #include "esphome/components/esp32_ble/ble.h" #include "esphome/components/esp32_ble_server/ble_2902.h" @@ -19,7 +21,13 @@ using namespace bytebuffer; static const char *const TAG = "esp32_improv.component"; static constexpr size_t IMPROV_MAX_LOG_BYTES = 128; -static const char *const ESPHOME_MY_LINK = "https://my.home-assistant.io/redirect/config_flow_start?domain=esphome"; +static constexpr char ESPHOME_MY_LINK[] = "https://my.home-assistant.io/redirect/config_flow_start?domain=esphome"; +// command + data length + trailing byte +static constexpr size_t RPC_RESPONSE_OVERHEAD = 3; +// Reserves the ESPHOME_MY_LINK entry; a maximal next URL displaces only the +// lower value web server URL +static constexpr size_t MAX_NEXT_URL_LEN = + improv::RPC_RESPONSE_MAX_SIZE - RPC_RESPONSE_OVERHEAD - 1 - sizeof(ESPHOME_MY_LINK); static constexpr uint16_t STOP_ADVERTISING_DELAY = 10000; // Delay (ms) before stopping service to allow BLE clients to read the final state static constexpr uint16_t NAME_ADVERTISING_INTERVAL = 60000; // Advertise name every 60 seconds @@ -285,8 +293,9 @@ void ESP32ImprovComponent::set_error_(improv::Error error) { } } -void ESP32ImprovComponent::send_response_(std::vector &&response) { - this->rpc_response_->set_value(std::move(response)); +void ESP32ImprovComponent::send_response_(std::span response) { + // The BLE characteristic owns its value, so one exact-size copy is required here + this->rpc_response_->set_value(std::vector(response.begin(), response.end())); if (this->state_ != improv::STATE_STOPPED) this->rpc_response_->notify(); } @@ -430,40 +439,35 @@ void ESP32ImprovComponent::check_wifi_connection_() { this->connecting_sta_ = {}; this->cancel_timeout("wifi-connect-timeout"); - // Build URL list with minimal allocations - // Maximum 3 URLs: custom next_url + ESPHOME_MY_LINK + webserver URL - std::string url_strings[3]; - size_t url_count = 0; + // Build the URL list directly into a stack buffer with no heap allocation + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::WIFI_SETTINGS); #ifdef USE_ESP32_IMPROV_NEXT_URL // Add next_url if configured (should be first per Improv BLE spec) - { - char url_buffer[384]; - size_t len = this->get_formatted_next_url_(url_buffer, sizeof(url_buffer)); - if (len > 0) { - url_strings[url_count++] = std::string(url_buffer, len); - } - } + this->add_next_url_(builder, MAX_NEXT_URL_LEN); #endif - // Add default URLs for backward compatibility - url_strings[url_count++] = ESPHOME_MY_LINK; + // Add default URLs for backward compatibility; MAX_NEXT_URL_LEN reserves this + // entry's space, so it always fits + builder.add_string(ESPHOME_MY_LINK, sizeof(ESPHOME_MY_LINK) - 1); #ifdef USE_WEBSERVER for (auto &ip : wifi::global_wifi_component->wifi_sta_ip_addresses()) { if (ip.is_ip4()) { - // "http://" (7) + IPv4 max (15) + ":" (1) + port max (5) + null = 29 - char url_buffer[32]; - memcpy(url_buffer, "http://", 7); // NOLINT(bugprone-not-null-terminated-result) - str_to null-terminates - ip.str_to(url_buffer + 7); - size_t len = strlen(url_buffer); - snprintf(url_buffer + len, sizeof(url_buffer) - len, ":%d", USE_WEBSERVER_PORT); - url_strings[url_count++] = url_buffer; + char ip_buf[network::IP_ADDRESS_BUFFER_SIZE]; + ip.str_to(ip_buf); + // "http://" (7) + IP (40) + ":" (1) + port (5) + null (1) = 54 + char webserver_url[7 + network::IP_ADDRESS_BUFFER_SIZE + 1 + 5 + 1]; + size_t len = + buf_append_printf(webserver_url, sizeof(webserver_url), 0, "http://%s:%u", ip_buf, USE_WEBSERVER_PORT); + if (!builder.add_string(webserver_url, len)) { + ESP_LOGW(TAG, "Response full; URL dropped"); + } break; } } #endif - this->send_response_(improv::build_rpc_response(improv::WIFI_SETTINGS, - std::vector(url_strings, url_strings + url_count))); + this->send_response_(builder.finish()); } else if (this->is_active() && this->state_ != improv::STATE_PROVISIONED) { ESP_LOGD(TAG, "WiFi provisioned externally"); } diff --git a/esphome/components/esp32_improv/esp32_improv_component.h b/esphome/components/esp32_improv/esp32_improv_component.h index d948dba3b3..414948c977 100644 --- a/esphome/components/esp32_improv/esp32_improv_component.h +++ b/esphome/components/esp32_improv/esp32_improv_component.h @@ -22,6 +22,7 @@ #include "esphome/components/output/binary_output.h" #endif +#include #include #ifdef USE_ESP32 @@ -109,7 +110,7 @@ class ESP32ImprovComponent final : public Component, public improv_base::ImprovB void set_state_(improv::State state, bool update_advertising = true); void set_error_(improv::Error error); improv::State get_initial_state_() const; - void send_response_(std::vector &&response); + void send_response_(std::span response); void process_incoming_data_(); void on_wifi_connect_timeout_(); void check_wifi_connection_(); diff --git a/esphome/components/improv_base/improv_base.cpp b/esphome/components/improv_base/improv_base.cpp index fa1b855d6c..1babeb5b5a 100644 --- a/esphome/components/improv_base/improv_base.cpp +++ b/esphome/components/improv_base/improv_base.cpp @@ -4,10 +4,13 @@ #include "esphome/components/network/util.h" #include "esphome/core/application.h" #include "esphome/core/defines.h" +#include "esphome/core/log.h" namespace esphome::improv_base { #if defined(USE_ESP32_IMPROV_NEXT_URL) || defined(USE_IMPROV_SERIAL_NEXT_URL) +static const char *const TAG = "improv_base"; + static constexpr const char DEVICE_NAME_PLACEHOLDER[] = "{{device_name}}"; static constexpr size_t DEVICE_NAME_PLACEHOLDER_LEN = sizeof(DEVICE_NAME_PLACEHOLDER) - 1; static constexpr const char IP_ADDRESS_PLACEHOLDER[] = "{{ip_address}}"; @@ -62,6 +65,21 @@ size_t ImprovBase::get_formatted_next_url_(char *buffer, size_t buffer_size) { *out = '\0'; return out - buffer; } + +void ImprovBase::add_next_url_(improv::RpcResponseBuilder &builder, size_t max_len) { + // The builder rejects strings above 254 bytes, so anything longer than this + // buffer could never be sent anyway + char url_buffer[256]; + size_t len = this->get_formatted_next_url_(url_buffer, sizeof(url_buffer)); + if (len == 0) { + return; + } + // max_len is the transport's budget for this entry; skipping an over-long URL + // here keeps the rest of the response sendable instead of oversizing the frame + if (len > max_len || !builder.add_string(url_buffer, len)) { + ESP_LOGW(TAG, "Next URL too long; skipping"); + } +} #endif } // namespace esphome::improv_base diff --git a/esphome/components/improv_base/improv_base.h b/esphome/components/improv_base/improv_base.h index 9dded85a46..352bb75d5f 100644 --- a/esphome/components/improv_base/improv_base.h +++ b/esphome/components/improv_base/improv_base.h @@ -3,6 +3,10 @@ #include #include "esphome/core/defines.h" +#if defined(USE_ESP32_IMPROV_NEXT_URL) || defined(USE_IMPROV_SERIAL_NEXT_URL) +#include +#endif + namespace esphome::improv_base { class ImprovBase { @@ -15,6 +19,8 @@ class ImprovBase { #if defined(USE_ESP32_IMPROV_NEXT_URL) || defined(USE_IMPROV_SERIAL_NEXT_URL) /// Format next_url_ into buffer, replacing placeholders. Returns length written. size_t get_formatted_next_url_(char *buffer, size_t buffer_size); + /// Append the formatted next_url to the RPC response, warning if it does not fit. + void add_next_url_(improv::RpcResponseBuilder &builder, size_t max_len); const char *next_url_{nullptr}; #endif }; diff --git a/esphome/components/improv_serial/improv_serial_component.cpp b/esphome/components/improv_serial/improv_serial_component.cpp index 9c7745ee0a..0fb18e9b0d 100644 --- a/esphome/components/improv_serial/improv_serial_component.cpp +++ b/esphome/components/improv_serial/improv_serial_component.cpp @@ -9,6 +9,8 @@ #include "esphome/components/logger/logger.h" #include "esphome/components/wifi/scan_list.h" +#include + namespace esphome::improv_serial { static const char *const TAG = "improv_serial"; @@ -61,8 +63,7 @@ void ImprovSerialComponent::loop() { this->cancel_timeout("wifi-connect-timeout"); this->set_state_(improv::STATE_PROVISIONED); - std::vector url = this->build_rpc_settings_response_(improv::WIFI_SETTINGS); - this->send_response_(url); + this->send_settings_response_(improv::WIFI_SETTINGS); } } } @@ -142,16 +143,11 @@ void ImprovSerialComponent::write_data_(const uint8_t *data, const size_t size) #endif } -std::vector ImprovSerialComponent::build_rpc_settings_response_(improv::Command command) { - std::vector urls; +void ImprovSerialComponent::send_settings_response_(improv::Command command) { + std::array buf; + improv::RpcResponseBuilder builder(buf, command); #ifdef USE_IMPROV_SERIAL_NEXT_URL - { - char url_buffer[384]; - size_t len = this->get_formatted_next_url_(url_buffer, sizeof(url_buffer)); - if (len > 0) { - urls.emplace_back(url_buffer, len); - } - } + this->add_next_url_(builder, MAX_NEXT_URL_LEN); #endif #ifdef USE_WEBSERVER for (auto &ip : wifi::global_wifi_component->wifi_sta_ip_addresses()) { @@ -160,25 +156,63 @@ std::vector ImprovSerialComponent::build_rpc_settings_response_(improv: ip.str_to(ip_buf); // "http://" (7) + IP (40) + ":" (1) + port (5) + null (1) = 54 char webserver_url[7 + network::IP_ADDRESS_BUFFER_SIZE + 1 + 5 + 1]; - snprintf(webserver_url, sizeof(webserver_url), "http://%s:%u", ip_buf, USE_WEBSERVER_PORT); - urls.emplace_back(webserver_url); + // buf_append_printf keeps the format string in flash on ESP8266 + size_t len = + buf_append_printf(webserver_url, sizeof(webserver_url), 0, "http://%s:%u", ip_buf, USE_WEBSERVER_PORT); + if (!builder.add_string(webserver_url, len)) { + ESP_LOGW(TAG, "Response full; URL dropped"); + } break; } } #endif - std::vector data = improv::build_rpc_response(command, urls, false); - return data; + this->send_response_(builder.finish(false)); } -std::vector ImprovSerialComponent::build_version_info_() { +void ImprovSerialComponent::send_version_info_() { +// Entry cost per field is sizeof(lit): a length byte plus the string #ifdef ESPHOME_PROJECT_NAME - std::vector infos = {ESPHOME_PROJECT_NAME, ESPHOME_PROJECT_VERSION, ESPHOME_VARIANT, App.get_name()}; + static constexpr size_t INFO_ENTRIES_LEN = + sizeof(ESPHOME_PROJECT_NAME) + sizeof(ESPHOME_PROJECT_VERSION) + sizeof(ESPHOME_VARIANT); #else - std::vector infos = {"ESPHome", ESPHOME_VERSION, ESPHOME_VARIANT, App.get_name()}; + static constexpr size_t INFO_ENTRIES_LEN = sizeof("ESPHome") + sizeof(ESPHOME_VERSION) + sizeof(ESPHOME_VARIANT); #endif - std::vector data = improv::build_rpc_response(improv::GET_DEVICE_INFO, infos, false); - return data; -}; + static_assert(INFO_ENTRIES_LEN < MAX_SERIAL_PAYLOAD, + "esphome project name and version too long for the improv_serial device info frame"); + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::GET_DEVICE_INFO); +#ifdef USE_ESP8266 + // Keep each literal in flash and copy it through an exact size stack buffer, + // so a long project name or version can never be truncated +#define IMPROV_ADD_INFO(lit) \ + do { \ + static const char progmem_str[] PROGMEM = lit; \ + char tmp[sizeof(lit)]; \ + progmem_memcpy(tmp, progmem_str, sizeof(lit)); \ + builder.add_string(tmp, sizeof(lit) - 1); \ + } while (0) +#else + // Literals are directly flash mapped on all other platforms +#define IMPROV_ADD_INFO(lit) builder.add_string(lit, sizeof(lit) - 1) +#endif +#ifdef ESPHOME_PROJECT_NAME + IMPROV_ADD_INFO(ESPHOME_PROJECT_NAME); + IMPROV_ADD_INFO(ESPHOME_PROJECT_VERSION); +#else + IMPROV_ADD_INFO("ESPHome"); + IMPROV_ADD_INFO(ESPHOME_VERSION); +#endif + IMPROV_ADD_INFO(ESPHOME_VARIANT); +#undef IMPROV_ADD_INFO + // Only the device name length is unknown at compile time + const auto &name = App.get_name(); + if (INFO_ENTRIES_LEN + 1 + name.size() <= MAX_SERIAL_PAYLOAD) { + builder.add_string(name.c_str(), name.size()); + } else { + ESP_LOGW(TAG, "Response full; device name dropped"); + } + this->send_response_(builder.finish(false)); +} bool ImprovSerialComponent::parse_improv_serial_byte_(uint8_t byte) { size_t at = this->rx_buffer_.size(); @@ -229,32 +263,35 @@ bool ImprovSerialComponent::parse_improv_payload_(improv::ImprovCommand &command } this->set_state_(this->state_); if (this->state_ == improv::STATE_PROVISIONED) { - std::vector url = this->build_rpc_settings_response_(improv::GET_CURRENT_STATE); - this->send_response_(url); + this->send_settings_response_(improv::GET_CURRENT_STATE); } return true; case improv::GET_DEVICE_INFO: { - std::vector info = this->build_version_info_(); - this->send_response_(info); + this->send_version_info_(); return true; } case improv::GET_WIFI_NETWORKS: { const auto &results = wifi::global_wifi_component->get_scan_result(); + std::array buf; for (const auto &scan : results) { bool with_auth = false; if (!wifi::should_show_scan_entry(results, scan, with_auth)) continue; // Send each ssid separately to avoid overflowing the buffer char rssi_buf[5]; // int8_t: -128 to 127, max 4 chars + null - *int8_to_str(rssi_buf, scan.get_rssi()) = '\0'; - std::vector data = improv::build_rpc_response( - improv::GET_WIFI_NETWORKS, {scan.get_ssid().str(), rssi_buf, YESNO(with_auth)}, false); - this->send_response_(data); + char *rssi_end = int8_to_str(rssi_buf, scan.get_rssi()); + *rssi_end = '\0'; + improv::RpcResponseBuilder builder(buf, improv::GET_WIFI_NETWORKS); + // SSID(32) + RSSI(4) + YESNO(3) entries always fit the payload + const auto &ssid = scan.get_ssid(); + builder.add_string(ssid.c_str(), ssid.size()); + builder.add_string(rssi_buf, rssi_end - rssi_buf); + builder.add_string(YESNO(with_auth)); + this->send_response_(builder.finish(false)); } // Send empty response to signify the end of the list. - std::vector data = - improv::build_rpc_response(improv::GET_WIFI_NETWORKS, std::vector{}, false); - this->send_response_(data); + improv::RpcResponseBuilder builder(buf, improv::GET_WIFI_NETWORKS); + this->send_response_(builder.finish(false)); return true; } default: { @@ -282,7 +319,14 @@ void ImprovSerialComponent::set_error_(improv::Error error) { this->write_data_(); } -void ImprovSerialComponent::send_response_(std::vector &response) { +void ImprovSerialComponent::send_response_(std::span response) { + // The serial frame length field is a single byte + if (response.size() > MAX_SERIAL_RESPONSE) { + ESP_LOGE(TAG, "Response too long"); + // Fail fast instead of leaving the client to wait out its timeout + this->set_error_(improv::ERROR_UNKNOWN); + return; + } this->tx_header_[TX_TYPE_IDX] = TYPE_RPC_RESPONSE; this->write_data_(response.data(), response.size()); } diff --git a/esphome/components/improv_serial/improv_serial_component.h b/esphome/components/improv_serial/improv_serial_component.h index 5a4eaaa945..692873bbb6 100644 --- a/esphome/components/improv_serial/improv_serial_component.h +++ b/esphome/components/improv_serial/improv_serial_component.h @@ -8,6 +8,7 @@ #include "esphome/core/helpers.h" #ifdef USE_WIFI #include +#include #include #ifdef USE_IMPROV_SERIAL_UART @@ -47,6 +48,22 @@ enum ImprovSerialType : uint8_t { static const uint16_t IMPROV_SERIAL_TIMEOUT = 100; static const uint8_t IMPROV_SERIAL_VERSION = 1; +// The serial frame length field is one byte +static constexpr size_t MAX_SERIAL_RESPONSE = 255; +// command + data length + trailing byte +static constexpr size_t RPC_RESPONSE_OVERHEAD = 3; +static constexpr size_t MAX_SERIAL_PAYLOAD = MAX_SERIAL_RESPONSE - RPC_RESPONSE_OVERHEAD; +#ifdef USE_WEBSERVER +// length byte + "http://" + IPv4 + ":" + port +static constexpr size_t WEBSERVER_URL_RESERVE = 1 + 7 + 15 + 1 + 5; +#else +static constexpr size_t WEBSERVER_URL_RESERVE = 0; +#endif +// Entry budget minus its own length byte +static constexpr size_t MAX_NEXT_URL_LEN = MAX_SERIAL_PAYLOAD - WEBSERVER_URL_RESERVE - 1; + +static_assert(MAX_SERIAL_RESPONSE <= improv::RPC_RESPONSE_MAX_SIZE, "builder buffer too small for the frame"); + class ImprovSerialComponent final : public Component, public improv_base::ImprovBase { public: void setup() override; @@ -66,11 +83,11 @@ class ImprovSerialComponent final : public Component, public improv_base::Improv void set_state_(improv::State state); void send_current_state_(improv::State state); void set_error_(improv::Error error); - void send_response_(std::vector &response); + void send_response_(std::span response); void on_wifi_connect_timeout_(); - std::vector build_rpc_settings_response_(improv::Command command); - std::vector build_version_info_(); + void send_settings_response_(improv::Command command); + void send_version_info_(); ESPHOME_ALWAYS_INLINE optional read_byte_() { optional byte; diff --git a/esphome/components/network/ip_address.h b/esphome/components/network/ip_address.h index f7aa7daf99..28f83cc4fa 100644 --- a/esphome/components/network/ip_address.h +++ b/esphome/components/network/ip_address.h @@ -188,6 +188,8 @@ struct IPAddress { } IPAddress(const std::string &in_address) { inet_aton(in_address.c_str(), &ip_addr_); } IPAddress(const ip_addr_t *other_ip) { ip_addr_ = *other_ip; } + bool is_ip4() const { return true; } + bool is_ip6() const { return false; } /// Write IP address to buffer. Buffer must be at least IP_ADDRESS_BUFFER_SIZE bytes. char *str_to(char *buf) const { inet_ntop(AF_INET, &ip_addr_, buf, IP_ADDRESS_BUFFER_SIZE); diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 993b9dce75..90ecfea72a 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -81,8 +81,6 @@ #define USE_HTTP_REQUEST_OTA_WATCHDOG_TIMEOUT 8000 // NOLINT #define USE_I2S_AUDIO_SPDIF_MODE #define USE_IMAGE -#define USE_IMPROV_SERIAL -#define USE_IMPROV_SERIAL_NEXT_URL #define USE_INFRARED #define USE_IR_RF #define USE_JSON @@ -223,6 +221,9 @@ #define USE_API_USER_DEFINED_ACTION_RESPONSES_JSON #define API_MAX_SEND_QUEUE 8 #define MAX_API_CONNECTIONS 6 +// The Improv library is not in the Zephyr tidy environment +#define USE_IMPROV_SERIAL +#define USE_IMPROV_SERIAL_NEXT_URL #define USE_MD5 #define USE_NOISE #define USE_SHA256 diff --git a/tests/components/improv_base/benchmark.yaml b/tests/components/improv_base/benchmark.yaml new file mode 100644 index 0000000000..8781e614a1 --- /dev/null +++ b/tests/components/improv_base/benchmark.yaml @@ -0,0 +1,6 @@ +# The builder test compares against the Improv library's build_rpc_response, +# so the library must be part of the unit test build. +# Keep the version in sync with the pin in esphome/components/improv_base/__init__.py. +esphome: + libraries: + - improv/Improv@1.2.7 diff --git a/tests/components/improv_base/rpc_response_builder_test.cpp b/tests/components/improv_base/rpc_response_builder_test.cpp new file mode 100644 index 0000000000..d9d0ad90d8 --- /dev/null +++ b/tests/components/improv_base/rpc_response_builder_test.cpp @@ -0,0 +1,102 @@ +#include + +#include +#include +#include +#include +#include + +#include + +namespace esphome::improv_base::testing { + +namespace { + +std::vector build_with_builder(improv::Command command, const std::vector &datum, + bool add_checksum) { + std::array buf; + improv::RpcResponseBuilder builder(buf, command); + for (const auto &str : datum) { + EXPECT_TRUE(builder.add_string(str.c_str(), str.size())); + } + auto out = builder.finish(add_checksum); + return {out.begin(), out.end()}; +} + +} // namespace + +// The serial path sends builder output where build_rpc_response bytes went before, +// so the two must match exactly, including the trailing 0x00 when checksums are off. +TEST(RpcResponseBuilder, ByteIdenticalToBuildRpcResponse) { + const std::vector device_info = {"ESPHome", "2026.9.0", "ESP32", "test-device"}; + const std::vector network = {"MySSID", "-67", "YES"}; + const std::vector empty = {}; + const std::vector max_payload = {std::string(254, 'x')}; + + for (bool add_checksum : {false, true}) { + for (const auto *datum : {&device_info, &network, &empty, &max_payload}) { + EXPECT_EQ(build_with_builder(improv::GET_DEVICE_INFO, *datum, add_checksum), + improv::build_rpc_response(improv::GET_DEVICE_INFO, *datum, add_checksum)); + } + } +} + +// Golden bytes independent of the library: command, data length, string entries, +// then the trailing byte (0x00 without checksum, additive checksum with). +TEST(RpcResponseBuilder, GoldenBytes) { + EXPECT_EQ(build_with_builder(improv::GET_WIFI_NETWORKS, {}, false), (std::vector{0x04, 0x00, 0x00})); + EXPECT_EQ(build_with_builder(improv::GET_WIFI_NETWORKS, {"ab"}, false), + (std::vector{0x04, 0x03, 0x02, 'a', 'b', 0x00})); + // Checksum: 0x04 + 0x03 + 0x02 + 'a' + 'b' = 0xCC + EXPECT_EQ(build_with_builder(improv::GET_WIFI_NETWORKS, {"ab"}, true), + (std::vector{0x04, 0x03, 0x02, 'a', 'b', 0xCC})); +} + +// esp32_improv calls finish() and build_rpc_response() with no checksum flag, +// so the two defaults must agree +TEST(RpcResponseBuilder, DefaultChecksumFlagMatches) { + const std::vector urls = {"https://example.com"}; + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::WIFI_SETTINGS); + for (const auto &str : urls) { + EXPECT_TRUE(builder.add_string(str.c_str(), str.size())); + } + auto out = builder.finish(); + EXPECT_EQ(std::vector(out.begin(), out.end()), improv::build_rpc_response(improv::WIFI_SETTINGS, urls)); +} + +TEST(RpcResponseBuilder, PayloadBudget) { + std::array buf; + + // 254 byte string fills the payload exactly; a second entry no longer fits + improv::RpcResponseBuilder full(buf, improv::GET_DEVICE_INFO); + const std::string big(254, 'x'); + EXPECT_TRUE(full.add_string(big.c_str(), big.size())); + EXPECT_FALSE(full.add_string("y", 1)); + + // 255 byte string can never fit (its length byte would exceed the budget) + improv::RpcResponseBuilder over(buf, improv::GET_DEVICE_INFO); + const std::string too_big(255, 'y'); + EXPECT_FALSE(over.add_string(too_big.c_str(), too_big.size())); + // A wildly out of range length must not wrap the position arithmetic + EXPECT_FALSE(over.add_string("z", static_cast(-1))); + auto out = over.finish(false); + EXPECT_EQ(std::vector(out.begin(), out.end()), (std::vector{0x03, 0x00, 0x00})); +} + +TEST(RpcResponseBuilder, FinishIsIdempotent) { + std::array buf; + improv::RpcResponseBuilder builder(buf, improv::GET_DEVICE_INFO); + EXPECT_TRUE(builder.add_string("abc", 3)); + auto first = builder.finish(true); + const std::vector expected(first.begin(), first.end()); + + EXPECT_FALSE(builder.add_string("late", 4)); + auto again = builder.finish(true); + EXPECT_EQ(std::vector(again.begin(), again.end()), expected); + // The checksum flag on a later call is ignored + auto no_checksum = builder.finish(false); + EXPECT_EQ(std::vector(no_checksum.begin(), no_checksum.end()), expected); +} + +} // namespace esphome::improv_base::testing diff --git a/tests/components/improv_serial/common-uart0.yaml b/tests/components/improv_serial/common-uart0.yaml index 7b7730fd46..45bf1e5c33 100644 --- a/tests/components/improv_serial/common-uart0.yaml +++ b/tests/components/improv_serial/common-uart0.yaml @@ -5,4 +5,6 @@ wifi: logger: hardware_uart: UART0 +# next_url compiles the USE_IMPROV_SERIAL_NEXT_URL branch and add_next_url_ improv_serial: + next_url: https://example.com/?device_name={{device_name}}&ip_address={{ip_address}} diff --git a/tests/integration/fixtures/improv_serial_uart.yaml b/tests/integration/fixtures/improv_serial_uart.yaml index 75ffe97809..daa41c6a95 100644 --- a/tests/integration/fixtures/improv_serial_uart.yaml +++ b/tests/integration/fixtures/improv_serial_uart.yaml @@ -38,3 +38,5 @@ uart_mock: improv_serial: uart_id: mock_uart + # Deterministic on host: only the device name placeholder is used + next_url: https://example.com/?device={{device_name}} diff --git a/tests/integration/test_improv_serial_uart.py b/tests/integration/test_improv_serial_uart.py index 7dad5f74bd..75fb3263d6 100644 --- a/tests/integration/test_improv_serial_uart.py +++ b/tests/integration/test_improv_serial_uart.py @@ -133,8 +133,15 @@ async def test_improv_serial_uart( ) await waiter.wait_for("save_wifi_sta ssid=NewNet") await waiter.wait_for("uart_mock", f"TX 12 bytes: {state_frame_hex(0x04)}") - # Settings RPC response with no URLs: payload [0x01, 0x00, 0x00] and footer - await waiter.wait_for("uart_mock", "TX 3 bytes: 01:00:00") - await waiter.wait_for( - "uart_mock", f"TX 2 bytes: {rpc_footer_hex(bytes([0x01, 0x00, 0x00]))}" + # Settings RPC response carries the formatted next_url and its footer + next_url = b"https://example.com/?device=improv-uart" + payload = ( + bytes([CMD_WIFI_SETTINGS, len(next_url) + 1, len(next_url)]) + + next_url + + b"\x00" ) + await waiter.wait_for( + "uart_mock", + f"TX {len(payload)} bytes: " + ":".join(f"{b:02X}" for b in payload), + ) + await waiter.wait_for("uart_mock", f"TX 2 bytes: {rpc_footer_hex(payload)}") From 51105a25db007d4a14a8de2013d27410d9a1e4a3 Mon Sep 17 00:00:00 2001 From: Iago Veiga Date: Thu, 27 Aug 2026 10:55:51 +0200 Subject: [PATCH 4/6] [tuya] Add water_heater platform (#17323) Co-authored-by: Claude Opus 4.8 Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com> --- CODEOWNERS | 1 + .../components/tuya/water_heater/__init__.py | 120 +++++++++++ .../tuya/water_heater/tuya_water_heater.cpp | 190 ++++++++++++++++++ .../tuya/water_heater/tuya_water_heater.h | 73 +++++++ tests/components/tuya/common.yaml | 21 ++ 5 files changed, 405 insertions(+) create mode 100644 esphome/components/tuya/water_heater/__init__.py create mode 100644 esphome/components/tuya/water_heater/tuya_water_heater.cpp create mode 100644 esphome/components/tuya/water_heater/tuya_water_heater.h diff --git a/CODEOWNERS b/CODEOWNERS index b898788b1a..e1287ca275 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -577,6 +577,7 @@ esphome/components/tuya/select/* @bearpawmaxim esphome/components/tuya/sensor/* @jesserockz esphome/components/tuya/switch/* @jesserockz esphome/components/tuya/text_sensor/* @dentra +esphome/components/tuya/water_heater/* @iago-veiga esphome/components/uart/* @esphome/core esphome/components/uart/button/* @ssieb esphome/components/uart/event/* @eoasmxd diff --git a/esphome/components/tuya/water_heater/__init__.py b/esphome/components/tuya/water_heater/__init__.py new file mode 100644 index 0000000000..7d1af791b4 --- /dev/null +++ b/esphome/components/tuya/water_heater/__init__.py @@ -0,0 +1,120 @@ +import esphome.codegen as cg +from esphome.components import water_heater +import esphome.config_validation as cv +from esphome.const import CONF_SUPPORTED_MODES, CONF_SWITCH_DATAPOINT +from esphome.types import ConfigType + +from .. import CONF_TUYA_ID, Tuya, tuya_ns + +DEPENDENCIES = ["tuya"] +CODEOWNERS = ["@iago-veiga"] + +CONF_TARGET_TEMPERATURE_DATAPOINT = "target_temperature_datapoint" +CONF_CURRENT_TEMPERATURE_DATAPOINT = "current_temperature_datapoint" +CONF_TARGET_TEMPERATURE_MULTIPLIER = "target_temperature_multiplier" +CONF_CURRENT_TEMPERATURE_MULTIPLIER = "current_temperature_multiplier" +CONF_MODE_DATAPOINT = "mode_datapoint" + +# Optional enum values that map a Tuya mode datapoint value to a WaterHeaterMode. +# Mirrors the "*_value" style used by the tuya climate fan modes. +CONF_ECO_VALUE = "eco_value" +CONF_ELECTRIC_VALUE = "electric_value" +CONF_PERFORMANCE_VALUE = "performance_value" +CONF_HIGH_DEMAND_VALUE = "high_demand_value" +CONF_HEAT_PUMP_VALUE = "heat_pump_value" +CONF_GAS_VALUE = "gas_value" + +# Map of config key -> C++ setter name, one per non-OFF WaterHeaterMode. OFF is not an enum +# value: it is represented by the switch datapoint being off, just like the tuya climate. +MODE_VALUES = { + CONF_ECO_VALUE: "set_eco_value", + CONF_ELECTRIC_VALUE: "set_electric_value", + CONF_PERFORMANCE_VALUE: "set_performance_value", + CONF_HIGH_DEMAND_VALUE: "set_high_demand_value", + CONF_HEAT_PUMP_VALUE: "set_heat_pump_value", + CONF_GAS_VALUE: "set_gas_value", +} + +TuyaWaterHeater = tuya_ns.class_( + "TuyaWaterHeater", water_heater.WaterHeater, cg.Component +) + + +def _validate(config: ConfigType) -> ConfigType: + # A mode datapoint is only useful if at least one mode value is mapped, and mode values + # only make sense together with a mode datapoint. + has_mode_values = any(key in config for key in MODE_VALUES) + if CONF_MODE_DATAPOINT in config and not has_mode_values: + raise cv.Invalid( + f"'{CONF_MODE_DATAPOINT}' requires at least one mode value " + f"(e.g. '{CONF_ECO_VALUE}' or '{CONF_ELECTRIC_VALUE}')" + ) + if has_mode_values and CONF_MODE_DATAPOINT not in config: + raise cv.Invalid(f"Mode values require '{CONF_MODE_DATAPOINT}' to be set") + return config + + +CONFIG_SCHEMA = cv.All( + water_heater.water_heater_schema(TuyaWaterHeater) + .extend( + { + cv.GenerateID(CONF_TUYA_ID): cv.use_id(Tuya), + cv.Required(CONF_SWITCH_DATAPOINT): cv.uint8_t, + cv.Optional(CONF_TARGET_TEMPERATURE_DATAPOINT): cv.uint8_t, + cv.Optional(CONF_CURRENT_TEMPERATURE_DATAPOINT): cv.uint8_t, + cv.Optional( + CONF_TARGET_TEMPERATURE_MULTIPLIER, default=1.0 + ): cv.positive_float, + cv.Optional( + CONF_CURRENT_TEMPERATURE_MULTIPLIER, default=1.0 + ): cv.positive_float, + cv.Optional(CONF_MODE_DATAPOINT): cv.uint8_t, + cv.Optional(CONF_ECO_VALUE): cv.uint8_t, + cv.Optional(CONF_ELECTRIC_VALUE): cv.uint8_t, + cv.Optional(CONF_PERFORMANCE_VALUE): cv.uint8_t, + cv.Optional(CONF_HIGH_DEMAND_VALUE): cv.uint8_t, + cv.Optional(CONF_HEAT_PUMP_VALUE): cv.uint8_t, + cv.Optional(CONF_GAS_VALUE): cv.uint8_t, + cv.Optional(CONF_SUPPORTED_MODES): cv.ensure_list( + water_heater.validate_water_heater_mode + ), + } + ) + .extend(cv.COMPONENT_SCHEMA), + _validate, +) + + +async def to_code(config: ConfigType) -> None: + var = await water_heater.new_water_heater(config) + await cg.register_component(var, config) + + paren = await cg.get_variable(config[CONF_TUYA_ID]) + cg.add(var.set_tuya_parent(paren)) + + cg.add(var.set_switch_id(config[CONF_SWITCH_DATAPOINT])) + + if (target_temp_dp := config.get(CONF_TARGET_TEMPERATURE_DATAPOINT)) is not None: + cg.add(var.set_target_temperature_id(target_temp_dp)) + if (current_temp_dp := config.get(CONF_CURRENT_TEMPERATURE_DATAPOINT)) is not None: + cg.add(var.set_current_temperature_id(current_temp_dp)) + + cg.add( + var.set_target_temperature_multiplier( + config[CONF_TARGET_TEMPERATURE_MULTIPLIER] + ) + ) + cg.add( + var.set_current_temperature_multiplier( + config[CONF_CURRENT_TEMPERATURE_MULTIPLIER] + ) + ) + + if (mode_dp := config.get(CONF_MODE_DATAPOINT)) is not None: + cg.add(var.set_mode_id(mode_dp)) + for key, setter in MODE_VALUES.items(): + if (value := config.get(key)) is not None: + cg.add(getattr(var, setter)(value)) + + if (supported_modes := config.get(CONF_SUPPORTED_MODES)) is not None: + cg.add(var.set_supported_modes(supported_modes)) diff --git a/esphome/components/tuya/water_heater/tuya_water_heater.cpp b/esphome/components/tuya/water_heater/tuya_water_heater.cpp new file mode 100644 index 0000000000..2fca3bf581 --- /dev/null +++ b/esphome/components/tuya/water_heater/tuya_water_heater.cpp @@ -0,0 +1,190 @@ +#include "tuya_water_heater.h" +#include "esphome/core/log.h" + +namespace esphome::tuya { + +static const char *const TAG = "tuya.water_heater"; + +void TuyaWaterHeater::setup() { + if (this->switch_id_.has_value()) { + this->parent_->register_listener(*this->switch_id_, [this](const TuyaDatapoint &datapoint) { + ESP_LOGV(TAG, "MCU reported switch is: %s", ONOFF(datapoint.value_bool)); + this->is_on_ = datapoint.value_bool; + this->set_state_flag_(water_heater::WATER_HEATER_STATE_ON, this->is_on_); + if (!this->is_on_) { + this->set_mode_(water_heater::WATER_HEATER_MODE_OFF); + } else { + // Turned on: use the last mode reported by the mode datapoint if we have one, otherwise + // fall back to a supported mode. Datapoints can arrive in any order, so the mode enum may + // have been reported before this switch update. + this->set_mode_(this->last_reported_mode_.value_or(this->default_on_mode_())); + } + this->publish_state(); + }); + } + + if (this->mode_id_.has_value()) { + this->parent_->register_listener(*this->mode_id_, [this](const TuyaDatapoint &datapoint) { + ESP_LOGV(TAG, "MCU reported mode value is: %u", datapoint.value_enum); + water_heater::WaterHeaterMode mode; + if (!this->mode_from_value_(datapoint.value_enum, mode)) { + return; + } + // Always remember the reported mode; only surface it while the heater is on (OFF is driven + // by the switch datapoint, not the mode enum). + this->last_reported_mode_ = mode; + if (this->is_on_ && this->mode_ != mode) { + this->set_mode_(mode); + this->publish_state(); + } + }); + } + + if (this->target_temperature_id_.has_value()) { + this->parent_->register_listener(*this->target_temperature_id_, [this](const TuyaDatapoint &datapoint) { + float value = datapoint.value_int * this->target_temperature_multiplier_; + ESP_LOGV(TAG, "MCU reported target temperature is: %.1f", value); + this->set_target_temperature_(value); + this->publish_state(); + }); + } + + if (this->current_temperature_id_.has_value()) { + this->parent_->register_listener(*this->current_temperature_id_, [this](const TuyaDatapoint &datapoint) { + float value = datapoint.value_int * this->current_temperature_multiplier_; + ESP_LOGV(TAG, "MCU reported current temperature is: %.1f", value); + this->set_current_temperature(value); + this->publish_state(); + }); + } +} + +water_heater::WaterHeaterCallInternal TuyaWaterHeater::make_call() { + return water_heater::WaterHeaterCallInternal(this); +} + +void TuyaWaterHeater::control(const water_heater::WaterHeaterCall &call) { + auto mode_val = call.get_mode(); + auto on_val = call.get_on(); + + // Determine the desired on/off state. An explicit on/off request wins; otherwise a mode of + // OFF means off and any other mode means on. + optional want_on = on_val; + if (mode_val.has_value() && !want_on.has_value()) { + want_on = *mode_val != water_heater::WATER_HEATER_MODE_OFF; + } + + if (want_on.has_value() && this->switch_id_.has_value()) { + ESP_LOGV(TAG, "Setting switch: %s", ONOFF(*want_on)); + this->parent_->set_boolean_datapoint_value(*this->switch_id_, *want_on); + } + + if (mode_val.has_value() && *mode_val != water_heater::WATER_HEATER_MODE_OFF && this->mode_id_.has_value()) { + uint8_t value; + if (this->value_from_mode_(*mode_val, value)) { + ESP_LOGV(TAG, "Setting mode value: %u", value); + this->parent_->set_enum_datapoint_value(*this->mode_id_, value); + } else { + ESP_LOGW(TAG, "No mode value configured for requested mode"); + } + } + + auto target_temp = call.get_target_temperature(); + if (!std::isnan(target_temp) && this->target_temperature_id_.has_value()) { + ESP_LOGV(TAG, "Setting target temperature: %.1f", target_temp); + this->parent_->set_integer_datapoint_value(*this->target_temperature_id_, + (int) (target_temp / this->target_temperature_multiplier_)); + } +} + +water_heater::WaterHeaterTraits TuyaWaterHeater::traits() { + water_heater::WaterHeaterTraits traits; + + traits.add_feature_flags(water_heater::WATER_HEATER_SUPPORTS_ON_OFF); + if (this->current_temperature_id_.has_value()) { + traits.add_feature_flags(water_heater::WATER_HEATER_SUPPORTS_CURRENT_TEMPERATURE); + } + if (this->target_temperature_id_.has_value()) { + traits.add_feature_flags(water_heater::WATER_HEATER_SUPPORTS_TARGET_TEMPERATURE); + } + if (!this->supported_modes_.empty()) { + traits.set_supported_modes(this->supported_modes_); + traits.add_feature_flags(water_heater::WATER_HEATER_SUPPORTS_OPERATION_MODE); + } + return traits; +} + +bool TuyaWaterHeater::mode_from_value_(uint8_t value, water_heater::WaterHeaterMode &mode) const { + if (this->eco_value_ == value) { + mode = water_heater::WATER_HEATER_MODE_ECO; + } else if (this->electric_value_ == value) { + mode = water_heater::WATER_HEATER_MODE_ELECTRIC; + } else if (this->performance_value_ == value) { + mode = water_heater::WATER_HEATER_MODE_PERFORMANCE; + } else if (this->high_demand_value_ == value) { + mode = water_heater::WATER_HEATER_MODE_HIGH_DEMAND; + } else if (this->heat_pump_value_ == value) { + mode = water_heater::WATER_HEATER_MODE_HEAT_PUMP; + } else if (this->gas_value_ == value) { + mode = water_heater::WATER_HEATER_MODE_GAS; + } else { + return false; + } + return true; +} + +bool TuyaWaterHeater::value_from_mode_(water_heater::WaterHeaterMode mode, uint8_t &value) const { + optional mapped; + switch (mode) { + case water_heater::WATER_HEATER_MODE_ECO: + mapped = this->eco_value_; + break; + case water_heater::WATER_HEATER_MODE_ELECTRIC: + mapped = this->electric_value_; + break; + case water_heater::WATER_HEATER_MODE_PERFORMANCE: + mapped = this->performance_value_; + break; + case water_heater::WATER_HEATER_MODE_HIGH_DEMAND: + mapped = this->high_demand_value_; + break; + case water_heater::WATER_HEATER_MODE_HEAT_PUMP: + mapped = this->heat_pump_value_; + break; + case water_heater::WATER_HEATER_MODE_GAS: + mapped = this->gas_value_; + break; + default: + break; + } + if (mapped.has_value()) { + value = *mapped; + return true; + } + return false; +} + +water_heater::WaterHeaterMode TuyaWaterHeater::default_on_mode_() const { + // Prefer the first configured supported non-OFF mode so we never surface a mode the user + // cannot control. Fall back to ELECTRIC when no supported modes are configured. + for (water_heater::WaterHeaterMode mode : this->supported_modes_) { + if (mode != water_heater::WATER_HEATER_MODE_OFF) { + return mode; + } + } + return water_heater::WATER_HEATER_MODE_ELECTRIC; +} + +void TuyaWaterHeater::dump_config() { + LOG_WATER_HEATER("", "Tuya Water Heater", this); + if (this->switch_id_.has_value()) + ESP_LOGCONFIG(TAG, " Switch has datapoint ID %u", *this->switch_id_); + if (this->mode_id_.has_value()) + ESP_LOGCONFIG(TAG, " Mode has datapoint ID %u", *this->mode_id_); + if (this->target_temperature_id_.has_value()) + ESP_LOGCONFIG(TAG, " Target Temperature has datapoint ID %u", *this->target_temperature_id_); + if (this->current_temperature_id_.has_value()) + ESP_LOGCONFIG(TAG, " Current Temperature has datapoint ID %u", *this->current_temperature_id_); +} + +} // namespace esphome::tuya diff --git a/esphome/components/tuya/water_heater/tuya_water_heater.h b/esphome/components/tuya/water_heater/tuya_water_heater.h new file mode 100644 index 0000000000..5ce0ce4dee --- /dev/null +++ b/esphome/components/tuya/water_heater/tuya_water_heater.h @@ -0,0 +1,73 @@ +#pragma once + +#include "esphome/core/component.h" +#include "esphome/components/tuya/tuya.h" +#include "esphome/components/water_heater/water_heater.h" + +namespace esphome::tuya { + +class TuyaWaterHeater final : public water_heater::WaterHeater, public Component { + public: + void setup() override; + void dump_config() override; + + void set_tuya_parent(Tuya *parent) { this->parent_ = parent; } + + void set_switch_id(uint8_t switch_id) { this->switch_id_ = switch_id; } + void set_target_temperature_id(uint8_t target_temperature_id) { + this->target_temperature_id_ = target_temperature_id; + } + void set_current_temperature_id(uint8_t current_temperature_id) { + this->current_temperature_id_ = current_temperature_id; + } + void set_target_temperature_multiplier(float multiplier) { this->target_temperature_multiplier_ = multiplier; } + void set_current_temperature_multiplier(float multiplier) { this->current_temperature_multiplier_ = multiplier; } + + void set_mode_id(uint8_t mode_id) { this->mode_id_ = mode_id; } + void set_eco_value(uint8_t value) { this->eco_value_ = value; } + void set_electric_value(uint8_t value) { this->electric_value_ = value; } + void set_performance_value(uint8_t value) { this->performance_value_ = value; } + void set_high_demand_value(uint8_t value) { this->high_demand_value_ = value; } + void set_heat_pump_value(uint8_t value) { this->heat_pump_value_ = value; } + void set_gas_value(uint8_t value) { this->gas_value_ = value; } + + void set_supported_modes(const std::initializer_list &modes) { + this->supported_modes_ = modes; + } + + water_heater::WaterHeaterCallInternal make_call() override; + + protected: + void control(const water_heater::WaterHeaterCall &call) override; + water_heater::WaterHeaterTraits traits() override; + + /// Map a Tuya mode datapoint enum value to a WaterHeaterMode. Returns true when a mapping + /// exists, writing the result to \p mode. + bool mode_from_value_(uint8_t value, water_heater::WaterHeaterMode &mode) const; + /// Map a WaterHeaterMode to its configured Tuya enum value. Returns true when a mapping exists. + bool value_from_mode_(water_heater::WaterHeaterMode mode, uint8_t &value) const; + + Tuya *parent_{nullptr}; + optional switch_id_{}; + optional target_temperature_id_{}; + optional current_temperature_id_{}; + optional mode_id_{}; + optional eco_value_{}; + optional electric_value_{}; + optional performance_value_{}; + optional high_demand_value_{}; + optional heat_pump_value_{}; + optional gas_value_{}; + float target_temperature_multiplier_{1.0f}; + float current_temperature_multiplier_{1.0f}; + water_heater::WaterHeaterModeMask supported_modes_; + /// Last non-OFF mode reported by the mode datapoint, applied when the heater turns on. + optional last_reported_mode_{}; + bool is_on_{false}; + + /// The mode to show when the heater is on but no mode datapoint value is known yet: the last + /// reported mode, else the first configured supported non-OFF mode, else ELECTRIC. + water_heater::WaterHeaterMode default_on_mode_() const; +}; + +} // namespace esphome::tuya diff --git a/tests/components/tuya/common.yaml b/tests/components/tuya/common.yaml index 9986d398f1..f52d47e7a0 100644 --- a/tests/components/tuya/common.yaml +++ b/tests/components/tuya/common.yaml @@ -80,3 +80,24 @@ switch: - platform: tuya id: tuya_switch switch_datapoint: 1 + +water_heater: + - platform: tuya + id: tuya_water_heater + name: Tuya Water Heater + switch_datapoint: 1 + current_temperature_datapoint: 3 + target_temperature_datapoint: 2 + current_temperature_multiplier: 0.5 + target_temperature_multiplier: 0.5 + mode_datapoint: 4 + eco_value: 0 + electric_value: 2 + supported_modes: + - "OFF" + - ECO + - ELECTRIC + visual: + min_temperature: 30 + max_temperature: 75 + target_temperature_step: 1 From 18b70026043d2c170e646276e067d524d16ee31f Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Thu, 27 Aug 2026 06:43:51 -0700 Subject: [PATCH 5/6] [modbus_controller] Apply the write offset byte-accurately on switch and output (#18787) Co-authored-by: J. Nick Koston --- .../components/modbus_controller/__init__.py | 15 ++++ .../modbus_controller/output/__init__.py | 38 +++++----- .../modbus_controller/output/modbus_output.h | 3 +- .../modbus_controller/switch/__init__.py | 10 +++ .../modbus_controller/switch/modbus_switch.h | 9 ++- .../modbus_controller/test_write_offset.py | 74 +++++++++++++++++++ .../uart_mock_modbus_register_offset.yaml | 2 +- tests/integration/test_uart_mock_modbus.py | 7 -- 8 files changed, 130 insertions(+), 28 deletions(-) create mode 100644 tests/component_tests/modbus_controller/test_write_offset.py diff --git a/esphome/components/modbus_controller/__init__.py b/esphome/components/modbus_controller/__init__.py index e87eccb32c..c390d8ab79 100644 --- a/esphome/components/modbus_controller/__init__.py +++ b/esphome/components/modbus_controller/__init__.py @@ -278,6 +278,21 @@ def _final_validate(config: ConfigType) -> None: FINAL_VALIDATE_SCHEMA = _final_validate +def reject_odd_holding_write_offset(config: ConfigType) -> ConfigType: + """Reject an odd byte offset on a holding-register write entity. + + A 16-bit register write cannot target half a register, so the residual byte is inexpressible. + """ + key = CONF_BYTE_OFFSET if CONF_BYTE_OFFSET in config else CONF_OFFSET + if config.get(key, 0) % 2: + raise cv.Invalid( + f"An odd '{key}' cannot be used with holding-register writes: a 16-bit register " + "write cannot target half a register. Use an even offset, or fold it into 'address'", + path=[key], + ) + return config + + def modbus_calc_properties(config: ConfigType) -> tuple[int, int]: byte_offset = 0 reg_count = 0 diff --git a/esphome/components/modbus_controller/output/__init__.py b/esphome/components/modbus_controller/output/__init__.py index 34a0f488ec..c2055fa690 100644 --- a/esphome/components/modbus_controller/output/__init__.py +++ b/esphome/components/modbus_controller/output/__init__.py @@ -14,6 +14,7 @@ from .. import ( SensorItem, modbus_calc_properties, modbus_controller_ns, + reject_odd_holding_write_offset, ) from ..const import ( CONF_CUSTOM_COMMAND, @@ -53,23 +54,26 @@ CONFIG_SCHEMA = cv.typed_schema( cv.Optional(CONF_USE_WRITE_MULTIPLE, default=False): cv.boolean, } ), - "holding": output.FLOAT_OUTPUT_SCHEMA.extend(ModbusItemBaseSchema).extend( - { - cv.GenerateID(): cv.declare_id(ModbusFloatOutput), - cv.Required(CONF_ADDRESS): cv.positive_int, - cv.Optional(CONF_CUSTOM_PDU): cv.invalid( - "custom_pdu is not supported for outputs; use a write_lambda instead" - ), - cv.Optional(CONF_CUSTOM_COMMAND): cv.invalid( - "custom_command is not supported for outputs; use a write_lambda instead" - ), - cv.Optional(CONF_VALUE_TYPE, default="U_WORD"): cv.enum( - SENSOR_VALUE_TYPE - ), - cv.Optional(CONF_WRITE_LAMBDA): cv.returning_lambda, - cv.Optional(CONF_MULTIPLY, default=1.0): cv.float_, - cv.Optional(CONF_USE_WRITE_MULTIPLE, default=False): cv.boolean, - } + "holding": cv.All( + output.FLOAT_OUTPUT_SCHEMA.extend(ModbusItemBaseSchema).extend( + { + cv.GenerateID(): cv.declare_id(ModbusFloatOutput), + cv.Required(CONF_ADDRESS): cv.positive_int, + cv.Optional(CONF_CUSTOM_PDU): cv.invalid( + "custom_pdu is not supported for outputs; use a write_lambda instead" + ), + cv.Optional(CONF_CUSTOM_COMMAND): cv.invalid( + "custom_command is not supported for outputs; use a write_lambda instead" + ), + cv.Optional(CONF_VALUE_TYPE, default="U_WORD"): cv.enum( + SENSOR_VALUE_TYPE + ), + cv.Optional(CONF_WRITE_LAMBDA): cv.returning_lambda, + cv.Optional(CONF_MULTIPLY, default=1.0): cv.float_, + cv.Optional(CONF_USE_WRITE_MULTIPLE, default=False): cv.boolean, + } + ), + reject_odd_holding_write_offset, ), }, lower=True, diff --git a/esphome/components/modbus_controller/output/modbus_output.h b/esphome/components/modbus_controller/output/modbus_output.h index b942dcea62..48153dc0b7 100644 --- a/esphome/components/modbus_controller/output/modbus_output.h +++ b/esphome/components/modbus_controller/output/modbus_output.h @@ -12,7 +12,8 @@ class ModbusFloatOutput final : public output::FloatOutput, public Component, pu public: ModbusFloatOutput(uint16_t start_address, uint8_t offset, SensorValueType value_type, int register_count) { this->register_type = modbus::EntityType::HOLDING; - this->set_address(start_address + offset); + // A byte offset folds into the address as whole registers; odd offsets are rejected at validation. + this->set_address(start_address + offset / 2); this->set_offset_from_start_address(0); this->bitmask = 0xFFFFFFFF; this->register_count = register_count; diff --git a/esphome/components/modbus_controller/switch/__init__.py b/esphome/components/modbus_controller/switch/__init__.py index c52067f941..49dc0bb222 100644 --- a/esphome/components/modbus_controller/switch/__init__.py +++ b/esphome/components/modbus_controller/switch/__init__.py @@ -11,6 +11,7 @@ from .. import ( add_modbus_base_properties, modbus_calc_properties, modbus_controller_ns, + reject_odd_holding_write_offset, validate_custom_pdu_item, validate_modbus_register, ) @@ -31,6 +32,14 @@ ModbusSwitch = modbus_controller_ns.class_( "ModbusSwitch", cg.Component, switch.Switch, SensorItem ) + +def _validate_holding_offset(config: ConfigType) -> ConfigType: + # Only a holding-register switch folds the byte offset into a 16-bit register write. + if config.get(CONF_REGISTER_TYPE) == "holding": + reject_odd_holding_write_offset(config) + return config + + CONFIG_SCHEMA = cv.All( switch.switch_schema(ModbusSwitch, default_restore_mode="DISABLED") .extend(cv.COMPONENT_SCHEMA) @@ -44,6 +53,7 @@ CONFIG_SCHEMA = cv.All( } ), validate_modbus_register, + _validate_holding_offset, ) FINAL_VALIDATE_SCHEMA = validate_custom_pdu_item diff --git a/esphome/components/modbus_controller/switch/modbus_switch.h b/esphome/components/modbus_controller/switch/modbus_switch.h index 1d3d03919f..bd1c837080 100644 --- a/esphome/components/modbus_controller/switch/modbus_switch.h +++ b/esphome/components/modbus_controller/switch/modbus_switch.h @@ -18,8 +18,13 @@ class ModbusSwitch final : public Component, public switch_::Switch, public Sens this->bitmask = bitmask; this->sensor_value_type = SensorValueType::BIT; this->register_count = 1; - if (register_type == modbus::EntityType::HOLDING || register_type == modbus::EntityType::COIL) { - this->set_address(this->start_address + offset); + // A holding byte offset folds into the address as whole registers (odd offsets are rejected at + // validation: a 16-bit register write cannot target half a register); a coil offset is a coil count. + if (register_type == modbus::EntityType::HOLDING) { + this->set_address(start_address + offset / 2); + this->set_offset_from_start_address(0); + } else if (register_type == modbus::EntityType::COIL) { + this->set_address(start_address + offset); this->set_offset_from_start_address(0); } this->force_new_range = force_new_range; diff --git a/tests/component_tests/modbus_controller/test_write_offset.py b/tests/component_tests/modbus_controller/test_write_offset.py new file mode 100644 index 0000000000..72bd3a933e --- /dev/null +++ b/tests/component_tests/modbus_controller/test_write_offset.py @@ -0,0 +1,74 @@ +"""Config validation for the byte offset on holding-register write entities. + +A 16-bit register write cannot target half a register, so an odd offset (or byte_offset) is +rejected for holding-register switches and outputs; even offsets and coil offsets pass. +""" + +import pytest +from voluptuous import Invalid, MultipleInvalid + +from esphome.components.modbus_controller.output import ( + CONFIG_SCHEMA as OUTPUT_CONFIG_SCHEMA, +) +from esphome.components.modbus_controller.switch import ( + CONFIG_SCHEMA as SWITCH_CONFIG_SCHEMA, +) +from esphome.const import CONF_ADDRESS, CONF_ID, CONF_NAME, CONF_OFFSET + + +def _switch_config(register_type: str, offset: int) -> dict: + return { + CONF_NAME: "test switch", + CONF_ADDRESS: 0x10, + "register_type": register_type, + CONF_OFFSET: offset, + } + + +def _output_config(register_type: str, offset: int) -> dict: + return { + CONF_ID: "test_output", + CONF_ADDRESS: 0x10, + "register_type": register_type, + CONF_OFFSET: offset, + } + + +def test_odd_offset_on_holding_switch_rejected() -> None: + with pytest.raises((Invalid, MultipleInvalid), match="odd"): + SWITCH_CONFIG_SCHEMA(_switch_config("holding", 3)) + + +def test_even_offset_on_holding_switch_accepted() -> None: + config = SWITCH_CONFIG_SCHEMA(_switch_config("holding", 2)) + assert config[CONF_OFFSET] == 2 + + +def test_odd_offset_on_coil_switch_accepted() -> None: + """A coil offset is a coil count, so odd values are fine.""" + config = SWITCH_CONFIG_SCHEMA(_switch_config("coil", 3)) + assert config[CONF_OFFSET] == 3 + + +def test_odd_byte_offset_on_holding_switch_rejected() -> None: + """byte_offset is the alias the validator must also catch.""" + config = _switch_config("holding", 0) + del config[CONF_OFFSET] + config["byte_offset"] = 3 + with pytest.raises((Invalid, MultipleInvalid), match="byte_offset"): + SWITCH_CONFIG_SCHEMA(config) + + +def test_odd_offset_on_holding_output_rejected() -> None: + with pytest.raises((Invalid, MultipleInvalid), match="odd"): + OUTPUT_CONFIG_SCHEMA(_output_config("holding", 3)) + + +def test_even_offset_on_holding_output_accepted() -> None: + config = OUTPUT_CONFIG_SCHEMA(_output_config("holding", 2)) + assert config[CONF_OFFSET] == 2 + + +def test_odd_offset_on_coil_output_accepted() -> None: + config = OUTPUT_CONFIG_SCHEMA(_output_config("coil", 3)) + assert config[CONF_OFFSET] == 3 diff --git a/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml b/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml index e93e78d5a3..21c451aa99 100644 --- a/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml +++ b/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml @@ -100,7 +100,7 @@ switch: offset: 2 assumed_state: true # A holding-register switch that READS its state. Byte offset 6 -> register 0x10 + 6/2 = 0x13. Post-fix - # the switch itself resolves to 0x13 (whole registers fold into the address, residual byte stays) and + # the switch itself resolves to 0x13 (the even byte offset folds into the address as whole registers) and # joins the 0x10..0x13 range, so no separate 0x13 sensor is needed. Pre-fix the whole byte offset folds # into the address (0x16), where the server answers ILLEGAL_DATA_ADDRESS and the switch never publishes. - platform: modbus_controller diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index 3dfeda9b37..ae9a481630 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -967,13 +967,6 @@ async def test_uart_mock_modbus_client_read_write( _assert_no_modbus_errors(error_log_lines, warning_log_lines) -@pytest.mark.xfail( - strict=True, - reason="Byte-accurate register-offset writes land in the follow-up offset fix; " - "until then the byte offset is folded into the address (writes 0x12 instead of " - "0x11). The write and read assertions both flip via the same switch-constructor " - "fold. Remove this marker when that change merges.", -) @pytest.mark.asyncio async def test_uart_mock_modbus_register_offset( yaml_config: str, From a8c8827a5b1902b980464a67270cc7e923d7f0c9 Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Thu, 27 Aug 2026 08:33:16 -0700 Subject: [PATCH 6/6] [modbus_controller] Switch write_lambda return value is the wire value only (#18788) Co-authored-by: J. Nick Koston --- .../switch/modbus_switch.cpp | 18 ++-- .../uart_mock_modbus_lambda_invert.yaml | 95 +++++++++++++++++++ tests/integration/test_uart_mock_modbus.py | 55 +++++++++++ 3 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 tests/integration/fixtures/uart_mock_modbus_lambda_invert.yaml diff --git a/esphome/components/modbus_controller/switch/modbus_switch.cpp b/esphome/components/modbus_controller/switch/modbus_switch.cpp index c942ff1e6f..7bf45366c0 100644 --- a/esphome/components/modbus_controller/switch/modbus_switch.cpp +++ b/esphome/components/modbus_controller/switch/modbus_switch.cpp @@ -65,6 +65,7 @@ void ModbusSwitch::write_state(bool state) { // so a rapidly-changing value writes the latest, not every intermediate. this->clear_tx_queue_for_device(); modbus::helpers::PduBuffer data; + bool write_value = state; if (this->write_transform_func_.has_value()) { // The lambda may drive the write itself via item->write_*/queue_pdu(), override the written value (return a // value), or (deprecated) fill `data` with a custom PDU. @@ -92,26 +93,29 @@ void ModbusSwitch::write_state(bool state) { ESP_LOGV(TAG, "Communication handled by lambda - exiting control"); return; } + // The returned bool is the wire value only; the entity still reports the requested state. A polled + // entity needs the read lambda inverted to match, or the next poll flips the display back. ESP_LOGV(TAG, "Value overwritten by lambda"); - state = val.value(); + write_value = val.value(); } - ESP_LOGV(TAG, "write_state '%s': new value = %s type = %d address = %X offset = %x", this->get_name().c_str(), - ONOFF(state), (int) this->register_type, this->start_address, this->offset); + ESP_LOGV(TAG, "write_state '%s': new value = %s (wire = %s) type = %d address = %X offset = %x", + this->get_name().c_str(), ONOFF(state), ONOFF(write_value), (int) this->register_type, this->start_address, + this->offset); bool queued; if (this->register_type == EntityType::COIL) { // offset for coil and discrete inputs is the coil/register number not bytes if (this->use_write_multiple_) { - std::array states{state}; + std::array states{write_value}; queued = this->write_multiple_coils(this->write_address(), states); } else { - queued = this->write_single_coil(this->write_address(), state); + queued = this->write_single_coil(this->write_address(), write_value); } } else { if (this->use_write_multiple_) { - std::array states{static_cast(state ? (0xFFFF & this->bitmask) : 0)}; + std::array states{static_cast(write_value ? (0xFFFF & this->bitmask) : 0)}; queued = this->write_multiple_registers(this->write_address(), states); } else { - queued = this->write_single_register(this->write_address(), state ? 0xFFFF & this->bitmask : 0u); + queued = this->write_single_register(this->write_address(), write_value ? 0xFFFF & this->bitmask : 0u); } } if (!queued) { diff --git a/tests/integration/fixtures/uart_mock_modbus_lambda_invert.yaml b/tests/integration/fixtures/uart_mock_modbus_lambda_invert.yaml new file mode 100644 index 0000000000..41afce70d6 --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_lambda_invert.yaml @@ -0,0 +1,95 @@ +esphome: + name: uart-mock-modbus-lambda-invert + +host: +api: +logger: + level: VERBOSE + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + +# Dummy uart entry to satisfy modbus's DEPENDENCIES = ["uart"] +# The actual UART bus used is the uart_mock component below +uart: + baud_rate: 115200 + port: /dev/null + +uart_mock: + - id: virtual_uart_server + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_controller + data: !lambda return data; + - id: virtual_uart_controller + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_server + data: !lambda return data; + +globals: + - id: reg40 + type: uint16_t + initial_value: "5" + +modbus: + - uart_id: virtual_uart_server + id: virtual_modbus_server + role: server + - uart_id: virtual_uart_controller + id: virtual_modbus_controller + role: client + turnaround_time: 10ms + +modbus_controller: + - address: 1 + modbus_id: virtual_modbus_controller + id: modbus_controller_1 + update_interval: 1s + +modbus_server: + - address: 1 + modbus_id: virtual_modbus_server + id: modbus_server_1 + registers: + - address: 0x40 + value_type: U_WORD + read_lambda: return id(reg40); + write_lambda: id(reg40) = x; return true; + +# An active-low holding switch: the write_lambda inverts the wire value, but the entity must still +# report the REQUESTED state. assumed_state keeps the register unpolled, so the published state comes +# only from write_state() - turning ON writes 0x0000 yet the switch shows ON. +switch: + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "invert_switch" + register_type: holding + address: 0x40 + assumed_state: true + write_lambda: |- + return !x; + +sensor: + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "reg_40" + address: 0x40 + register_type: holding + value_type: U_WORD + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + # This test does not have anything to start (mock is autostart) diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index ae9a481630..f88febabf5 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -1058,6 +1058,61 @@ async def test_uart_mock_modbus_lambda_write( await tracker.await_change(wrote_30, "reg_30", timeout=4.0) +@pytest.mark.asyncio +async def test_uart_mock_modbus_lambda_invert( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that a write_lambda's return value is the wire value only. + + `invert_switch` is an active-low holding switch whose write_lambda returns !x. Turning it ON must + write 0x0000 to the register (observed through the independent reg_40 sensor) while the entity + reports ON - the requested state, not the inverted wire value. Turning it OFF writes 0xFFFF and + reports OFF. The switch is assumed_state, so the published state comes only from write_state(). + """ + + tracker = SensorTracker(["reg_40"]) + initial = tracker.expect("reg_40", 5) + wrote_on = tracker.expect("reg_40", 0) + wrote_off = tracker.expect("reg_40", 65535) + + async with ( + run_compiled(yaml_config), + api_client_connected() as client, + ): + entities = await tracker.setup_and_start_scenario(client) + await tracker.await_change(initial, "reg_40", timeout=4.0) + + switch = find_entity(entities, "invert_switch", SwitchInfo) + assert switch is not None, "invert_switch not found" + + client.switch_command(switch.key, True) + # The wire byte carries the inverted value... + await tracker.await_change(wrote_on, "reg_40", timeout=4.0) + # ...while the entity reports the requested state. Switch states are deduped, so this relies on + # wait_for_state's fresh subscribe_states re-dumping every entity's current state. + await wait_for_state( + client, + lambda s: ( + getattr(s, "key", None) == switch.key + and getattr(s, "state", None) is True + ), + timeout=6.0, + ) + + client.switch_command(switch.key, False) + await tracker.await_change(wrote_off, "reg_40", timeout=4.0) + await wait_for_state( + client, + lambda s: ( + getattr(s, "key", None) == switch.key + and getattr(s, "state", None) is False + ), + timeout=6.0, + ) + + @pytest.mark.asyncio async def test_uart_mock_modbus_deprecated_write_buffer( yaml_config: str,