From 0ff5270632f59a8fd4d7ed25cd71c2ce92ae5b07 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 4 Mar 2026 16:57:19 -1000 Subject: [PATCH 1/6] [ci] Fix codeowner approval label workflow for fork PRs (#14490) Co-authored-by: Claude Opus 4.6 --- .github/scripts/codeowners.js | 88 +++++++++- .../codeowner-approved-label-update.yml | 95 +++++++++++ .../workflows/codeowner-approved-label.yml | 153 +++++------------- 3 files changed, 217 insertions(+), 119 deletions(-) create mode 100644 .github/workflows/codeowner-approved-label-update.yml diff --git a/.github/scripts/codeowners.js b/.github/scripts/codeowners.js index 9a10391699..5d69c11b1a 100644 --- a/.github/scripts/codeowners.js +++ b/.github/scripts/codeowners.js @@ -2,7 +2,7 @@ // // Used by: // - codeowner-review-request.yml -// - codeowner-approved-label.yml +// - codeowner-approved-label.yml + codeowner-approved-label-update.yml // - auto-label-pr/detectors.js (detectCodeOwner) /** @@ -133,11 +133,95 @@ function loadCodeowners(repoRoot = '.') { return parseCodeowners(content); } +/** Possible label actions returned by determineLabelAction. */ +const LabelAction = Object.freeze({ + ADD: 'add', + REMOVE: 'remove', + NONE: 'none', +}); + +/** + * Determine what label action is needed for a PR based on codeowner approvals. + * + * Checks changed files against CODEOWNERS patterns, reviews, and current labels + * to decide if the label should be added, removed, or left unchanged. + * + * @param {object} github - octokit instance from actions/github-script + * @param {string} owner - repo owner + * @param {string} repo - repo name + * @param {number} pr_number - pull request number + * @param {Array} codeownersPatterns - from loadCodeowners / fetchCodeowners + * @param {string} labelName - label to manage + * @returns {Promise} + */ +async function determineLabelAction(github, owner, repo, pr_number, codeownersPatterns, labelName) { + // Get the list of changed files in this PR + const prFiles = await github.paginate( + github.rest.pulls.listFiles, + { owner, repo, pull_number: pr_number } + ); + + const changedFiles = prFiles.map(file => file.filename); + console.log(`Found ${changedFiles.length} changed files`); + + if (changedFiles.length === 0) { + console.log('No changed files found'); + return LabelAction.NONE; + } + + // Get effective owners using last-match-wins semantics + const effective = getEffectiveOwners(changedFiles, codeownersPatterns); + const componentCodeowners = effective.users; + + console.log(`Component-specific codeowners: ${Array.from(componentCodeowners).join(', ') || '(none)'}`); + + // Get current labels + const { data: currentLabels } = await github.rest.issues.listLabelsOnIssue({ + owner, repo, issue_number: pr_number + }); + const hasLabel = currentLabels.some(label => label.name === labelName); + + if (componentCodeowners.size === 0) { + console.log('No component-specific codeowners found'); + return hasLabel ? LabelAction.REMOVE : LabelAction.NONE; + } + + // Get all reviews and find latest per user + const reviews = await github.paginate( + github.rest.pulls.listReviews, + { owner, repo, pull_number: pr_number } + ); + + const latestReviewByUser = new Map(); + for (const review of reviews) { + if (!review.user || review.user.type === 'Bot' || review.state === 'COMMENTED') continue; + latestReviewByUser.set(review.user.login, review); + } + + // Check if any component-specific codeowner has an active approval + let hasCodeownerApproval = false; + for (const [login, review] of latestReviewByUser) { + if (review.state === 'APPROVED' && componentCodeowners.has(login)) { + console.log(`Codeowner '${login}' has approved`); + hasCodeownerApproval = true; + break; + } + } + + if (hasCodeownerApproval && !hasLabel) return LabelAction.ADD; + if (!hasCodeownerApproval && hasLabel) return LabelAction.REMOVE; + + console.log(`Label already ${hasLabel ? 'present' : 'absent'}, no change needed`); + return LabelAction.NONE; +} + module.exports = { globToRegex, parseCodeowners, fetchCodeowners, loadCodeowners, classifyOwners, - getEffectiveOwners + getEffectiveOwners, + LabelAction, + determineLabelAction }; diff --git a/.github/workflows/codeowner-approved-label-update.yml b/.github/workflows/codeowner-approved-label-update.yml new file mode 100644 index 0000000000..9168cce1d6 --- /dev/null +++ b/.github/workflows/codeowner-approved-label-update.yml @@ -0,0 +1,95 @@ +# Fallback for fork PRs: phase 1 (codeowner-approved-label.yml) handles +# non-fork PRs directly but can't write labels on fork PRs (read-only token). +# This workflow re-determines the action and applies it if needed. + +name: Codeowner Approved Label Update + +on: + workflow_run: + workflows: ["Codeowner Approved Label"] + types: [completed] + +permissions: + issues: write + pull-requests: read + contents: read + +jobs: + update-label: + name: Run + if: > + github.event.workflow_run.conclusion == 'success' && + github.event.workflow_run.event == 'pull_request_review' + runs-on: ubuntu-latest + steps: + - name: Get PR details + id: pr + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + REPO: ${{ github.repository }} + run: | + pr_data=$(gh pr list --repo "$REPO" --state open --search "$HEAD_SHA" \ + --json number,baseRefName --jq '.[0] // empty') + + if [ -z "$pr_data" ]; then + echo "No open PR found for SHA $HEAD_SHA, skipping" + echo "skip=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + pr_number=$(echo "$pr_data" | jq -r '.number') + base_ref=$(echo "$pr_data" | jq -r '.baseRefName') + + echo "pr_number=$pr_number" >> "$GITHUB_OUTPUT" + echo "base_ref=$base_ref" >> "$GITHUB_OUTPUT" + echo "Found PR #$pr_number targeting $base_ref" + + - name: Checkout base repository + if: steps.pr.outputs.skip != 'true' + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + repository: ${{ github.repository }} + ref: ${{ steps.pr.outputs.base_ref }} + sparse-checkout: | + .github/scripts/codeowners.js + CODEOWNERS + + - name: Update label + if: steps.pr.outputs.skip != 'true' + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + PR_NUMBER: ${{ steps.pr.outputs.pr_number }} + with: + script: | + const { loadCodeowners, determineLabelAction, LabelAction } = require('./.github/scripts/codeowners.js'); + + const owner = context.repo.owner; + const repo = context.repo.repo; + const pr_number = parseInt(process.env.PR_NUMBER, 10); + const LABEL_NAME = 'code-owner-approved'; + + console.log(`Processing PR #${pr_number} for codeowner approval label`); + + const codeownersPatterns = loadCodeowners(); + const action = await determineLabelAction( + github, owner, repo, pr_number, codeownersPatterns, LABEL_NAME + ); + + if (action === LabelAction.ADD) { + await github.rest.issues.addLabels({ + owner, repo, issue_number: pr_number, labels: [LABEL_NAME] + }); + console.log(`Added '${LABEL_NAME}' label`); + } else if (action === LabelAction.REMOVE) { + try { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pr_number, name: LABEL_NAME + }); + console.log(`Removed '${LABEL_NAME}' label`); + } catch (error) { + if (error.status !== 404) throw error; + } + } else { + console.log('No label change needed'); + } diff --git a/.github/workflows/codeowner-approved-label.yml b/.github/workflows/codeowner-approved-label.yml index 200f18f544..12199bd0b0 100644 --- a/.github/workflows/codeowner-approved-label.yml +++ b/.github/workflows/codeowner-approved-label.yml @@ -1,9 +1,9 @@ -# This workflow adds/removes a 'code-owner-approved' label when a -# component-specific codeowner approves (or dismisses) a PR. -# This helps maintainers prioritize PRs that have codeowner sign-off. +# Adds/removes a 'code-owner-approved' label when a component-specific +# codeowner approves (or dismisses) a PR. # -# Only component-specific codeowners count — the catch-all @esphome/core -# team is excluded so the label reflects domain-expert approval. +# Handles non-fork PRs directly. For fork PRs the GITHUB_TOKEN is read-only, +# so label writes are deferred to codeowner-approved-label-update.yml which +# triggers via workflow_run with write permissions. name: Codeowner Approved Label @@ -26,134 +26,53 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: ref: ${{ github.event.pull_request.base.sha }} + sparse-checkout: | + .github/scripts/codeowners.js + CODEOWNERS - name: Check codeowner approval and update label uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + PR_NUMBER: ${{ github.event.pull_request.number }} with: script: | - const { loadCodeowners, getEffectiveOwners } = require('./.github/scripts/codeowners.js'); + const { loadCodeowners, determineLabelAction, LabelAction } = require('./.github/scripts/codeowners.js'); const owner = context.repo.owner; const repo = context.repo.repo; - const pr_number = context.payload.pull_request.number; + const pr_number = parseInt(process.env.PR_NUMBER, 10); const LABEL_NAME = 'code-owner-approved'; console.log(`Processing PR #${pr_number} for codeowner approval label`); + const codeownersPatterns = loadCodeowners(); + const action = await determineLabelAction( + github, owner, repo, pr_number, codeownersPatterns, LABEL_NAME + ); + + if (action === LabelAction.NONE) { + console.log('No label change needed'); + return; + } + try { - // Get the list of changed files in this PR (with pagination) - const prFiles = await github.paginate( - github.rest.pulls.listFiles, - { - owner, - repo, - pull_number: pr_number - } - ); - - const changedFiles = prFiles.map(file => file.filename); - console.log(`Found ${changedFiles.length} changed files`); - - if (changedFiles.length === 0) { - console.log('No changed files found, skipping'); - return; - } - - // Parse CODEOWNERS from the checked-out base branch - const codeownersPatterns = loadCodeowners(); - - // Get effective owners using last-match-wins semantics - const effective = getEffectiveOwners(changedFiles, codeownersPatterns); - - // Only keep individual component-specific codeowners (exclude teams) - const componentCodeowners = effective.users; - - console.log(`Component-specific codeowners for changed files: ${Array.from(componentCodeowners).join(', ') || '(none)'}`); - - if (componentCodeowners.size === 0) { - console.log('No component-specific codeowners found for changed files'); - // Remove label if present since there are no component codeowners - try { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: pr_number, - name: LABEL_NAME - }); - console.log(`Removed '${LABEL_NAME}' label (no component codeowners)`); - } catch (error) { - if (error.status !== 404) { - console.log(`Failed to remove label: ${error.message}`); - } - } - return; - } - - // Get all reviews on the PR - const reviews = await github.paginate( - github.rest.pulls.listReviews, - { - owner, - repo, - pull_number: pr_number - } - ); - - // Get the latest review per user (reviews are returned chronologically) - const latestReviewByUser = new Map(); - for (const review of reviews) { - // Skip bot reviews and comment-only reviews - if (!review.user || review.user.type === 'Bot' || review.state === 'COMMENTED') continue; - latestReviewByUser.set(review.user.login, review); - } - - // Check if any component-specific codeowner has an active approval - let hasCodeownerApproval = false; - for (const [login, review] of latestReviewByUser) { - if (review.state === 'APPROVED' && componentCodeowners.has(login)) { - console.log(`Codeowner '${login}' has approved`); - hasCodeownerApproval = true; - break; - } - } - - // Get current labels to check if label is already present - const { data: currentLabels } = await github.rest.issues.listLabelsOnIssue({ - owner, - repo, - issue_number: pr_number - }); - const hasLabel = currentLabels.some(label => label.name === LABEL_NAME); - - if (hasCodeownerApproval && !hasLabel) { - // Add the label + if (action === LabelAction.ADD) { await github.rest.issues.addLabels({ - owner, - repo, - issue_number: pr_number, - labels: [LABEL_NAME] + owner, repo, issue_number: pr_number, labels: [LABEL_NAME] }); console.log(`Added '${LABEL_NAME}' label`); - } else if (!hasCodeownerApproval && hasLabel) { - // Remove the label - try { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: pr_number, - name: LABEL_NAME - }); - console.log(`Removed '${LABEL_NAME}' label`); - } catch (error) { - if (error.status !== 404) { - console.log(`Failed to remove label: ${error.message}`); - } - } - } else { - console.log(`Label already ${hasLabel ? 'present' : 'absent'}, no change needed`); + } else if (action === LabelAction.REMOVE) { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pr_number, name: LABEL_NAME + }); + console.log(`Removed '${LABEL_NAME}' label`); } - } catch (error) { - console.error(error); - core.setFailed(`Failed to process codeowner approval label: ${error.message}`); + if (error.status === 403) { + console.log('Fork PR: deferring label write to phase 2 workflow'); + } else if (error.status === 404) { + console.log('Label already removed'); + } else { + throw error; + } } From f5c37bf486d1a8905b7518478fec23a2e6e3b9ae Mon Sep 17 00:00:00 2001 From: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com> Date: Thu, 5 Mar 2026 14:24:01 +1100 Subject: [PATCH 2/6] [packet_transport] Minimise heap allocations (#14482) --- .../packet_transport/packet_transport.cpp | 79 ++-- .../packet_transport/packet_transport.h | 19 +- script/cpp_unit_test.py | 44 +- tests/components/packet_transport/common.h | 98 ++++ .../components/packet_transport/cpp_test.yaml | 11 + .../packet_transport_test.cpp | 445 ++++++++++++++++++ 6 files changed, 659 insertions(+), 37 deletions(-) create mode 100644 tests/components/packet_transport/common.h create mode 100644 tests/components/packet_transport/cpp_test.yaml create mode 100644 tests/components/packet_transport/packet_transport_test.cpp diff --git a/esphome/components/packet_transport/packet_transport.cpp b/esphome/components/packet_transport/packet_transport.cpp index d2c5920001..f241cc6114 100644 --- a/esphome/components/packet_transport/packet_transport.cpp +++ b/esphome/components/packet_transport/packet_transport.cpp @@ -3,6 +3,8 @@ #include "esphome/core/helpers.h" #include "packet_transport.h" +#include + #include "esphome/components/xxtea/xxtea.h" namespace esphome { @@ -77,7 +79,7 @@ enum DecodeResult { DECODE_EMPTY, }; -static const size_t MAX_PING_KEYS = 4; +static constexpr size_t MAX_PING_KEYS = 4; static inline void add(std::vector &vec, uint32_t data) { vec.push_back(data & 0xFF); @@ -168,7 +170,7 @@ class PacketDecoder { return true; } - bool decrypt(const uint32_t *key) { + bool decrypt(const uint32_t *key) const { if (this->get_remaining_size() % 4 != 0) { return false; } @@ -249,9 +251,9 @@ void PacketTransport::init_data_() { } else { add(this->data_, DATA_KEY); } - for (const auto &pkey : this->ping_keys_) { + for (auto &value : this->ping_keys_ | std::views::values) { add(this->data_, PING_KEY); - add(this->data_, pkey.second); + add(this->data_, value); } } @@ -331,7 +333,7 @@ void PacketTransport::update() { auto now = millis() / 1000; if (this->last_key_time_ + this->ping_pong_recyle_time_ < now) { this->resend_ping_key_ = this->ping_pong_enable_; - ESP_LOGV(TAG, "Ping request, age %u", now - this->last_key_time_); + ESP_LOGV(TAG, "Ping request, age %" PRIu32, now - this->last_key_time_); this->last_key_time_ = now; } for (const auto &provider : this->providers_) { @@ -339,24 +341,32 @@ void PacketTransport::update() { if (key_response_age > (this->ping_pong_recyle_time_ * 2u)) { #ifdef USE_STATUS_SENSOR if (provider.second.status_sensor != nullptr && provider.second.status_sensor->state) { - ESP_LOGI(TAG, "Ping status for %s timeout at %u with age %u", provider.first.c_str(), now, key_response_age); + ESP_LOGI(TAG, "Ping status for %s timeout at %" PRIu32 " with age %" PRIu32, provider.first.c_str(), now, + key_response_age); provider.second.status_sensor->publish_state(false); } #endif #ifdef USE_SENSOR - for (auto &sensor : this->remote_sensors_[provider.first]) { - sensor.second->publish_state(NAN); + auto it = this->remote_sensors_.find(provider.first); + if (it != this->remote_sensors_.end()) { + for (auto &val : it->second | std::views::values) { + val->publish_state(NAN); + } } #endif #ifdef USE_BINARY_SENSOR - for (auto &sensor : this->remote_binary_sensors_[provider.first]) { - sensor.second->invalidate_state(); + auto bs_it = this->remote_binary_sensors_.find(provider.first); + if (bs_it != this->remote_binary_sensors_.end()) { + for (auto &val : bs_it->second | std::views::values) { + val->invalidate_state(); + } } #endif } else { #ifdef USE_STATUS_SENSOR if (provider.second.status_sensor != nullptr && !provider.second.status_sensor->state) { - ESP_LOGI(TAG, "Ping status for %s restored at %u with age %u", provider.first.c_str(), now, key_response_age); + ESP_LOGI(TAG, "Ping status for %s restored at %" PRIu32 " with age %" PRIu32, provider.first.c_str(), now, + key_response_age); provider.second.status_sensor->publish_state(true); } #endif @@ -367,11 +377,16 @@ void PacketTransport::update() { void PacketTransport::add_key_(const char *name, uint32_t key) { if (!this->is_encrypted_()) return; - if (this->ping_keys_.count(name) == 0 && this->ping_keys_.size() == MAX_PING_KEYS) { - ESP_LOGW(TAG, "Ping key from %s discarded", name); - return; + auto it = this->ping_keys_.find(name); + if (it == this->ping_keys_.end()) { + if (this->ping_keys_.size() == MAX_PING_KEYS) { + ESP_LOGW(TAG, "Ping key from %s discarded", name); + return; + } + this->ping_keys_.emplace(name, key); // allocates string key once only + } else { + it->second = key; // key string already exists in map, no allocation } - this->ping_keys_[name] = key; this->updated_ = true; ESP_LOGV(TAG, "Ping key from %s now %X", name, (unsigned) key); } @@ -431,17 +446,19 @@ void PacketTransport::process_(std::span data) { return; } - if (this->providers_.count(namebuf) == 0) { + auto it = this->providers_.find(namebuf); + if (it == this->providers_.end()) { ESP_LOGVV(TAG, "Unknown hostname %s", namebuf); return; } + auto &provider = it->second; ESP_LOGV(TAG, "Found hostname %s", namebuf); #ifdef USE_SENSOR - auto &sensors = this->remote_sensors_[namebuf]; + auto &sensors = this->remote_sensors_.try_emplace(namebuf).first->second; #endif #ifdef USE_BINARY_SENSOR - auto &binary_sensors = this->remote_binary_sensors_[namebuf]; + auto &binary_sensors = this->remote_binary_sensors_.try_emplace(namebuf).first->second; #endif if (!decoder.bump_to(4)) { @@ -453,7 +470,6 @@ void PacketTransport::process_(std::span data) { return; } - auto &provider = this->providers_[namebuf]; // if encryption not used with this host, ping check is pointless since it would be easily spoofed. if (provider.encryption_key.empty()) ping_key_seen = true; @@ -495,16 +511,19 @@ void PacketTransport::process_(std::span data) { if (decoder.decode(BINARY_SENSOR_KEY, namebuf, sizeof(namebuf), byte) == DECODE_OK) { ESP_LOGV(TAG, "Got binary sensor %s %d", namebuf, byte); #ifdef USE_BINARY_SENSOR - if (binary_sensors.count(namebuf) != 0) - binary_sensors[namebuf]->publish_state(byte != 0); + auto bs = binary_sensors.find(namebuf); + if (bs != binary_sensors.end()) { + bs->second->publish_state(byte != 0); + } #endif continue; } if (decoder.decode(SENSOR_KEY, namebuf, sizeof(namebuf), rdata.u32) == DECODE_OK) { ESP_LOGV(TAG, "Got sensor %s %f", namebuf, rdata.f32); #ifdef USE_SENSOR - if (sensors.count(namebuf) != 0) - sensors[namebuf]->publish_state(rdata.f32); + auto sensor_it = sensors.find(namebuf); + if (sensor_it != sensors.end()) + sensor_it->second->publish_state(rdata.f32); #endif continue; } @@ -537,12 +556,18 @@ void PacketTransport::dump_config() { ESP_LOGCONFIG(TAG, " Remote host: %s", host.first.c_str()); ESP_LOGCONFIG(TAG, " Encrypted: %s", YESNO(!host.second.encryption_key.empty())); #ifdef USE_SENSOR - for (const auto &sensor : this->remote_sensors_[host.first.c_str()]) - ESP_LOGCONFIG(TAG, " Sensor: %s", sensor.first.c_str()); + auto rs = this->remote_sensors_.find(host.first.c_str()); + if (rs != this->remote_sensors_.end()) { + for (const auto &key : rs->second | std::views::keys) + ESP_LOGCONFIG(TAG, " Sensor: %s", key.c_str()); + } #endif #ifdef USE_BINARY_SENSOR - for (const auto &sensor : this->remote_binary_sensors_[host.first.c_str()]) - ESP_LOGCONFIG(TAG, " Binary Sensor: %s", sensor.first.c_str()); + auto rbs = this->remote_binary_sensors_.find(host.first.c_str()); + if (rbs != this->remote_binary_sensors_.end()) { + for (const auto &key : rbs->second | std::views::keys) + ESP_LOGCONFIG(TAG, " Binary Sensor: %s", key.c_str()); + } #endif } } diff --git a/esphome/components/packet_transport/packet_transport.h b/esphome/components/packet_transport/packet_transport.h index a236744231..b3798738e2 100644 --- a/esphome/components/packet_transport/packet_transport.h +++ b/esphome/components/packet_transport/packet_transport.h @@ -24,6 +24,9 @@ namespace esphome { namespace packet_transport { +// std::less provides allocation-free comparison with const char * +template using string_map_t = std::map>; + struct Provider { std::vector encryption_key; const char *name; @@ -79,15 +82,15 @@ class PacketTransport : public PollingComponent { #endif void add_provider(const char *hostname) { - if (this->providers_.count(hostname) == 0) { + if (!this->providers_.contains(hostname)) { Provider provider{}; provider.name = hostname; this->providers_[hostname] = provider; #ifdef USE_SENSOR - this->remote_sensors_[hostname] = std::map(); + this->remote_sensors_[hostname] = string_map_t(); #endif #ifdef USE_BINARY_SENSOR - this->remote_binary_sensors_[hostname] = std::map(); + this->remote_binary_sensors_[hostname] = string_map_t(); #endif } } @@ -139,23 +142,23 @@ class PacketTransport : public PollingComponent { #ifdef USE_SENSOR std::vector sensors_{}; - std::map> remote_sensors_{}; + string_map_t> remote_sensors_{}; #endif #ifdef USE_BINARY_SENSOR std::vector binary_sensors_{}; - std::map> remote_binary_sensors_{}; + string_map_t> remote_binary_sensors_{}; #endif - std::map providers_{}; + string_map_t providers_{}; std::vector ping_header_{}; std::vector header_{}; std::vector data_{}; - std::map ping_keys_{}; + string_map_t ping_keys_{}; const char *platform_name_{""}; void add_key_(const char *name, uint32_t key); void send_ping_pong_request_(); - inline bool is_encrypted_() { return !this->encryption_key_.empty(); } + bool is_encrypted_() const { return !this->encryption_key_.empty(); } }; } // namespace packet_transport diff --git a/script/cpp_unit_test.py b/script/cpp_unit_test.py index 02b133060a..b87261ab33 100755 --- a/script/cpp_unit_test.py +++ b/script/cpp_unit_test.py @@ -12,6 +12,7 @@ from esphome.__main__ import command_compile, parse_args from esphome.config import validate_config from esphome.core import CORE from esphome.platformio_api import get_idedata +from esphome.yaml_util import load_yaml # This must coincide with the version in /platformio.ini PLATFORMIO_GOOGLE_TEST_LIB = "google/googletest@^1.15.2" @@ -44,6 +45,38 @@ def filter_components_without_tests(components: list[str]) -> list[str]: return filtered_components +# Name of optional per-component YAML config merged into the test build +# before validation so that platform defines (USE_SENSOR, etc.) are generated. +CPP_TEST_CONFIG_FILE = "cpp_test.yaml" + + +def load_component_test_configs(components: list[str]) -> dict: + """Load cpp_test.yaml files from test component directories. + + These configs are merged into the base test config *before* validation + so that entity registration runs during code generation, which causes + the corresponding USE_* defines to be emitted. + """ + merged: dict = {} + for component in components: + config_file = COMPONENTS_TESTS_DIR / component / CPP_TEST_CONFIG_FILE + if not config_file.exists(): + continue + component_config = load_yaml(config_file) + if not component_config: + continue + for key, value in component_config.items(): + if ( + key in merged + and isinstance(merged[key], list) + and isinstance(value, list) + ): + merged[key].extend(value) + else: + merged[key] = value + return merged + + def create_test_config(config_name: str, includes: list[str]) -> dict: """Create ESPHome test configuration for C++ unit tests. @@ -115,6 +148,11 @@ def run_tests(selected_components: list[str]) -> int: config = create_test_config(config_name, includes) + # Merge component-specific test configs (e.g. sensor instances) before + # validation so that entity registration and USE_* defines work. + extra_config = load_component_test_configs(components) + config.update(extra_config) + CORE.config_path = COMPONENTS_TESTS_DIR / "dummy.yaml" CORE.dashboard = None @@ -122,8 +160,10 @@ def run_tests(selected_components: list[str]) -> int: config = validate_config(config, {}) # Add all components and dependencies to the base configuration after validation, so their files - # are added to the build. - config.update({key: {} for key in components_with_dependencies}) + # are added to the build. Use setdefault to avoid overwriting entries that were + # already validated (e.g. sensor instances from cpp_test.yaml). + for key in components_with_dependencies: + config.setdefault(key, {}) print(f"Testing components: {', '.join(components)}") CORE.config = config diff --git a/tests/components/packet_transport/common.h b/tests/components/packet_transport/common.h new file mode 100644 index 0000000000..f8caa7bb68 --- /dev/null +++ b/tests/components/packet_transport/common.h @@ -0,0 +1,98 @@ +#pragma once +#include +#include +#include +#include +#include +#include "esphome/components/packet_transport/packet_transport.h" + +namespace esphome::packet_transport::testing { + +// Protocol constants mirrored from packet_transport.cpp for test packet construction. +static constexpr uint16_t MAGIC_NUMBER = 0x4553; +static constexpr uint16_t MAGIC_PING = 0x5048; + +// Concrete testable implementation of PacketTransport. +// Captures sent packets and exposes protected members for verification. +// +// Sensor round-trip tests require USE_SENSOR / USE_BINARY_SENSOR to be defined, +// which happens when 'sensor' and 'binary_sensor' components are in the build. +// Run with --all or include those components to enable the full test suite. +class TestablePacketTransport : public PacketTransport { + public: + using PacketTransport::add_key_; + using PacketTransport::data_; + using PacketTransport::encryption_key_; + using PacketTransport::flush_; + using PacketTransport::header_; + using PacketTransport::increment_code_; + using PacketTransport::init_data_; + using PacketTransport::is_encrypted_; + using PacketTransport::is_provider_; + using PacketTransport::name_; + using PacketTransport::ping_key_; + using PacketTransport::ping_keys_; + using PacketTransport::ping_pong_enable_; + using PacketTransport::ping_pong_recyle_time_; + using PacketTransport::process_; + using PacketTransport::providers_; + using PacketTransport::rolling_code_; + using PacketTransport::rolling_code_enable_; + using PacketTransport::send_data_; + using PacketTransport::updated_; +#ifdef USE_SENSOR + using PacketTransport::add_data_; + using PacketTransport::remote_sensors_; + using PacketTransport::sensors_; +#endif +#ifdef USE_BINARY_SENSOR + using PacketTransport::add_binary_data_; + using PacketTransport::binary_sensors_; + using PacketTransport::remote_binary_sensors_; +#endif + + // NOTE: std::vector is used here for test convenience. For production code, + // consider using StaticVector or FixedVector from esphome/core/helpers.h instead. + mutable std::vector> sent_packets; + size_t max_packet_size{512}; + bool send_enabled{true}; + + void send_packet(const std::vector &buf) const override { this->sent_packets.push_back(buf); } + size_t get_max_packet_size() override { return this->max_packet_size; } + bool should_send() override { return this->send_enabled; } + + /// Build the packet header for testing without requiring App or global_preferences. + void init_for_test(const char *name) { + this->name_ = name; + this->header_.clear(); + // MAGIC_NUMBER as uint16_t little-endian + this->header_.push_back(MAGIC_NUMBER & 0xFF); + this->header_.push_back((MAGIC_NUMBER >> 8) & 0xFF); + // Length-prefixed hostname + auto len = strlen(name); + this->header_.push_back(static_cast(len)); + for (size_t i = 0; i < len; i++) + this->header_.push_back(name[i]); + // Pad to 4-byte boundary + while (this->header_.size() & 0x3) + this->header_.push_back(0); + } +}; + +/// Build a MAGIC_PING packet for testing add_key_ / ping-pong flows. +inline std::vector build_ping_packet(const char *hostname, uint32_t key) { + std::vector packet; + packet.push_back(MAGIC_PING & 0xFF); + packet.push_back((MAGIC_PING >> 8) & 0xFF); + auto len = strlen(hostname); + packet.push_back(static_cast(len)); + for (size_t i = 0; i < len; i++) + packet.push_back(hostname[i]); + packet.push_back(key & 0xFF); + packet.push_back((key >> 8) & 0xFF); + packet.push_back((key >> 16) & 0xFF); + packet.push_back((key >> 24) & 0xFF); + return packet; +} + +} // namespace esphome::packet_transport::testing diff --git a/tests/components/packet_transport/cpp_test.yaml b/tests/components/packet_transport/cpp_test.yaml new file mode 100644 index 0000000000..fa39df3c0a --- /dev/null +++ b/tests/components/packet_transport/cpp_test.yaml @@ -0,0 +1,11 @@ +# Extra component configuration required by C++ unit tests. +# Loaded by cpp_unit_test.py and merged into the test build config +# before validation, so that platform defines (USE_SENSOR, etc.) are generated. + +sensor: + - platform: template + id: test_cpp_sensor + +binary_sensor: + - platform: template + id: test_cpp_binary_sensor diff --git a/tests/components/packet_transport/packet_transport_test.cpp b/tests/components/packet_transport/packet_transport_test.cpp new file mode 100644 index 0000000000..d8f11ca607 --- /dev/null +++ b/tests/components/packet_transport/packet_transport_test.cpp @@ -0,0 +1,445 @@ +#include "common.h" + +namespace esphome::packet_transport::testing { + +// --- Configuration setter tests --- + +TEST(PacketTransportTest, SetIsProvider) { + TestablePacketTransport transport; + transport.set_is_provider(true); + EXPECT_TRUE(transport.is_provider_); +} + +TEST(PacketTransportTest, SetEncryptionKey) { + TestablePacketTransport transport; + std::vector key(32, 0xAB); + transport.set_encryption_key(key); + EXPECT_EQ(transport.encryption_key_, key); + EXPECT_TRUE(transport.is_encrypted_()); +} + +TEST(PacketTransportTest, NoEncryptionByDefault) { + TestablePacketTransport transport; + EXPECT_FALSE(transport.is_encrypted_()); +} + +TEST(PacketTransportTest, SetRollingCodeEnable) { + TestablePacketTransport transport; + transport.set_rolling_code_enable(true); + EXPECT_TRUE(transport.rolling_code_enable_); +} + +TEST(PacketTransportTest, SetPingPongEnable) { + TestablePacketTransport transport; + transport.set_ping_pong_enable(true); + EXPECT_TRUE(transport.ping_pong_enable_); +} + +TEST(PacketTransportTest, SetPingPongRecycleTime) { + TestablePacketTransport transport; + transport.set_ping_pong_recycle_time(600); + EXPECT_EQ(transport.ping_pong_recyle_time_, 600u); +} + +// --- Provider management --- + +TEST(PacketTransportTest, AddProvider) { + TestablePacketTransport transport; + transport.add_provider("host1"); + EXPECT_TRUE(transport.providers_.contains("host1")); + EXPECT_EQ(transport.providers_.size(), 1u); +} + +TEST(PacketTransportTest, AddProviderDuplicate) { + TestablePacketTransport transport; + transport.add_provider("host1"); + transport.add_provider("host1"); + EXPECT_EQ(transport.providers_.size(), 1u); +} + +TEST(PacketTransportTest, SetProviderEncryption) { + TestablePacketTransport transport; + transport.add_provider("host1"); + std::vector key(32, 0xCD); + transport.set_provider_encryption("host1", key); + EXPECT_EQ(transport.providers_["host1"].encryption_key, key); +} + +// --- Sensor management (requires USE_SENSOR / USE_BINARY_SENSOR) --- + +#ifdef USE_SENSOR +TEST(PacketTransportTest, AddSensor) { + TestablePacketTransport transport; + sensor::Sensor s; + transport.add_sensor("temp", &s); + ASSERT_EQ(transport.sensors_.size(), 1u); + EXPECT_STREQ(transport.sensors_[0].id, "temp"); + EXPECT_EQ(transport.sensors_[0].sensor, &s); + EXPECT_TRUE(transport.sensors_[0].updated); +} + +TEST(PacketTransportTest, AddRemoteSensor) { + TestablePacketTransport transport; + sensor::Sensor s; + transport.add_remote_sensor("host1", "remote_temp", &s); + EXPECT_TRUE(transport.providers_.contains("host1")); + EXPECT_EQ(transport.remote_sensors_["host1"]["remote_temp"], &s); +} +#endif + +#ifdef USE_BINARY_SENSOR +TEST(PacketTransportTest, AddBinarySensor) { + TestablePacketTransport transport; + binary_sensor::BinarySensor bs; + transport.add_binary_sensor("motion", &bs); + ASSERT_EQ(transport.binary_sensors_.size(), 1u); + EXPECT_STREQ(transport.binary_sensors_[0].id, "motion"); + EXPECT_EQ(transport.binary_sensors_[0].sensor, &bs); +} + +TEST(PacketTransportTest, AddRemoteBinarySensor) { + TestablePacketTransport transport; + binary_sensor::BinarySensor bs; + transport.add_remote_binary_sensor("host1", "remote_motion", &bs); + EXPECT_TRUE(transport.providers_.contains("host1")); + EXPECT_EQ(transport.remote_binary_sensors_["host1"]["remote_motion"], &bs); +} +#endif + +// --- Unencrypted round-trip tests (require USE_SENSOR / USE_BINARY_SENSOR) --- + +#ifdef USE_SENSOR +TEST(PacketTransportTest, UnencryptedSensorRoundTrip) { + // Encoder + TestablePacketTransport encoder; + encoder.init_for_test("sender"); + sensor::Sensor local_sensor; + local_sensor.state = 42.5f; + encoder.add_sensor("temp", &local_sensor); + + encoder.send_data_(true); + ASSERT_EQ(encoder.sent_packets.size(), 1u); + + // Decoder + TestablePacketTransport decoder; + decoder.init_for_test("receiver"); + sensor::Sensor remote_sensor; + remote_sensor.state = -999.0f; // sentinel + decoder.add_remote_sensor("sender", "temp", &remote_sensor); + + auto &packet = encoder.sent_packets[0]; + decoder.process_({packet.data(), packet.size()}); + EXPECT_FLOAT_EQ(remote_sensor.state, 42.5f); +} +#endif + +#ifdef USE_BINARY_SENSOR +TEST(PacketTransportTest, UnencryptedBinarySensorRoundTrip) { + TestablePacketTransport encoder; + encoder.init_for_test("sender"); + binary_sensor::BinarySensor local_bs; + local_bs.state = true; + encoder.add_binary_sensor("motion", &local_bs); + + encoder.send_data_(true); + ASSERT_EQ(encoder.sent_packets.size(), 1u); + + TestablePacketTransport decoder; + decoder.init_for_test("receiver"); + binary_sensor::BinarySensor remote_bs; + decoder.add_remote_binary_sensor("sender", "motion", &remote_bs); + + auto &packet = encoder.sent_packets[0]; + decoder.process_({packet.data(), packet.size()}); + EXPECT_TRUE(remote_bs.state); +} +#endif + +#if defined(USE_SENSOR) && defined(USE_BINARY_SENSOR) +TEST(PacketTransportTest, MultipleSensorsRoundTrip) { + TestablePacketTransport encoder; + encoder.init_for_test("sender"); + + sensor::Sensor s1, s2; + s1.state = 10.0f; + s2.state = 20.0f; + encoder.add_sensor("s1", &s1); + encoder.add_sensor("s2", &s2); + + binary_sensor::BinarySensor bs1; + bs1.state = true; + encoder.add_binary_sensor("bs1", &bs1); + + encoder.send_data_(true); + ASSERT_EQ(encoder.sent_packets.size(), 1u); + + TestablePacketTransport decoder; + decoder.init_for_test("receiver"); + sensor::Sensor rs1, rs2; + binary_sensor::BinarySensor rbs1; + rs1.state = -999.0f; + rs2.state = -999.0f; + decoder.add_remote_sensor("sender", "s1", &rs1); + decoder.add_remote_sensor("sender", "s2", &rs2); + decoder.add_remote_binary_sensor("sender", "bs1", &rbs1); + + auto &packet = encoder.sent_packets[0]; + decoder.process_({packet.data(), packet.size()}); + + EXPECT_FLOAT_EQ(rs1.state, 10.0f); + EXPECT_FLOAT_EQ(rs2.state, 20.0f); + EXPECT_TRUE(rbs1.state); +} +#endif + +// --- Encrypted round-trip --- + +#ifdef USE_SENSOR +TEST(PacketTransportTest, EncryptedSensorRoundTrip) { + std::vector key(32); + for (int i = 0; i < 32; i++) + key[i] = i; + + TestablePacketTransport encoder; + encoder.init_for_test("sender"); + encoder.set_encryption_key(key); + sensor::Sensor local_sensor; + local_sensor.state = 99.9f; + encoder.add_sensor("temp", &local_sensor); + + encoder.send_data_(true); + ASSERT_EQ(encoder.sent_packets.size(), 1u); + + TestablePacketTransport decoder; + decoder.init_for_test("receiver"); + sensor::Sensor remote_sensor; + remote_sensor.state = -999.0f; + decoder.add_remote_sensor("sender", "temp", &remote_sensor); + decoder.set_provider_encryption("sender", key); + + auto &packet = encoder.sent_packets[0]; + decoder.process_({packet.data(), packet.size()}); + EXPECT_FLOAT_EQ(remote_sensor.state, 99.9f); +} + +// --- Selective send --- + +TEST(PacketTransportTest, SendDataOnlyUpdated) { + TestablePacketTransport encoder; + encoder.init_for_test("sender"); + + sensor::Sensor s1, s2; + s1.state = 1.0f; + s2.state = 2.0f; + encoder.add_sensor("s1", &s1); + encoder.add_sensor("s2", &s2); + + // Mark s1 as not updated, only s2 as updated + encoder.sensors_[0].updated = false; + encoder.sensors_[1].updated = true; + + encoder.send_data_(false); + ASSERT_EQ(encoder.sent_packets.size(), 1u); + + TestablePacketTransport decoder; + decoder.init_for_test("receiver"); + sensor::Sensor rs1, rs2; + rs1.state = -999.0f; + rs2.state = -999.0f; + decoder.add_remote_sensor("sender", "s1", &rs1); + decoder.add_remote_sensor("sender", "s2", &rs2); + + auto &packet = encoder.sent_packets[0]; + decoder.process_({packet.data(), packet.size()}); + + EXPECT_FLOAT_EQ(rs1.state, -999.0f); // not updated, not sent + EXPECT_FLOAT_EQ(rs2.state, 2.0f); // updated, sent +} +#endif + +// --- Ping key tests --- + +TEST(PacketTransportTest, PingKeyStoredWhenEncrypted) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + transport.set_encryption_key(std::vector(32, 0xAA)); + + auto ping = build_ping_packet("requester", 0xDEADBEEF); + transport.process_({ping.data(), ping.size()}); + + ASSERT_EQ(transport.ping_keys_.size(), 1u); + EXPECT_EQ(transport.ping_keys_["requester"], 0xDEADBEEFu); +} + +TEST(PacketTransportTest, PingKeyIgnoredWhenNotEncrypted) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + // No encryption key — add_key_ should be a no-op + + auto ping = build_ping_packet("requester", 0xDEADBEEF); + transport.process_({ping.data(), ping.size()}); + + EXPECT_TRUE(transport.ping_keys_.empty()); +} + +TEST(PacketTransportTest, PingKeyUpdatedOnRepeat) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + transport.set_encryption_key(std::vector(32, 0xAA)); + + auto ping1 = build_ping_packet("host1", 0x1111); + transport.process_({ping1.data(), ping1.size()}); + EXPECT_EQ(transport.ping_keys_["host1"], 0x1111u); + + // Same host, new key value — should update in place + auto ping2 = build_ping_packet("host1", 0x2222); + transport.process_({ping2.data(), ping2.size()}); + EXPECT_EQ(transport.ping_keys_.size(), 1u); + EXPECT_EQ(transport.ping_keys_["host1"], 0x2222u); +} + +TEST(PacketTransportTest, PingKeyMaxLimit) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + transport.set_encryption_key(std::vector(32, 0xAA)); + + // Fill to MAX_PING_KEYS (4) + for (int i = 0; i < 4; i++) { + char name[16]; + snprintf(name, sizeof(name), "host%d", i); + auto ping = build_ping_packet(name, 0x1000 + i); + transport.process_({ping.data(), ping.size()}); + } + EXPECT_EQ(transport.ping_keys_.size(), 4u); + + // 5th key should be discarded + auto ping = build_ping_packet("host4", 0x9999); + transport.process_({ping.data(), ping.size()}); + EXPECT_EQ(transport.ping_keys_.size(), 4u); + EXPECT_FALSE(transport.ping_keys_.contains("host4")); +} + +#ifdef USE_SENSOR +TEST(PacketTransportTest, PingKeyIncludedInTransmittedPacket) { + std::vector key(32, 0xBB); + + // Responder: encrypted, owns a sensor + TestablePacketTransport responder; + responder.init_for_test("responder"); + responder.set_encryption_key(key); + sensor::Sensor local_sensor; + local_sensor.state = 77.7f; + responder.add_sensor("temp", &local_sensor); + + // Requester sends a MAGIC_PING that the responder processes + auto ping = build_ping_packet("requester", 0xDEADBEEF); + responder.process_({ping.data(), ping.size()}); + ASSERT_EQ(responder.ping_keys_.size(), 1u); + + // Responder sends sensor data — ping key should be embedded + responder.send_data_(true); + ASSERT_EQ(responder.sent_packets.size(), 1u); + + // Requester: encrypted provider, ping-pong enabled, expects key 0xDEADBEEF + TestablePacketTransport requester; + requester.init_for_test("requester"); + requester.set_ping_pong_enable(true); + requester.ping_key_ = 0xDEADBEEF; + sensor::Sensor remote_sensor; + remote_sensor.state = -999.0f; + requester.add_remote_sensor("responder", "temp", &remote_sensor); + requester.set_provider_encryption("responder", key); + + // The requester decrypts the packet and finds its ping key echoed back, + // which gates the sensor data — if the key is missing, data is blocked. + auto &packet = responder.sent_packets[0]; + requester.process_({packet.data(), packet.size()}); + EXPECT_FLOAT_EQ(remote_sensor.state, 77.7f); +} + +TEST(PacketTransportTest, MissingPingKeyBlocksSensorData) { + std::vector key(32, 0xBB); + + // Responder sends data WITHOUT receiving any MAGIC_PING first — no ping keys + TestablePacketTransport responder; + responder.init_for_test("responder"); + responder.set_encryption_key(key); + sensor::Sensor local_sensor; + local_sensor.state = 77.7f; + responder.add_sensor("temp", &local_sensor); + responder.send_data_(true); + ASSERT_EQ(responder.sent_packets.size(), 1u); + + // Requester with ping-pong enabled expects a key that isn't in the packet + TestablePacketTransport requester; + requester.init_for_test("requester"); + requester.set_ping_pong_enable(true); + requester.ping_key_ = 0xDEADBEEF; + sensor::Sensor remote_sensor; + remote_sensor.state = -999.0f; + requester.add_remote_sensor("responder", "temp", &remote_sensor); + requester.set_provider_encryption("responder", key); + + auto &packet = responder.sent_packets[0]; + requester.process_({packet.data(), packet.size()}); + EXPECT_FLOAT_EQ(remote_sensor.state, -999.0f); // blocked — ping key not found +} +#endif + +// --- Process error handling --- + +TEST(PacketTransportTest, ProcessShortBuffer) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + uint8_t buf[] = {0x53}; + // Too short for a magic number - should return safely + transport.process_({buf, 1}); +} + +TEST(PacketTransportTest, ProcessBadMagic) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + uint8_t buf[] = {0xFF, 0xFF, 0x00, 0x00}; + // Wrong magic - should return safely + transport.process_({buf, sizeof(buf)}); +} + +TEST(PacketTransportTest, ProcessOwnHostname) { + TestablePacketTransport transport; + transport.init_for_test("myself"); + // Build a packet from "myself" using a separate encoder + TestablePacketTransport fake_sender; + fake_sender.init_for_test("myself"); + fake_sender.send_data_(true); + ASSERT_EQ(fake_sender.sent_packets.size(), 1u); + + auto &packet = fake_sender.sent_packets[0]; + // Should be silently ignored because hostname matches our own + transport.process_({packet.data(), packet.size()}); +} + +TEST(PacketTransportTest, ProcessUnknownHostname) { + TestablePacketTransport transport; + transport.init_for_test("receiver"); + // No providers registered - "unknown" will not be found + TestablePacketTransport sender; + sender.init_for_test("unknown"); + sender.send_data_(true); + ASSERT_EQ(sender.sent_packets.size(), 1u); + + auto &packet = sender.sent_packets[0]; + // Should return safely without crash + transport.process_({packet.data(), packet.size()}); +} + +// --- Send disabled --- + +TEST(PacketTransportTest, NoSendWhenDisabled) { + TestablePacketTransport transport; + transport.init_for_test("sender"); + transport.send_enabled = false; + transport.send_data_(true); + EXPECT_TRUE(transport.sent_packets.empty()); +} + +} // namespace esphome::packet_transport::testing From 0e2a10c5f02cd4ba37ed53dde370bdd0a54c43ac Mon Sep 17 00:00:00 2001 From: rwrozelle Date: Wed, 4 Mar 2026 19:34:13 -0800 Subject: [PATCH 3/6] [openthread] Cache is_connected() for cheap inline access (#14484) Co-authored-by: J. Nick Koston --- esphome/components/openthread/openthread.cpp | 25 ++++++------------- esphome/components/openthread/openthread.h | 5 +++- .../components/openthread/openthread_esp.cpp | 3 +++ 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/esphome/components/openthread/openthread.cpp b/esphome/components/openthread/openthread.cpp index 92897a7e96..9452f5a41e 100644 --- a/esphome/components/openthread/openthread.cpp +++ b/esphome/components/openthread/openthread.cpp @@ -1,9 +1,7 @@ #include "esphome/core/defines.h" #ifdef USE_OPENTHREAD #include "openthread.h" -#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 5, 0) #include "esp_openthread.h" -#endif #include @@ -48,22 +46,15 @@ void OpenThreadComponent::dump_config() { } } -bool OpenThreadComponent::is_connected() { - auto lock = InstanceLock::try_acquire(100); - if (!lock) { - ESP_LOGW(TAG, "Failed to acquire OpenThread lock in is_connected"); - return false; +void OpenThreadComponent::on_state_changed_(otChangedFlags flags, void *context) { + if (flags & OT_CHANGED_THREAD_ROLE) { + auto *self = static_cast(context); + // This runs on the OpenThread task thread with the OT lock held, + // so we can safely call otThreadGetDeviceRole directly. + otInstance *instance = esp_openthread_get_instance(); + otDeviceRole role = otThreadGetDeviceRole(instance); + self->connected_ = role >= OT_DEVICE_ROLE_CHILD; } - - otInstance *instance = lock->get_instance(); - if (instance == nullptr) { - return false; - } - - otDeviceRole role = otThreadGetDeviceRole(instance); - - // TODO: If we're a leader, check that there is at least 1 known peer - return role >= OT_DEVICE_ROLE_CHILD; } // Gets the off-mesh routable address diff --git a/esphome/components/openthread/openthread.h b/esphome/components/openthread/openthread.h index 728847afa5..d853c58f95 100644 --- a/esphome/components/openthread/openthread.h +++ b/esphome/components/openthread/openthread.h @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -26,7 +27,7 @@ class OpenThreadComponent : public Component { bool teardown() override; float get_setup_priority() const override { return setup_priority::WIFI; } - bool is_connected(); + bool is_connected() const { return this->connected_; } network::IPAddresses get_ip_addresses(); std::optional get_omr_address(); void ot_main(); @@ -42,6 +43,7 @@ class OpenThreadComponent : public Component { protected: std::optional get_omr_address_(InstanceLock &lock); + static void on_state_changed_(otChangedFlags flags, void *context); std::function factory_reset_external_callback_; #if CONFIG_OPENTHREAD_MTD uint32_t poll_period_{0}; @@ -49,6 +51,7 @@ class OpenThreadComponent : public Component { std::optional output_power_{}; bool teardown_started_{false}; bool teardown_complete_{false}; + bool connected_{false}; private: // Stores a pointer to a string literal (static storage duration). diff --git a/esphome/components/openthread/openthread_esp.cpp b/esphome/components/openthread/openthread_esp.cpp index 2af78b729f..2296e32b7f 100644 --- a/esphome/components/openthread/openthread_esp.cpp +++ b/esphome/components/openthread/openthread_esp.cpp @@ -175,6 +175,9 @@ void OpenThreadComponent::ot_main() { // Pass the existing dataset, or NULL which will use the preprocessor definitions ESP_ERROR_CHECK(esp_openthread_auto_start(dataset.mLength > 0 ? &dataset : nullptr)); + // Register state change callback to update connected_ reactively instead of polling + otSetStateChangedCallback(instance, OpenThreadComponent::on_state_changed_, this); + esp_openthread_launch_mainloop(); // Clean up From 6af723e87db44e506d2b177441d499339f153172 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 4 Mar 2026 22:27:45 -1000 Subject: [PATCH 4/6] [socket] Add socket wake support for RP2040 Enable instant wake on socket activity for RP2040 using ARM WFE/SEV hardware instructions, matching ESP8266's esp_delay/esp_schedule pattern. Previously, RP2040 used plain delay() in the main loop which could not be interrupted by incoming socket data. Now LWIP recv/accept callbacks call socket_wake() which sets a flag and sends a hardware event (__sev) to wake the main loop from __wfe() sleep immediately. The implementation uses a one-shot pico-sdk timer alarm for timeout (same pattern as ESP8266's os_timer_arm) combined with __wfe() for true hardware sleep between events. --- .../components/socket/lwip_raw_tcp_impl.cpp | 61 ++++++++++++++++++- esphome/components/socket/socket.h | 12 ++-- esphome/core/application.cpp | 8 ++- 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index d697bd47a5..f5bb9d6cd2 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -13,6 +13,11 @@ #include // For esp_schedule() #endif +#ifdef USE_RP2040 +#include // For __sev(), __wfe() +#include // For add_alarm_in_ms(), cancel_alarm() +#endif + namespace esphome::socket { #ifdef USE_ESP8266 @@ -42,6 +47,58 @@ void IRAM_ATTR socket_wake() { } #endif +#ifdef USE_RP2040 +// RP2040 (non-FreeRTOS) socket wake using hardware WFE/SEV instructions. +// +// Same pattern as ESP8266's esp_delay()/esp_schedule(): set a one-shot timer, +// then sleep with __wfe(). Wake on either: +// - Timer alarm fires → callback calls __sev() → __wfe() returns → timeout +// - Socket data arrives → LWIP callback calls socket_wake() → __sev() → __wfe() returns → early wake +// +// CYW43 WiFi chip communicates via SPI interrupts on core 0. When data arrives, +// the GPIO interrupt fires → async_context pendsv processes CYW43/LWIP → recv/accept +// callbacks call socket_wake() → __sev() wakes the main loop from __wfe() sleep. +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +static volatile bool s_socket_woke = false; +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +static volatile bool s_delay_expired = false; + +static int64_t alarm_callback_(alarm_id_t id, void *user_data) { + (void) id; + (void) user_data; + s_delay_expired = true; + // Wake the main loop from __wfe() sleep — timeout expired. + __sev(); + // Return 0 = don't reschedule (one-shot) + return 0; +} + +void socket_delay(uint32_t ms) { + if (ms == 0) + return; + s_socket_woke = false; + s_delay_expired = false; + // Set a one-shot timer to wake us after the timeout + alarm_id_t alarm = add_alarm_in_ms(ms, alarm_callback_, nullptr, true); + // Sleep until woken by either the timer alarm or socket_wake(). + // __wfe() may return spuriously (stale event register, other interrupts), + // so we loop checking both flags. + while (!s_socket_woke && !s_delay_expired) { + __wfe(); + } + // Cancel timer if we woke early (socket data arrived before timeout) + if (alarm > 0 && !s_delay_expired) + cancel_alarm(alarm); +} + +void socket_wake() { + s_socket_woke = true; + // Wake the main loop from __wfe() sleep. __sev() is a global event that + // wakes any core sleeping in __wfe(). This is ISR-safe. + __sev(); +} +#endif + static const char *const TAG = "socket.lwip"; // set to 1 to enable verbose lwip logging @@ -371,7 +428,7 @@ err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { } else { pbuf_cat(this->rx_buf_, pb); } -#ifdef USE_ESP8266 +#if (defined(USE_ESP8266) || defined(USE_RP2040)) // Wake the main loop immediately so it can process the received data. socket_wake(); #endif @@ -650,7 +707,7 @@ err_t LWIPRawListenImpl::accept_fn_(struct tcp_pcb *newpcb, err_t err) { sock->init(); this->accepted_sockets_[this->accepted_socket_count_++] = std::move(sock); LWIP_LOG("Accepted connection, queue size: %d", this->accepted_socket_count_); -#ifdef USE_ESP8266 +#if (defined(USE_ESP8266) || defined(USE_RP2040)) // Wake the main loop immediately so it can accept the new connection. socket_wake(); #endif diff --git a/esphome/components/socket/socket.h b/esphome/components/socket/socket.h index 0884e4ba3e..65f1b9a4c3 100644 --- a/esphome/components/socket/socket.h +++ b/esphome/components/socket/socket.h @@ -120,13 +120,17 @@ socklen_t set_sockaddr_any(struct sockaddr *addr, socklen_t addrlen, uint16_t po /// Format sockaddr into caller-provided buffer, returns length written (excluding null) size_t format_sockaddr_to(const struct sockaddr *addr_ptr, socklen_t len, std::span buf); -#if defined(USE_ESP8266) && defined(USE_SOCKET_IMPL_LWIP_TCP) +#if (defined(USE_ESP8266) || defined(USE_RP2040)) && defined(USE_SOCKET_IMPL_LWIP_TCP) /// Delay that can be woken early by socket activity. -/// On ESP8266, lwip callbacks set a flag and call esp_schedule() to wake the delay. +/// On ESP8266, uses esp_delay() with a callback that checks socket activity. +/// On RP2040, uses __wfe() (Wait For Event) to truly sleep until an interrupt +/// (CYW43 GPIO, timer alarm) fires, then processes pending async_context work. void socket_delay(uint32_t ms); -/// Signal socket/IO activity and wake the main loop from esp_delay() early. -/// ISR-safe: uses IRAM_ATTR internally and only sets a volatile flag + esp_schedule(). +/// Signal socket/IO activity and wake the main loop early. +/// On ESP8266: sets flag + esp_schedule(). +/// On RP2040: sets flag + __sev() (Send Event) to wake from __wfe(). +/// ISR-safe on both platforms. void socket_wake(); // NOLINT(readability-redundant-declaration) #endif diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index db1c8a0c0a..8685bff360 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -32,7 +32,7 @@ #include "esphome/components/status_led/status_led.h" #endif -#if defined(USE_ESP8266) && defined(USE_SOCKET_IMPL_LWIP_TCP) +#if (defined(USE_ESP8266) || defined(USE_RP2040)) && defined(USE_SOCKET_IMPL_LWIP_TCP) #include "esphome/components/socket/socket.h" #endif @@ -713,8 +713,10 @@ void Application::yield_with_select_(uint32_t delay_ms) { } // No sockets registered or select() failed - use regular delay delay(delay_ms); -#elif defined(USE_ESP8266) && defined(USE_SOCKET_IMPL_LWIP_TCP) - // No select support but can wake on socket activity via esp_schedule() +#elif (defined(USE_ESP8266) || defined(USE_RP2040)) && defined(USE_SOCKET_IMPL_LWIP_TCP) + // No select support but can wake on socket activity + // ESP8266: via esp_schedule() + // RP2040: via __sev()/__wfe() hardware sleep/wake socket::socket_delay(delay_ms); #else // No select support, use regular delay From 8050fa6801bca15704d66b9fc24b376885f1f1fe Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 4 Mar 2026 22:30:28 -1000 Subject: [PATCH 5/6] Use #elif for mutually exclusive platform guards --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index f5bb9d6cd2..bcd7791281 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -45,9 +45,7 @@ void IRAM_ATTR socket_wake() { s_socket_woke = true; esp_schedule(); } -#endif - -#ifdef USE_RP2040 +#elif defined(USE_RP2040) // RP2040 (non-FreeRTOS) socket wake using hardware WFE/SEV instructions. // // Same pattern as ESP8266's esp_delay()/esp_schedule(): set a one-shot timer, From 12616bcc60354197bc358a33d91b884dd5ecf1fd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 4 Mar 2026 22:31:35 -1000 Subject: [PATCH 6/6] Code quality: consistent elif includes, fix callback name, handle alarm error --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index bcd7791281..7b5e1cdba5 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -11,9 +11,7 @@ #ifdef USE_ESP8266 #include // For esp_schedule() -#endif - -#ifdef USE_RP2040 +#elif defined(USE_RP2040) #include // For __sev(), __wfe() #include // For add_alarm_in_ms(), cancel_alarm() #endif @@ -61,7 +59,7 @@ static volatile bool s_socket_woke = false; // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static volatile bool s_delay_expired = false; -static int64_t alarm_callback_(alarm_id_t id, void *user_data) { +static int64_t alarm_callback(alarm_id_t id, void *user_data) { (void) id; (void) user_data; s_delay_expired = true; @@ -76,8 +74,11 @@ void socket_delay(uint32_t ms) { return; s_socket_woke = false; s_delay_expired = false; - // Set a one-shot timer to wake us after the timeout - alarm_id_t alarm = add_alarm_in_ms(ms, alarm_callback_, nullptr, true); + // Set a one-shot timer to wake us after the timeout. + // add_alarm_in_ms returns >0 on success, 0 if time already passed, <0 on error. + alarm_id_t alarm = add_alarm_in_ms(ms, alarm_callback, nullptr, true); + if (alarm <= 0) + return; // Timer already fired or no alarm slots available // Sleep until woken by either the timer alarm or socket_wake(). // __wfe() may return spuriously (stale event register, other interrupts), // so we loop checking both flags. @@ -85,7 +86,7 @@ void socket_delay(uint32_t ms) { __wfe(); } // Cancel timer if we woke early (socket data arrived before timeout) - if (alarm > 0 && !s_delay_expired) + if (!s_delay_expired) cancel_alarm(alarm); }