From 768ab5b672bceed3c81d09db60c3d710bcbc93f1 Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Fri, 28 Aug 2026 11:16:41 -0700 Subject: [PATCH 1/6] [modbus] Hub and helpers cleanup; tighten queue_pdu validation (#18847) --- esphome/components/modbus/__init__.py | 5 +- esphome/components/modbus/modbus.cpp | 91 ++------ esphome/components/modbus/modbus.h | 214 ++++++------------ .../components/modbus/modbus_definitions.h | 1 - esphome/components/modbus/modbus_helpers.cpp | 13 +- esphome/components/modbus/modbus_helpers.h | 39 ++-- .../modbus/modbus_client_hub_test.cpp | 81 +++++-- .../components/modbus/modbus_helpers_test.cpp | 6 + .../modbus/modbus_unknown_function_test.cpp | 32 ++- 9 files changed, 207 insertions(+), 275 deletions(-) diff --git a/esphome/components/modbus/__init__.py b/esphome/components/modbus/__init__.py index 769858e72a..76cfdbed70 100644 --- a/esphome/components/modbus/__init__.py +++ b/esphome/components/modbus/__init__.py @@ -89,9 +89,8 @@ _WRITE_FUNCTION_CODES = frozenset({0x05, 0x06, 0x0F, 0x10, 0x16, 0x17}) def is_function_code_write(function_code: int) -> bool: """True if the Modbus function code writes (mutates). The exception bit (0x80) is masked off first, - so an exception-flagged code still classifies by its base code - stricter than the runtime hub, - whose classify() treats an exception-flagged code as a read. Keep in sync with - modbus::helpers::is_function_code_write().""" + so an exception-flagged code still classifies by its base code (the runtime hub never queues one: + queue_pdu() refuses them). Keep in sync with modbus::helpers::is_function_code_write().""" return function_code & 0x7F in _WRITE_FUNCTION_CODES diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index e83ffb2708..aa998d283a 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -10,17 +10,12 @@ namespace esphome::modbus { static const char *const TAG = "modbus"; -// Maximum bytes to log for Modbus frames (truncated if larger) static constexpr size_t MODBUS_MAX_LOG_BYTES = 64; // Approximate bits per character on the wire (depends on parity/stop bit config) static constexpr uint32_t MODBUS_BITS_PER_CHAR = 11; -// Milliseconds per second static constexpr uint32_t MS_PER_SEC = 1000; -// Shortest gap between two "no device accepted broadcast" warnings -static constexpr uint32_t UNACCEPTED_BROADCAST_WARN_INTERVAL_MS = 60 * MS_PER_SEC; - void Modbus::setup() { if (this->flow_control_pin_ != nullptr) { this->flow_control_pin_->setup(); @@ -43,10 +38,7 @@ void Modbus::setup() { } void Modbus::loop() { - // Receive any available bytes from UART this->receive_bytes_(); - - // Parse bytes into frames and process them this->parse_modbus_frames(); } @@ -55,7 +47,7 @@ void ModbusClientHub::loop() { // never times out an entry whose pending count has not been drained. No-op when nothing is owed. this->sweep_(); - this->Modbus::loop(); // receive bytes and parse frames + this->Modbus::loop(); // Send-wait watchdog: only the cheap time check runs at loop rate; expire_waiting_() looks the // entry up and holds off if the response has started arriving. @@ -104,11 +96,8 @@ bool Modbus::timeout_() { } int32_t Modbus::tx_delay_remaining() { - // We use millis() here and elsewhere instead of App.get_loop_component_start_time() to avoid stale timestamps - // It's critical in all timestamp comparisons that the left timestamp comes before the right one in time - // If we use a cached value in place of millis() and last_modbus_byte_ is updated inside our loop - // then the comparison is backwards (small negative which wraps to large positive) and will cause a false timeout - // So in this component we don't use any cached timestamp values to avoid these annoying bugs + // millis() here and everywhere in this component, never a cached loop timestamp: a cached "now" can + // predate last_modbus_byte_, and the unsigned subtraction then wraps huge and forces a false timeout. const uint32_t now = millis(); return std::max({(int32_t) 0, (int32_t) (this->last_send_tx_offset_ + this->frame_delay_ms_ - (now - this->last_send_)), @@ -124,22 +113,13 @@ int32_t ModbusClientHub::tx_delay_remaining() { } bool Modbus::tx_blocked() { - // We block transmission in any of these cases: - // 1. There are bytes in the UART Rx buffer - // 2. There are bytes in our Rx buffer - // 3. The last sent byte isn't more than tx_delay ms ago (i.e. wait to tell receivers that our previous Tx is done) - // 4. The last received byte isn't more than tx_delay ms ago (i.e. wait to be sure there isn't more Rx coming) - // N.B. We allow a small delay (MODBUS_TX_MAX_DELAY_MS) to avoid looping on small delays. This gets handled by - // send_frame_. + // Blocked while any rx bytes are pending, or within tx_delay of the last byte in either direction + // (receivers must see our previous tx as done, and more rx may be coming). A remaining delay up to + // MODBUS_TX_MAX_DELAY_MS doesn't block - send_frame_ absorbs it instead of looping on small waits. return this->available() || !this->rx_buffer_.empty() || this->tx_delay_remaining() > MODBUS_TX_MAX_DELAY_MS; } -bool ModbusClientHub::tx_blocked() { - // We block transmission in any of these case: - // 1. We're waiting for a response (a waiting entry: WAITING/INTERRUPTED/WAITING_RETIRED/INTERRUPTED_RETIRED) - // 2. Any of the base class tx_blocked conditions - return this->waiting_for_response_ || this->Modbus::tx_blocked(); -} +bool ModbusClientHub::tx_blocked() { return this->waiting_for_response_ || this->Modbus::tx_blocked(); } bool ModbusClientHub::tx_buffer_empty() { // "Empty" for ready_for_immediate_send(): no one-shot is queued ahead of the caller. Entries in @@ -219,10 +199,9 @@ void ModbusServerHub::parse_modbus_frames() { this->clear_rx_buffer_(LOG_STR("timeout after partial response"), true); } +// Scans forward from min_length to find a frame boundary by CRC match for unknown-length function codes. +// Returns the matched frame length, or 0 if no valid CRC was found within MAX_FRAME_SIZE. uint16_t Modbus::find_frame_end_by_crc_(uint16_t min_length) const { - // Unknown-length functions (user-defined codes, unimplemented management codes, unassigned values) - // could be any length - we have to rely on the CRC to determine completeness. - // If a CRC match is never found, the buffer will eventually overflow and be cleared. const uint8_t *raw = &this->rx_buffer_[0]; const size_t size = this->rx_buffer_.size(); const auto max_len = static_cast(std::min(size, size_t(MAX_FRAME_SIZE))); @@ -531,8 +510,7 @@ void ModbusServerHub::process_broadcast_frame_(uint8_t function_code, std::span< return; } // A broadcast is never answered, so a rejecting device has no other feedback channel: report the - // per-device outcome at V, and warn if the write reached nobody at all. - bool accepted = false; + // per-device outcome at V. for (auto *device : this->devices_) { // Same handlers as an addressed write - a device cannot tell a broadcast apart, and does not need // to: the hub owns the difference, which is only that no reply is ever sent. @@ -542,24 +520,6 @@ void ModbusServerHub::process_broadcast_frame_(uint8_t function_code, std::span< if (device_status.has_value()) { ESP_LOGV(TAG, "Device %" PRIu8 " rejected broadcast write with exception %" PRIu8, device->get_address(), static_cast(device_status.value())); - } else { - accepted = true; - } - } - if (!accepted && !this->devices_.empty()) { - const uint16_t entity_count = coils ? coil_count : static_cast(registers.size()); - const LogString *const entity_name = coils ? LOG_STR("coils") : LOG_STR("registers"); - // Warn at most once per interval, then drop to VERBOSE: on a shared bus a broadcast aimed at other nodes - // repeats forever, so warning per frame would flood the log. - const uint32_t now = millis(); - if (this->last_unaccepted_broadcast_warn_ == 0 || - now - this->last_unaccepted_broadcast_warn_ > UNACCEPTED_BROADCAST_WARN_INTERVAL_MS) { - this->last_unaccepted_broadcast_warn_ = now; - ESP_LOGW(TAG, "No device accepted broadcast write of %" PRIu16 " %s at 0x%04X", entity_count, - LOG_STR_ARG(entity_name), start_address); - } else { - ESP_LOGV(TAG, "No device accepted broadcast write of %" PRIu16 " %s at 0x%04X", entity_count, - LOG_STR_ARG(entity_name), start_address); } } } @@ -783,8 +743,6 @@ bool Modbus::send_frame_(const ModbusFrame &frame) { delay(tx_delay_remaining); } - // The delay above can span several ms; a byte arriving in that window blocks transmission after the - // caller's gate already passed. Don't collide with the incoming frame - leave the entry to retry. if (this->tx_blocked()) { return false; } @@ -831,7 +789,7 @@ void ModbusClientHub::send_next_frame_() { // reports the transmission, and the entry then retires with no terminal callback instead of // occupying the waiting slot until the send-wait timeout expires. The turnaround delay already // spaces the next frame; the following sweep erases the entry. - ESP_LOGV(TAG, "Broadcast to address 0 sent; no reply expected (fire-and-forget)"); + ESP_LOGV(TAG, "Broadcast to address 0 sent; no reply expected"); cmd->complete_broadcast(); this->sweep_needed_ = true; return; @@ -983,6 +941,8 @@ bool ModbusDeviceCommand::timed_out() { this->decrement_pending(); // resolve this request (WAITING-origin, so pending >= 1) if (this->device == nullptr) return false; // resolved, no one to tell + // A cleared frame that timed out still honors a retry: the clear is address-scoped (any device may + // call it) while the retry is the owning device's call via on_no_response - the bus obeys the owner. if (this->device->on_no_response(this->frame.pdu())) this->increment_pending(); // granted retry = re-request (capped) return true; @@ -1054,18 +1014,14 @@ bool ModbusClientHub::queue_pdu(uint8_t address, std::span pdu, M ESP_LOGE(TAG, "Frame too large, refused: %" PRIu8 ":%zu bytes", address, pdu.size()); return false; } - // classify() drives both the broadcast guard and the continuous check below; compute it once. - const CommandPriority priority = ModbusDeviceCommand::classify(pdu[0]); - // A broadcast (address 0) is never answered (Modbus 4.1), so it is only meaningful for a command that - // changes state. Refuse a broadcast that expects a reply - anything but a write or a custom/vendor code - - // as it could never deliver a result, so the caller learns via the false return (and on_not_sent). - // 0x17 (read/write multiple) is a knowing inclusion: classify() treats it as a write, so its write half - // lands on every server and its unanswerable read half is simply discarded. An exception-flagged custom - // code (0x80 bit set) is refused: is_function_code_custom() masks that bit away, so exclude it explicitly - // here to match classify()'s exception-first handling of the write side. - if (address == BROADCAST_ADDRESS && priority != CommandPriority::WRITE && - (!helpers::is_function_code_custom(pdu[0]) || helpers::is_function_code_exception(pdu[0]))) { + if (helpers::is_function_code_exception(pdu[0])) { + ESP_LOGW(TAG, "Exception PDU refused for address %" PRIu8 ": function code 0x%X has the exception bit set", address, + pdu[0]); + return false; + } + + if (address == BROADCAST_ADDRESS && !helpers::is_function_code_broadcastable(pdu[0])) { ESP_LOGW(TAG, "Broadcast refused for function 0x%X: a broadcast (address 0) is never answered", pdu[0]); return false; } @@ -1073,7 +1029,7 @@ bool ModbusClientHub::queue_pdu(uint8_t address, std::span pdu, M // Normalize the caller's options in place (the param is a by-value copy) so everything stored or // merged below carries effective options, never the raw request. // continuous is ignored for every mutating code (re-writing a value forever is never intended). - if (options.continuous && priority == CommandPriority::WRITE) { + if (options.continuous && helpers::is_function_code_write(pdu[0])) { ESP_LOGW(TAG, "continuous is ignored for a mutating function (0x%X, address %" PRIu8 ")", pdu[0], address); options.continuous = false; } @@ -1089,9 +1045,7 @@ bool ModbusClientHub::queue_pdu(uint8_t address, std::span pdu, M continue; if (device == nullptr) { // A dropped read is routine (DEBUG); a dropped write/custom warns (unobservable without a device). - const bool requeueable = - !helpers::is_function_code_exception(pdu[0]) && helpers::is_function_code_read_only(pdu[0]); - if (requeueable) { + if (helpers::is_function_code_read_only(pdu[0])) { ESP_LOGD(TAG, "Anonymous duplicate of active frame for %" PRIu8 " (function 0x%X), dropped", address, pdu[0]); } else { ESP_LOGW(TAG, @@ -1364,7 +1318,6 @@ void ModbusClientDevice::dispatch_response_(std::span request_pdu } } -// Default on_custom_response handler to warn when responses unexpectedly trigger on_custom_response void ModbusClientDevice::on_custom_response(std::span request_pdu, std::span response_pdu, ResponseStatus status) { // The dispatcher never calls this with an empty request, but this is a public virtual - stay safe. diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index e766ff04cc..3ddaafb9fc 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -16,26 +16,21 @@ namespace esphome::modbus { -// Tx queue backstop. Duplicate frames dedup into one entry, so reads can never approach this in a -// sane config - it exists to stop a runaway generator of distinct frames (e.g. a loop writing a -// changing value) from growing the heap unboundedly. The deque grows on demand; this reserves nothing. -// Worst case the cap permits: 128 distinct max-size frames = ~32 kB of spilled frame data plus -// ~3 kB of deque node storage (typical 8-byte frames stay inline; large PDUs spill to one -// allocation each) - pathological configs only, but the numbers matter when tuning for ESP8266. +// Tx queue backstop: duplicates dedup into one entry, so only a runaway generator of distinct frames +// (e.g. a loop writing a changing value) could grow the heap unboundedly. static constexpr uint16_t MODBUS_TX_BUFFER_SIZE = 128; static constexpr uint16_t MODBUS_TX_MAX_DELAY_MS = 5; // Typical frames -- reads and single-register/coil writes -- are exactly 8 bytes -// (address + 5-byte PDU + 2-byte CRC) and fit inline with no heap allocation. +// (address + 5-byte PDU + 2-byte CRC). static constexpr uint16_t MODBUS_FRAME_INLINE_SIZE = 8; struct ModbusFrame { - // Frame held in a small-buffer-optimized buffer. Typical frames fit inline; only larger - // multi-register or custom frames spill to a single heap allocation. This keeps the common, - // high-frequency tx traffic off the heap entirely, avoiding per-frame alloc/free churn. - // The buffer tracks its own length, so no separate size field is needed. - SmallInlineBuffer data; // Modbus RTU max is 256 bytes + // Small-buffer-optimized: typical frames fit inline, keeping high-frequency tx traffic off the + // heap; only large multi-register or custom frames spill to a single heap allocation. + SmallInlineBuffer data; + // A frame is [address][PDU...][CRC lo][CRC hi]. These are the only places that need to know that layout ModbusFrame(uint8_t address, const uint8_t *pdu, uint16_t pdu_len) { uint8_t *buf = this->data.init(pdu_len + 3); buf[0] = address; @@ -46,12 +41,9 @@ struct ModbusFrame { } uint16_t size() const { return static_cast(this->data.size()); } - - // A frame is [address][PDU...][CRC lo][CRC hi]. These are the only places that need to know that layout uint8_t address() const { return this->data.data()[0]; } - /// The PDU: function code + data, without address or CRC. Only valid while the frame is alive. - /// Requires a complete frame (size() >= MIN_FRAME_SIZE, guaranteed by the constructors) - the - /// subtraction would wrap on anything shorter. + /// A PDU is [function code][data...] without address or CRC. Only valid while the frame is alive. + /// Requires a complete frame (size() >= MIN_FRAME_SIZE, guaranteed by the constructors) std::span pdu() const { return std::span(this->data.data() + 1, this->size() - 3u); } }; @@ -73,15 +65,9 @@ class Modbus : public uart::UARTDevice, public Component { virtual int32_t tx_delay_remaining(); virtual void parse_modbus_frames() = 0; bool parse_modbus_server_frame_(); - // pdu is the whole PDU (function code + payload, no address/CRC); pdu[0] is the (standard or custom) function code. virtual void process_modbus_server_frame(uint8_t address, std::span pdu) = 0; void clear_rx_buffer_(const LogString *reason, bool warn = false, size_t bytes_to_clear = 0); - // Transmit a frame. Callers gate on tx_blocked() first, but the pre-send delay can span several ms, - // so this re-checks after the delay and returns false without transmitting if a byte arrived in that - // window (the caller then leaves its entry to retry). Returns true once the frame has been transmitted. bool send_frame_(const ModbusFrame &frame); - // Scans forward from min_length to find a frame boundary by CRC match for custom function codes. - // Returns the matched frame length, or 0 if no valid CRC was found within MAX_FRAME_SIZE. uint16_t find_frame_end_by_crc_(uint16_t min_length) const; uint32_t last_modbus_byte_{0}; @@ -99,8 +85,7 @@ class Modbus : public uart::UARTDevice, public Component { class ModbusClientDevice; class ModbusServerDevice; -// Transmit ordering, highest first: writes before one-shot reads before continuous polls. Derived -// at selection time, never caller-chosen or stored. +// Transmit ordering, highest first: writes before one-shot reads before continuous polls. enum class CommandPriority : uint8_t { CONTINUOUS = 0, READ, WRITE }; // Per-entry lifecycle state. Waiting states (see waiting_state()) hold the bus; the sweep delivers owed @@ -112,20 +97,15 @@ enum class FrameState : uint8_t { RECEIVED_EXCEPTION, TIMED_OUT, // on_no_response delivered at the send-wait timeout; awaiting reschedule/erase INTERRUPTED, // unexpected frame arrived; ignores this transaction, waits out the timeout - WAITING_RETIRED, // cleared while WAITING: a late response is still delivered as its usual terminal - INTERRUPTED_RETIRED, // cleared while INTERRUPTED: still distrusts late frames, ends in on_no_response - RETIRED, // cleared, off the wire + WAITING_RETIRED, // retired while WAITING: a late response is still delivered as its usual terminal + INTERRUPTED_RETIRED, // retired while INTERRUPTED: still distrusts late frames, ends in on_no_response + RETIRED, // retired, off the wire }; // Per-command send options. Append-only; pass via designated initializers ({.continuous = true}). -// The queue entry stores this struct whole, so a new field arrives at the queue with no plumbing - -// but it arrives inert. Every new field must define three rules before it does anything: -// 1. normalization in queue_pdu() (is it valid for this function code? e.g. continuous is -// stripped for mutating codes), -// 2. a merge rule for when a duplicate send absorbs into a live entry (continuous -// upgrades/downgrades via make_continuous(); a new field needs its own answer), -// 3. teardown: retire() resets the whole struct; silent_retire() leaves it, relying on the sweep -// to erase the entry. +// A new field reaches the queue with no plumbing but arrives inert until it defines three rules: +// normalization in queue_pdu(), a merge rule for duplicate absorption, and teardown in +// retire()/silent_retire(). struct CommandOptions { // A continuous poll lives in the queue until cancelled or failed; ignored for mutating codes. bool continuous{false}; @@ -135,17 +115,13 @@ struct ModbusDeviceCommand { ModbusClientDevice *device; ModbusFrame frame; // Place-in-line stamp (hub's free-running counter); selection takes the oldest for round-robin - // fairness within a class. Meant to wrap. Declared ahead of the byte fields so the tail packs - // densely and a growing CommandOptions eats trailing padding before enlarging the struct. + // fairness within a class. Meant to wrap. uint16_t seq{0}; FrameState state{FrameState::READY}; // Accepted requests this entry stands for, capped at max_pending(); drains one terminal each. // A continuous poll is a subscription: pending fixed at 1, removed only by cancellation or failure. uint8_t pending{1}; - // The entry's LIVE effective options, not a record of the caller's request: queue_pdu() normalizes - // before storing, duplicate absorption mutates continuous via make_continuous(), and retire() resets - // the struct (silent_retire() leaves it, relying on the sweep to erase the entry). See the - // CommandOptions comment for the rules a new field must define. + // The entry's LIVE effective options, not a record of the caller's request CommandOptions options; // Build a command from a PDU span (caller bounds it to MAX_PDU_SIZE) and pre-normalized options; @@ -154,28 +130,22 @@ struct ModbusDeviceCommand { CommandOptions options = {}, uint16_t seq = 0) : device(device), frame(address, pdu.data(), static_cast(pdu.size())), seq(seq), options(options) {} - // Transmit ordering class, derived (never stored): a continuous poll ranks below every one-shot. CommandPriority priority() const { - return this->options.continuous ? CommandPriority::CONTINUOUS : classify(this->frame.pdu()[0]); - } - // Wire-derived class: mutating codes rank WRITE; exception-flagged codes are excluded. - static CommandPriority classify(uint8_t function_code) { - if (helpers::is_function_code_exception(function_code)) - return CommandPriority::READ; - if (helpers::is_function_code_write(function_code)) { + if (this->options.continuous) + return CommandPriority::CONTINUOUS; + if (helpers::is_function_code_write(this->frame.pdu()[0])) { return CommandPriority::WRITE; } return CommandPriority::READ; } - // Requests this entry can serve: a standard read twice (run plus one re-run), everything else once. + // Requests this entry can serve uint8_t max_pending() const { const uint8_t fc = this->frame.pdu()[0]; - const bool requeueable = !helpers::is_function_code_exception(fc) && helpers::is_function_code_read_only(fc); - return (requeueable && !this->options.continuous) ? 2 : 1; + return (helpers::is_function_code_read_only(fc) && !this->options.continuous) ? 2 : 1; } - // Device-scoped clear: detach with no callback (device-less, pending 0). An entry still waiting for - // a response keeps its state as a reply-ignoring shell that resolves silently; any other goes RETIRED. + // Device-scoped clear: detach with no callback. An entry still waiting for a response keeps its state as a + // reply-ignoring shell that resolves silently; any other goes RETIRED. void silent_retire() { if (!this->waiting_state()) this->state = FrameState::RETIRED; @@ -183,28 +153,18 @@ struct ModbusDeviceCommand { this->device = nullptr; } // Fire-and-forget completion for a broadcast (address 0): the frame was transmitted (on_sent already - // fired), but a broadcast is never answered (Modbus 4.1), so the entry retires with NO terminal - // callback and the sweep erases it. Unlike response()/error()/timed_out(), it delivers nothing. - // A broadcast only carries a write or a custom code (reads are refused at queue_pdu()), and every such - // code caps pending at 1, so pending is always 1 here - clear it. + // fired), but a broadcast is never answered (Modbus 4.1), so the entry retires with no terminal callback. void complete_broadcast() { this->state = FrameState::RETIRED; this->pending = 0; } - // Re-ready for another transmission, restamped to the tail of its class (hub passes next_seq_++). + // Re-ready for another transmission, restamped to the tail of its class void requeue(uint16_t seq) { this->state = FrameState::READY; this->seq = seq; } // Re-task a frame that lives on: upgrade a one-shot to a continuous poll, or downgrade a poll back to - // a one-shot. Either way the entry keeps running and owes a request, so this is not a plain setter - - // to tear an entry down instead, use retire()/silent_retire(), which leave pending as the count owed. - // On: the entry becomes a continuous poll, superseding any absorbed requests (pending resets to the - // single subscription). Off: a one-shot duplicate has cancelled the poll, but the entry must still run - // once to serve that request - so restore one first. While the flag is still set max_pending() is 1, - // so the restore lifts a terminated poll (pending 0, after an error/timeout) back to 1 and is a no-op - // on a live poll already at 1; the flag drops afterwards, when a read's cap can widen to 2 without - // retroactively inflating that no-op. + // a one-shot. void make_continuous(bool continuous) { if (continuous) { this->options.continuous = true; @@ -214,13 +174,9 @@ struct ModbusDeviceCommand { this->options.continuous = false; } } - // Address-scoped clear: keep pending and device so the sweep delivers one on_not_sent() per un-run + // Address-scoped clear: keep pending and device so the sweep delivers one on_not_sent() per un-delivered // request. An entry still waiting for a response keeps its in-flight request (whose usual terminal is - // still coming) and drains only its duplicates: WAITING -> WAITING_RETIRED, and INTERRUPTED -> - // INTERRUPTED_RETIRED which keeps distrusting late frames (they were already interrupted). Any other - // state -> RETIRED, draining everything. A cleared frame that then times out still honors a retry: - // the clear is address-scoped (any device may call it) while the retry is the owning device's call - // via on_no_response - the bus obeys the owner. + // still coming) and drains only its duplicates. void retire() { if (this->state == FrameState::WAITING) { this->state = FrameState::WAITING_RETIRED; @@ -229,10 +185,10 @@ struct ModbusDeviceCommand { } else if (!this->waiting_state()) { // an already-retired shell stays put; off the wire -> RETIRED this->state = FrameState::RETIRED; } - this->options = {}; // reset every option so a future field is torn down without editing here + this->options = {}; // reset every option } - // True while the entry is still waiting for a response; the erase pass exempts these even at pending 0. + // True while the entry is still waiting for a response bool waiting_state() const { return this->state == FrameState::WAITING || this->state == FrameState::INTERRUPTED || this->state == FrameState::WAITING_RETIRED || this->state == FrameState::INTERRUPTED_RETIRED; @@ -245,7 +201,7 @@ struct ModbusDeviceCommand { } return false; } - // Add one request, honouring the cap; false = already at cap (absorb a duplicate, restore a retry). + bool increment_pending() { if (this->pending < this->max_pending()) { this->pending++; @@ -255,7 +211,7 @@ struct ModbusDeviceCommand { } // Terminal/lifecycle methods: each owns its transition, callback, and pending accounting and - // returns whether a callback ran. Out-of-line: ModbusClientDevice is incomplete here. + // returns whether a callback ran. bool sent(); bool response(std::span response_pdu); bool error(ExceptionCode exception_code); @@ -264,9 +220,6 @@ struct ModbusDeviceCommand { bool notify_retired(); /// True if this command carries the same wire frame (address + PDU) as the given one. - /// Cancellation matches the exact frame, not the action instance: a continuous poll whose - /// start_address (or other field) is templated produces one poll per distinct frame, and a later - /// cancel built from different argument values will not reach the polls it does not byte-match. bool same_frame(uint8_t address, std::span pdu) const { const auto own_pdu = this->frame.pdu(); return own_pdu.size() == pdu.size() && this->frame.address() == address && @@ -291,17 +244,13 @@ class ModbusClientHub : public Modbus { payload_len), device); }; - /// Queue a request. The name says queue, not send: the frame is appended to the transmit queue and - /// goes out later from loop(), so a true return means accepted into the machine (it will resolve in - /// exactly one terminal callback - except a broadcast (address 0), which is never answered and so gets - /// only on_sent()), NOT that anything reached the wire - that is on_sent(). False means - /// it never entered the machine at all (empty or oversize PDU, full queue, anonymous or over-cap - /// duplicate) and no callback of any kind will follow; the false return is the whole story. + /// Queue a request. True = accepted: it resolves in exactly one terminal callback (a broadcast, + /// address 0, gets only on_sent()). False = refused, and no callback of any kind follows. + /// Neither means anything reached the wire - on_sent() reports that. bool queue_pdu(uint8_t address, std::span pdu, ModbusClientDevice *device = nullptr, CommandOptions options = {}); - // Remove before 2027.2.0. Deliberately the signature 2026.7.4 shipped - void, and no CommandOptions: - // the bool return and the options argument arrived after that release, so nothing external can be - // relying on them under this name. Callers who want the queued/refused answer move to queue_pdu(). + // Remove before 2027.2.0. Deliberately the void, no-options signature 2026.7.4 shipped: nothing + // external can rely on the later additions under this name. ESPDEPRECATED("Use queue_pdu() instead - the call queues a request, it does not send one, and it " "reports whether the request was accepted. Removed in 2027.2.0", "2026.8.0") @@ -310,9 +259,10 @@ class ModbusClientHub : public Modbus { } ESPDEPRECATED("Use queue_pdu(payload[0], , device) instead. Removed in 2027.2.0", "2026.8.0") void send_raw(const std::vector &payload, ModbusClientDevice *device = nullptr); - // Clear an address's commands; each un-run request resolves via on_not_sent(), but a frame on the - // wire still runs to its usual terminal. clear_tx_queue_for_device() instead discards silently. + // Clear all commands matching the given address; each unsent request resolves via on_not_sent(), but a + // frame on the wire still runs to its usual terminal. void clear_tx_queue_for_address(uint8_t address); + // Clear all commands for a given device; no callbacks are delivered. void clear_tx_queue_for_device(ModbusClientDevice *device); protected: @@ -322,8 +272,7 @@ class ModbusClientHub : public Modbus { void send_next_frame_(); // Deliver owed callbacks from a quiescent hub and apply lifecycle bookkeeping; see FrameState. void sweep_(); - // The selection function: best READY entry (WRITE class first, then one-shot reads, then the - // least-recently-served continuous; FIFO by seq within each group), or nullptr. + // The selection function: best READY entry (ordered by priority; FIFO by seq within each group), or nullptr. ModbusDeviceCommand *select_next_ready_(); // Locate the single entry waiting for a response (WAITING/INTERRUPTED/WAITING_RETIRED/INTERRUPTED_RETIRED). ModbusDeviceCommand *find_waiting_(); @@ -349,13 +298,10 @@ class ModbusClientHub : public Modbus { // Transaction status: std::nullopt on success, otherwise a Modbus exception code using ResponseStatus = std::optional; -/// True when a transaction carried no exception. The optional holds the exception, so has_value() means -/// the request FAILED - the inverse of how "status" usually reads. Prefer this at the call site; the -/// bare !status.has_value() has already been mistaken for a failure check more than once. Where the code -/// is going to unwrap the exception anyway, status.has_value() followed by status.value() stays clearer. +/// True when a transaction carried no exception. inline bool succeeded(ResponseStatus status) { return !status.has_value(); } -// Register values exchanged with server handlers, in host byte order. Sized at the larger of the two protocol +// Register values exchanged with server handlers, in address order. Sized at the larger of the two protocol // maxima (read = 125 / 0x7D, write = 123 / 0x7B); the per-direction count limit is enforced by the hub, not by // the capacity of this type. using RegisterValues = StaticVector; @@ -373,59 +319,46 @@ class ModbusServerHub : public Modbus { void process_modbus_client_frame_(uint8_t address, uint8_t function_code, std::span data); // Dispatches a broadcast (address 0) write to every registered device; broadcasts are never answered. void process_broadcast_frame_(uint8_t function_code, std::span data); - // Parses a WRITE_SINGLE_REGISTER / WRITE_MULTIPLE_REGISTERS PDU into start_address and the host-order register - // values, validating the register count and address range. Returns std::nullopt on success, otherwise the Modbus - // exception code describing the failure. Shared by unicast writes (which reply with the exception) and broadcast - // writes (which silently drop invalid frames). + // Parses a WRITE_SINGLE_REGISTER / WRITE_MULTIPLE_REGISTERS PDU into start_address and the address order register + // values, validating the register count and address range. Shared by unicast and broadcast writes. ResponseStatus parse_write_single_(std::span data, uint16_t &start_address, RegisterValues ®isters); ResponseStatus parse_write_multiple_(std::span data, uint16_t &start_address, RegisterValues ®isters); - // Appends the big-endian register values in values to registers, in host byte order. + // Assembles host-order registers from the big-endian bytes in values and appends them to registers. void assemble_registers_(std::span values, RegisterValues ®isters); ModbusServerDevice *find_device_(uint8_t address); - // Returns std::nullopt if [start_address, start_address + count) fits in the 16-bit address space, - // otherwise ILLEGAL_DATA_ADDRESS. The caller sends the exception reply if one is required - a broadcast - // write is never answered, so the check cannot send it itself. Shared by the register and - // coil/discrete-input handlers, which all address the same 16-bit space. + // Returns std::nullopt if [start_address, start_address + count) fits in a 16-bit address space, otherwise + // ILLEGAL_DATA_ADDRESS. The caller sends the exception reply if one is required. Shared by the + // register/coil/discrete-input handlers, which all use a 16-bit address space. ResponseStatus check_address_range_(uint16_t start_address, uint16_t count); - // Parses a read request PDU (start address(2) + quantity(2)), shared by the register and - // coil/discrete-input reads so the two cannot drift apart. max_entities is the protocol ceiling for the - // function code; entity_name only labels the rejection log. + // Parses read request data. max_entities is the protocol ceiling for the function code; entity_name labels + // the rejection log. ResponseStatus parse_read_request_(std::span data, uint16_t max_entities, const LogString *entity_name, uint16_t &start_address, uint16_t &count); - // Parses a single-coil write PDU (FC 0x05), which carries a 2-byte on/off value rather than packed - // bytes. The caller packs value into a byte it owns to build the PackedBits view the handlers take. + // Parses single-coil write data ResponseStatus parse_write_single_coil_(std::span data, uint16_t &start_address, bool &value); - // Parses a multiple-coil write PDU (FC 0x0F) into a packed-bit view pointing straight into the receive - // buffer, so the coil values are never copied. Both coil parsers are shared by the addressed and - // broadcast paths so the two validate identically. + // Parses write-multiple-coil data into a packed-bit view pointing straight into the receive buffer, so the + // coil values are never copied. ResponseStatus parse_write_multiple_coils_(std::span data, uint16_t &start_address, uint16_t &count, std::span &packed_bytes); - // Builds the body of a register read response (byte count followed by the big-endian register values) into - // response_buffer. Shared by every function code that answers with register values, so the read reply stays - // identical across them. Returns false once an exception has been sent: the one the handler reported via - // status, or SERVICE_DEVICE_FAILURE if it returned the wrong number of registers, the count exceeds the - // protocol read limit, or the body does not fit. + // Builds the body of a register read response into response_buffer. Returns false once an exception has + // been sent: the one the handler reported via status, or SERVICE_DEVICE_FAILURE if it returned the wrong + // number of registers, the count exceeds the protocol read limit, or the body does not fit. bool build_or_reject_read_response_(uint8_t address, uint8_t function_code, ResponseStatus status, uint16_t number_of_registers, const RegisterValues ®isters, std::span response_buffer, uint16_t &response_len); void send_raw_(const uint8_t *payload, uint16_t len); // Sends and logs the exception reply when status holds one; returns true if the request was rejected. - // Every parse and handler rejection funnels through here, so the reply and its log cannot drift apart. bool rejected_(uint8_t address, uint8_t function_code, ResponseStatus status); void send_exception_(uint8_t address, uint8_t function_code, ExceptionCode exception_code); void send_response_(uint8_t address, uint8_t function_code, const uint8_t *payload, uint16_t payload_len); uint8_t expecting_peer_response_{0}; std::vector devices_; - // Stamp of the last "broadcast reached no device" warning, 0 until the first one is logged. Rate limiting - // on time rather than on address keeps the log bounded no matter how many addresses a shared bus carries. - uint32_t last_unaccepted_broadcast_warn_{0}; - // Holds the raw payload of a single reply deferred for sending when tx was blocked at send time. // Only one server reply can be waiting at once, so a single fixed buffer avoids heap allocation. std::array deferred_payload_; @@ -555,10 +488,7 @@ class ModbusClientDevice { helpers::create_client_pdu((FunctionCode) function, start_address, number_of_entities, payload, payload_len), this); } - /// See ModbusClientHub::queue_pdu(): true = accepted into the queue and a terminal callback will - /// follow (except a broadcast (address 0), which is never answered and so gets only on_sent()), - /// false = refused at the door and nothing further happens. Neither means the frame is on the wire; - /// on_sent() reports that. + /// See ModbusClientHub::queue_pdu() for the return contract. bool queue_pdu(std::span pdu, CommandOptions options = {}) { return this->parent_->queue_pdu(this->address_, pdu, this, options); } @@ -573,11 +503,8 @@ class ModbusClientDevice { return; // too short to contain a PDU; refused at the door like any invalid send this->parent_->queue_pdu(payload[0], std::span(payload).subspan(1), this); } - // The typed request builders below all queue through queue_pdu(), so they share its contract: true - // means the request is queued and will resolve in exactly one terminal callback (except a broadcast - // (address 0), which is never answered and so gets only on_sent()), false means it was refused outright - // with no callback. Neither says the frame has been transmitted - on_sent() does. - // Reads via the table-appropriate function code; an unreadable entity type maps to INVALID, which + // The typed request builders below all queue through queue_pdu() and share its return contract. + // Reads use the table-appropriate function code; an unreadable entity type maps to INVALID, which // create_read_pdu() rejects into an empty PDU and queue_pdu() refuses with a false return. bool read_entities(EntityType entity_type, uint16_t start_address, uint16_t number_of_entities, CommandOptions options = {}) { @@ -619,11 +546,9 @@ class ModbusClientDevice { bool write_multiple_coils(uint16_t start_address, PackedBits bits) { return this->queue_pdu(helpers::create_write_coils_pdu(start_address, bits)); } - /// FC 0x17: the read-back is delivered through on_read_holding_registers() (the response carries only the - /// read registers, the same wire shape as a holding-register read). A device exception - typically a - /// rejected write half - arrives at that same on_read_holding_registers() with the error in its status, - /// exactly as success does, so a subclass overriding that one callback handles both outcomes and never - /// needs to also override on_error(). + /// FC 0x17: the read-back is delivered through on_read_holding_registers(), and a device exception + /// (typically a rejected write half) arrives there too via its status - one callback handles both + /// outcomes with no on_error() override needed. bool read_write_multiple_registers(uint16_t read_start_address, uint16_t read_count, uint16_t write_start_address, std::span write_values) { return this->queue_pdu(helpers::create_read_write_multiple_registers_pdu(read_start_address, read_count, @@ -644,12 +569,9 @@ class ModbusClientDevice { bool custom_response_warned_{false}; // first unhandled custom response warns; repeats log at VERBOSE }; -// Compatibility shim for external components written against the pre-2026.8 API, which subclassed -// ModbusDevice and overrode on_modbus_data()/on_modbus_error(). The name is free (nothing in-tree -// uses it), so instead of a plain alias it adapts the new span-based hooks back to the old -// signatures: on_modbus_data() receives the response payload as an owning vector (the heap copy -// exists only on this deprecated path) and on_modbus_error() the function code and exception code. -// Remove before 2027.2.0 (window restarted when the plain alias became a behavior shim in 2026.8.0) +// Compatibility shim adapting the span-based hooks back to the pre-2026.8 on_modbus_data()/ +// on_modbus_error() signatures (the owning-vector heap copy exists only on this deprecated path). +// Remove before 2027.2.0 (window restarted when the plain alias became a behavior shim in 2026.8.0). class ESPDEPRECATED("Subclass ModbusClientDevice and override on_response()/on_error() instead. Removed in 2027.2.0", "2026.8.0") ModbusDevice : public ModbusClientDevice { public: diff --git a/esphome/components/modbus/modbus_definitions.h b/esphome/components/modbus/modbus_definitions.h index 83f314352f..089fc3d1ae 100644 --- a/esphome/components/modbus/modbus_definitions.h +++ b/esphome/components/modbus/modbus_definitions.h @@ -47,7 +47,6 @@ enum class FunctionCode : uint8_t { using ModbusFunctionCode ESPDEPRECATED("Use modbus::FunctionCode instead. Removed in 2027.2.0", "2026.8.0") = FunctionCode; -/*Allow direct comparison operators between FunctionCode and uint8_t*/ inline bool operator==(FunctionCode lhs, uint8_t rhs) { return static_cast(lhs) == rhs; } inline bool operator==(uint8_t lhs, FunctionCode rhs) { return lhs == static_cast(rhs); } inline bool operator!=(FunctionCode lhs, uint8_t rhs) { return !(static_cast(lhs) == rhs); } diff --git a/esphome/components/modbus/modbus_helpers.cpp b/esphome/components/modbus/modbus_helpers.cpp index db21b6e6fd..836d9b2d38 100644 --- a/esphome/components/modbus/modbus_helpers.cpp +++ b/esphome/components/modbus/modbus_helpers.cpp @@ -30,9 +30,11 @@ uint16_t server_pdu_length(const uint8_t *frame, size_t size) { switch (static_cast(frame[0])) { case FunctionCode::READ_COILS: case FunctionCode::READ_DISCRETE_INPUTS: + // function(1) + byte count(1) + packed coil bytes + return 2 + (size > 1 ? std::min(frame[1], uint8_t(packed_bit_bytes(MAX_NUM_OF_COILS_TO_READ))) : 0); case FunctionCode::READ_HOLDING_REGISTERS: case FunctionCode::READ_INPUT_REGISTERS: - // function(1) + byte count(1) + data + // function(1) + byte count(1) + register data return 2 + (size > 1 ? std::min(frame[1], uint8_t(MAX_NUM_OF_REGISTERS_TO_READ * 2)) : 0); case FunctionCode::WRITE_SINGLE_COIL: case FunctionCode::WRITE_SINGLE_REGISTER: @@ -60,6 +62,9 @@ uint16_t server_pdu_length(const uint8_t *frame, size_t size) { uint16_t client_pdu_length(const uint8_t *frame, size_t size) { if (size < MIN_PDU_SIZE) return MIN_PDU_SIZE; + if (is_function_code_exception(frame[0])) { + return 2; // never a valid request; sized like the exception reply so the CRC fails at once + } switch (static_cast(frame[0])) { case FunctionCode::READ_COILS: case FunctionCode::READ_DISCRETE_INPUTS: @@ -381,8 +386,6 @@ ReadPdu create_read_pdu(FunctionCode function_code, uint16_t start_address, uint PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, uint16_t number_of_entities, const uint8_t *values, size_t values_len) { PduBuffer pdu; // declared before every return so NRVO fires (all paths return the same object) - // Generic entry point; prefer the direction- and type-specific builders (create_read_pdu(), - // create_write_registers_pdu(), etc.) which bound their inputs per spec. if (is_function_code_read_only(static_cast(function_code))) { if (values != nullptr || values_len > 0) { ESP_LOGW(TAG, "Values provided for read function code %02X, but will be ignored", @@ -445,9 +448,7 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, return pdu; } // The quantity is spec-bounded above, so the data length just has to agree with it exactly - // (registers are 2 bytes each, coils pack 8 per byte). This is the same consistency the response - // dispatch enforces via is_client_pdu_standard(), so a frame built here can never be classified - // non-standard on reply, and the spec bound keeps the PDU within capacity by construction. + // (registers are 2 bytes each, coils pack 8 per byte). // Checked before the header append: a failed check must return an empty PDU, not a 5-byte partial one. const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS; const size_t expected_len = bits ? packed_bit_bytes(number_of_entities) : static_cast(number_of_entities) * 2; diff --git a/esphome/components/modbus/modbus_helpers.h b/esphome/components/modbus/modbus_helpers.h index 76056ed3e8..c3bccc4cba 100644 --- a/esphome/components/modbus/modbus_helpers.h +++ b/esphome/components/modbus/modbus_helpers.h @@ -60,14 +60,11 @@ inline bool is_function_code_custom(uint8_t function_code) { /// in step with those switches). Deliberately wider than is_function_code_custom(): the user-defined /// ranges are unknown to the parser too, but so are the assigned-but-unimplemented codes /// (READ_EXCEPTION_STATUS, DIAGNOSTICS, GET_COMM_EVENT_*, REPORT_SERVER_ID) and every unassigned value. -/// The 0x80 exception flag is masked off first, so a frame with it set classifies by its base code - -/// even though a spec exception reply has a known 2-byte PDU. That is deliberate, matching what -/// is_function_code_custom() has always done: some vendors use codes with the 0x80 bit set as ordinary -/// codes with longer payloads, so the response parser CRC-scans these rather than assuming the spec -/// length. For an intact spec exception the scan matches at its first candidate, so only a corrupt one -/// pays (recovery by timeout instead of an immediate CRC failure). +/// Exception-flagged codes (0x80 set) are always the 2-byte spec exception shape, so never unknown. inline bool is_function_code_unknown_length(uint8_t function_code) { - switch (static_cast(function_code & FUNCTION_CODE_MASK)) { + if (is_function_code_exception(function_code)) + return false; + switch (static_cast(function_code)) { case FunctionCode::READ_COILS: case FunctionCode::READ_DISCRETE_INPUTS: case FunctionCode::READ_HOLDING_REGISTERS: @@ -87,6 +84,17 @@ inline bool is_function_code_unknown_length(uint8_t function_code) { } } +/// True when the underlying function code (exception bit masked off) may be broadcast (address 0). +/// Refused: the reads (including read-write), plus every other code whose response length the parser +/// knows (file record, FIFO). Allowed: the writes, and any code the parser does not know, since the +/// hub cannot tell one of those apart from a vendor write. +inline bool is_function_code_broadcastable(uint8_t function_code) { + uint8_t masked_function_code = function_code & FUNCTION_CODE_MASK; + if (is_function_code_read(masked_function_code)) + return false; + return is_function_code_write(masked_function_code) || is_function_code_unknown_length(masked_function_code); +} + // Returns the expected length of a server response PDU based on the function code. // If too few bytes have arrived to determine the length, returns the minimum length. `size` is the // number of bytes available so far, which may exceed the eventual PDU (e.g. include the frame's CRC @@ -205,7 +213,7 @@ enum class SensorValueType : uint8_t { S_DWORD = 0x4, // 2 Registers signed BIT = 0x5, U_DWORD_R = 0x6, // 2 Registers unsigned - S_DWORD_R = 0x7, // 2 Registers unsigned + S_DWORD_R = 0x7, // 2 Registers signed U_QWORD = 0x8, S_QWORD = 0x9, U_QWORD_R = 0xA, @@ -280,7 +288,7 @@ inline uint8_t c_to_hex(char c) { return (c >= 'A') ? (c >= 'a') ? (c - 'a' + 10 * byte_from_hex_str("1122", 1) returns uint_8 value 0x22 == 34 * byte_from_hex_str("1122", 0) returns 0x11 * @param value string containing hex encoding - * @param position offset in bytes. Because each byte is encoded in 2 hex digits the position of the original byte in + * @param pos offset in bytes. Because each byte is encoded in 2 hex digits the position of the original byte in * the hex string is byte_pos * 2 * @return byte value */ @@ -292,8 +300,7 @@ inline uint8_t byte_from_hex_str(const std::string &value, uint8_t pos) { /** Get a word from a hex string * @param value string containing hex encoding - * @param position offset in bytes. Because each byte is encoded in 2 hex digits the position of the original byte in - * the hex string is byte_pos * 2 + * @param pos offset in bytes (see byte_from_hex_str) * @return word value */ inline uint16_t word_from_hex_str(const std::string &value, uint8_t pos) { @@ -302,8 +309,7 @@ inline uint16_t word_from_hex_str(const std::string &value, uint8_t pos) { /** Get a dword from a hex string * @param value string containing hex encoding - * @param position offset in bytes. Because each byte is encoded in 2 hex digits the position of the original byte in - * the hex string is byte_pos * 2 + * @param pos offset in bytes (see byte_from_hex_str) * @return dword value */ inline uint32_t dword_from_hex_str(const std::string &value, uint8_t pos) { @@ -312,8 +318,7 @@ inline uint32_t dword_from_hex_str(const std::string &value, uint8_t pos) { /** Get a qword from a hex string * @param value string containing hex encoding - * @param position offset in bytes. Because each byte is encoded in 2 hex digits the position of the original byte in - * the hex string is byte_pos * 2 + * @param pos offset in bytes (see byte_from_hex_str) * @return qword value */ inline uint64_t qword_from_hex_str(const std::string &value, uint8_t pos) { @@ -328,9 +333,9 @@ template T get_data(const std::vector &data, size_t buffer_ * Responses for coil are packed into bytes . * coil 3 is bit 3 of the first response byte * coil 9 is bit 2 of the second response byte - * @param coil number of the cil + * @param bit index of the bit to extract * @param data modbus response buffer (uint8_t) - * @return content of coil register + * @return value of the requested bit */ inline bool bit_from_packed(int bit, std::span data) { auto data_byte = bit / 8; diff --git a/tests/components/modbus/modbus_client_hub_test.cpp b/tests/components/modbus/modbus_client_hub_test.cpp index 026df34bcc..18c04f32d5 100644 --- a/tests/components/modbus/modbus_client_hub_test.cpp +++ b/tests/components/modbus/modbus_client_hub_test.cpp @@ -775,7 +775,7 @@ TEST(ModbusClientHubBroadcast, DeliversNoTerminalToTypedDevice) { // A broadcast is only meaningful for a command that changes state; a broadcast READ could never be // answered, so the hub refuses it at the door (false return, no entry queued) rather than silently -// retiring it. Writes, 0x17, and custom codes still go through (covered above). +// retiring it. Writes and custom/unknown codes still go through (covered in the neighboring tests). TEST(ModbusClientHubBroadcast, RefusesReadBroadcast) { NullUART uart; NoResponseProbeHub hub; @@ -814,9 +814,8 @@ TEST(ModbusClientHubBroadcast, AcceptsCustomBroadcast) { EXPECT_EQ(hub.entries(), 0u); // the entry is gone } -// An exception-flagged custom code (0x80 bit set) is not a real request: is_function_code_custom() masks -// the bit away and would accept it, but the broadcast guard excludes it, matching classify()'s handling -// of an exception-flagged write. +// An exception-flagged code (0x80 bit set) is never a valid request - that bit is response-only - so +// queue_pdu refuses it up front, before the broadcast guard, whatever its base code. TEST(ModbusClientHubBroadcast, RefusesExceptionFlaggedCustomBroadcast) { NullUART uart; NoResponseProbeHub hub; @@ -833,6 +832,50 @@ TEST(ModbusClientHubBroadcast, RefusesExceptionFlaggedCustomBroadcast) { EXPECT_EQ(device.sent_count_, 0); // never transmitted } +// FC23 (read/write multiple) has a read half that expects a reply, so the Modbus spec does not allow it +// as a broadcast. is_function_code_read() covers it, so the broadcast guard refuses it despite its write +// half. +TEST(ModbusClientHubBroadcast, RefusesReadWriteMultipleBroadcast) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + // fc, read start+qty, write start+qty, byte count, one data word. + const uint8_t read_write_multiple[] = {0x17, 0x00, 0x00, 0x00, 0x01, 0x00, 0x10, 0x00, 0x01, 0x02, 0xBE, 0xEF}; + EXPECT_FALSE(device.queue_pdu(read_write_multiple)); // its read half could never be answered + EXPECT_EQ(hub.entries(), 0u); +} + +// FC 0x18 (read FIFO queue) is not a "read" by is_function_code_read(), but the hub has an explicit +// response-length rule for it - it demonstrably expects a reply, so it cannot broadcast. +TEST(ModbusClientHubBroadcast, RefusesKnownLengthNonWriteBroadcast) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t read_fifo[] = {0x18, 0x00, 0x10}; // fc, FIFO pointer address + EXPECT_FALSE(device.queue_pdu(read_fifo)); + EXPECT_EQ(hub.entries(), 0u); +} + +// A code that is neither a read nor exception-flagged (here 0x63, unassigned) is fire-and-forget on a +// broadcast: the hub can't know it isn't a vendor write, so it is accepted and delivered to all devices. +TEST(ModbusClientHubBroadcast, AcceptsNonReadUnknownBroadcast) { + NullUART uart; + NoResponseProbeHub hub; + hub.set_uart_parent(&uart); + hub.setup(); + BroadcastProbeDevice device(&hub, BROADCAST_ADDRESS); + + const uint8_t unknown[] = {0x63, 0x00, 0x01}; + EXPECT_TRUE(device.queue_pdu(unknown)); // not a read, so not refused + EXPECT_EQ(hub.entries(), 1u); +} + namespace { // tx_blocked() clear for send_next_frame_'s gate, then blocked for send_frame_'s post-delay re-check. class RejectPostDelayHub : public NoResponseProbeHub { @@ -1882,30 +1925,20 @@ TEST(ModbusClientHubPriority, ResendFromOnResponseAbsorbsIntoCompletingCommand) EXPECT_FALSE(hub.queued(0).options.continuous); // the one-shot re-send downgraded the poll } -// An exception-flagged function code is never silently re-sendable, even though the read check -// masks the exception bit: its duplicate takes the drop path like any other non-read. -TEST(ModbusClientHubPriority, ExceptionFlaggedDuplicateDroppedNotPromoted) { +// The exception bit marks a response, so a request carrying it is refused outright. +TEST(ModbusClientHubPriority, ExceptionFlaggedPduRefused) { NoResponseProbeHub hub; SentCountingDevice device(&hub, 0x02); - const uint8_t weird[] = {0x83, 0x01, 0x00, 0x00, 0x02}; // read-shaped but exception-flagged - EXPECT_TRUE(device.queue_pdu(weird)); - EXPECT_FALSE(device.queue_pdu(weird)); // non-requeueable: cap of one, so the duplicate is refused + // The 0x80 exception flag is a response-only bit; a request must never set it. queue_pdu refuses an + // exception-flagged PDU up front - nothing is queued - whether its base code reads (0x83 = 0x03 | 0x80) + // or writes (0x86 = 0x06 | 0x80). + const uint8_t read_shaped[] = {0x83, 0x01, 0x00, 0x00, 0x02}; + const uint8_t write_shaped[] = {0x86, 0x00, 0x10, 0xBE, 0xEF}; + EXPECT_FALSE(device.queue_pdu(read_shaped)); + EXPECT_FALSE(device.queue_pdu(write_shaped)); hub.sweep_for_test(); - - ASSERT_EQ(hub.queued_frames(), 1u); - EXPECT_EQ(hub.queued(0).pending, 1u); - EXPECT_EQ(device.not_sent_count_, 0); - - // The write-shaped twin (0x86 masks to WRITE_SINGLE_REGISTER) must not take WRITE-class - // ordering either: exception-flagged codes are excluded from the mutates classification. - const uint8_t weird_write[] = {0x86, 0x00, 0x10, 0xBE, 0xEF}; - device.queue_pdu(weird_write); - ASSERT_EQ(hub.queued_frames(), 2u); - EXPECT_EQ(hub.queued(1).priority(), CommandPriority::READ); // not WRITE - const ModbusDeviceCommand *next = hub.next_ready(); - ASSERT_NE(next, nullptr); - EXPECT_EQ(next->frame.pdu()[0], 0x83); // FIFO by age: it did not jump the older entry + EXPECT_EQ(hub.queued_frames(), 0u); } namespace { diff --git a/tests/components/modbus/modbus_helpers_test.cpp b/tests/components/modbus/modbus_helpers_test.cpp index 53f51b016b..28573dc8a6 100644 --- a/tests/components/modbus/modbus_helpers_test.cpp +++ b/tests/components/modbus/modbus_helpers_test.cpp @@ -63,6 +63,12 @@ TEST(ModbusClientFrameLength, TooShortReturnsMinimum) { EXPECT_EQ(client_frame_length(frame, 1), MIN_FRAME_SIZE); } +TEST(ModbusClientFrameLength, ExceptionFlaggedIsTheExceptionShape) { + // Sized at 2 so an exception-flagged request fails its CRC at once instead of being scanned for. + const uint8_t exception_request[] = {0x83, 0x02}; + EXPECT_EQ(client_pdu_length(exception_request, sizeof(exception_request)), 2); +} + TEST(ModbusClientFrameLength, ReadAndWriteSingleAreFixed) { // basic_register request fixture is a read-holding request -> 8 bytes const uint8_t read[] = {0x01, 0x03, 0x00, 0x03, 0x00, 0x01, 0x74, 0x0A}; diff --git a/tests/components/modbus/modbus_unknown_function_test.cpp b/tests/components/modbus/modbus_unknown_function_test.cpp index 8b91d088b8..33f0cd24df 100644 --- a/tests/components/modbus/modbus_unknown_function_test.cpp +++ b/tests/components/modbus/modbus_unknown_function_test.cpp @@ -54,7 +54,8 @@ class TestServerHub : public ModbusServerHub { // The frame-length parsers have explicit cases for exactly these 13 codes; every other value - the // assigned-but-unimplemented management codes, both user-defined ranges, and all unassigned codes - -// must classify as unknown length. The exception flag masks off first. +// must classify as unknown length. Exception replies are always the 2-byte spec shape, so every +// 0x80-set code is known length. TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) { for (uint8_t fc : {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x0F, 0x10, 0x14, 0x15, 0x16, 0x17, 0x18}) { EXPECT_FALSE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << int(fc); @@ -62,11 +63,13 @@ TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) { for (uint8_t fc : {0x07, 0x08, 0x0B, 0x0C, 0x11, 0x2A, 0x41, 0x48, 0x49, 0x64, 0x6E, 0x00, 0x7F}) { EXPECT_TRUE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << int(fc); } - // Exception replies classify by their base code. + // Every exception-flagged code is known length (the 2-byte spec exception shape), whatever its base. EXPECT_FALSE(helpers::is_function_code_unknown_length(0x83)); - EXPECT_TRUE(helpers::is_function_code_unknown_length(0x87)); - // Strictly wider than the user-defined ranges: every custom code is unknown-length, but not vice versa. - for (int fc = 0; fc <= 0xFF; fc++) { + EXPECT_FALSE(helpers::is_function_code_unknown_length(0x87)); + EXPECT_FALSE(helpers::is_function_code_unknown_length(0xC9)); + // Strictly wider than the user-defined ranges below 0x80: every non-exception custom code is + // unknown-length, but not vice versa. + for (int fc = 0; fc <= 0x7F; fc++) { if (helpers::is_function_code_custom(fc)) EXPECT_TRUE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << fc; } @@ -75,10 +78,10 @@ TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) { // Derived contract check: the helper must say "unknown" exactly when both length parsers fall // through to default. With a zero-filled max-size PDU every explicit case returns at least 2 // (file records bottom out at 2, FIFO at 3) and only default returns MIN_PDU_SIZE, so comparing - // against MIN_PDU_SIZE detects a case added to either switch without updating the helper. The - // loop stops at 0x7F: above it the helper masks the exception flag off while client_pdu_length() - // switches on the unmasked byte and server_pdu_length() early-returns the exception length. - for (int fc = 0; fc <= 0x7F; fc++) { + // against MIN_PDU_SIZE detects a case added to either switch without updating the helper. Both + // parsers early-return the 2-byte exception shape above 0x7F, which the helper's own exception + // early-return mirrors, so the whole byte range is covered. + for (int fc = 0; fc <= 0xFF; fc++) { const uint8_t pdu[MAX_PDU_SIZE] = {static_cast(fc)}; // zero header fields EXPECT_EQ(helpers::is_function_code_unknown_length(fc), helpers::client_pdu_length(pdu, sizeof(pdu)) == MIN_PDU_SIZE) @@ -89,6 +92,17 @@ TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) { } } +// Broadcastable = writes plus unknown codes (possible vendor writes); everything known to expect a +// reply is not. Classifies the underlying code: the exception bit masks off first (0x85 as 0x05). +TEST(ModbusUnknownFunction, BroadcastableClassification) { + for (uint8_t fc : {0x05, 0x06, 0x0F, 0x10, 0x16, 0x49, 0x63, 0x6E, 0x85, 0xC9}) { + EXPECT_TRUE(helpers::is_function_code_broadcastable(fc)) << "fc 0x" << std::hex << int(fc); + } + for (uint8_t fc : {0x01, 0x02, 0x03, 0x04, 0x14, 0x15, 0x17, 0x18, 0x83, 0x97}) { + EXPECT_FALSE(helpers::is_function_code_broadcastable(fc)) << "fc 0x" << std::hex << int(fc); + } +} + // A response with a function code outside the user-defined ranges (0x49) has no length case in // server_pdu_length(), so the parser must find the frame end by CRC scan - the same way it already // handles user-defined codes. Frame: address + FC 0x49 + 3 data bytes + CRC = 7 bytes. Without the From 59397b4e28e2b7a7faec74969dc35bd1a0a0bc79 Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Fri, 28 Aug 2026 11:36:24 -0700 Subject: [PATCH 2/6] [modbus] Build single-value register writes on a right-sized stack buffer (#18844) Co-authored-by: J. Nick Koston --- esphome/components/modbus/modbus.h | 3 +++ .../components/modbus/modbus_definitions.h | 3 +++ esphome/components/modbus/modbus_helpers.cpp | 22 ++++++++++++++++--- esphome/components/modbus/modbus_helpers.h | 13 +++++++++++ esphome/core/helpers.h | 1 + .../components/modbus/modbus_helpers_test.cpp | 22 +++++++++++++++++++ 6 files changed, 61 insertions(+), 3 deletions(-) diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index 3ddaafb9fc..69a7eb82e3 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -534,6 +534,9 @@ class ModbusClientDevice { return this->queue_pdu(helpers::create_write_single_coil_pdu(address, value)); } bool write_multiple_registers(uint16_t start_address, std::span values) { + // Empty goes to the full-size builder so the rejection log names this method's limit, not the small one's. + if (!values.empty() && values.size() <= helpers::MAX_FEW_REGISTERS) + return this->queue_pdu(helpers::create_write_few_registers_pdu(start_address, values)); return this->queue_pdu(helpers::create_write_registers_pdu(start_address, values)); } /// Note: std::vector cannot bind to std::span; use a contiguous bool container or the packed diff --git a/esphome/components/modbus/modbus_definitions.h b/esphome/components/modbus/modbus_definitions.h index 089fc3d1ae..0939f9e76c 100644 --- a/esphome/components/modbus/modbus_definitions.h +++ b/esphome/components/modbus/modbus_definitions.h @@ -116,6 +116,9 @@ static constexpr uint16_t MAX_RAW_SIZE = 254; // Max RAW size is 256 - CRC(2) = static constexpr uint16_t READ_PDU_SIZE = 5; // A single-write PDU is always function code(1) + address(2) + value(2) static constexpr uint16_t WRITE_SINGLE_PDU_SIZE = 5; +// A multiple-write PDU starts with function code(1) + start address(2) + quantity(2) + byte count(1), +// followed by two bytes per register. +static constexpr uint16_t WRITE_MULTIPLE_HEADER_SIZE = 6; static constexpr uint16_t MAX_FRAME_SIZE = 256; // 4.1 Address 0 is the broadcast address: the request is processed by every device and never answered. diff --git a/esphome/components/modbus/modbus_helpers.cpp b/esphome/components/modbus/modbus_helpers.cpp index 836d9b2d38..92bd06cdf5 100644 --- a/esphome/components/modbus/modbus_helpers.cpp +++ b/esphome/components/modbus/modbus_helpers.cpp @@ -485,9 +485,12 @@ static bool register_block_in_range(const LogString *role, uint16_t start_addres return true; } -PduBuffer create_write_registers_pdu(uint16_t start_address, std::span values) { - PduBuffer pdu; // declared before every return so NRVO fires (all paths return the same object) - if (!register_block_in_range(LOG_STR("Write"), start_address, values.size(), MAX_NUM_OF_REGISTERS_TO_WRITE)) { +// The ceiling comes from the buffer itself: push_back() drops silently, so a bound wider than the buffer +// would put a truncated frame on the wire. +template static Pdu build_write_registers_pdu(uint16_t start_address, std::span values) { + constexpr auto max_registers = static_cast((Pdu::capacity() - WRITE_MULTIPLE_HEADER_SIZE) / 2); + Pdu pdu; // declared before every return so NRVO fires (all paths return the same object) + if (!register_block_in_range(LOG_STR("Write"), start_address, values.size(), max_registers)) { return pdu; } append_pdu_header(pdu, FunctionCode::WRITE_MULTIPLE_REGISTERS, start_address, values.size()); @@ -498,6 +501,19 @@ PduBuffer create_write_registers_pdu(uint16_t start_address, std::span values) { + return build_write_registers_pdu(start_address, values); +} + +WriteFewRegistersPdu create_write_few_registers_pdu(uint16_t start_address, std::span values) { + return build_write_registers_pdu(start_address, values); +} + PduBuffer create_read_write_multiple_registers_pdu(uint16_t read_start_address, uint16_t read_count, uint16_t write_start_address, std::span write_values) { diff --git a/esphome/components/modbus/modbus_helpers.h b/esphome/components/modbus/modbus_helpers.h index c3bccc4cba..a070ce250c 100644 --- a/esphome/components/modbus/modbus_helpers.h +++ b/esphome/components/modbus/modbus_helpers.h @@ -473,11 +473,15 @@ inline int64_t payload_to_number(const std::vector &data, SensorValueTy */ std::optional registers_to_number(const uint16_t *registers, size_t count, SensorValueType sensor_value_type); +/// The widest standard numeric value (a QWORD) spans 4 registers, so one entity value never writes more. +static constexpr uint16_t MAX_FEW_REGISTERS = 4; + // Named PDU buffer types: the builders' storage strategy (currently stack-allocated StaticVector, // right-sized per shape) can be swapped in one place without touching every signature. using PduBuffer = StaticVector; using ReadPdu = StaticVector; using WriteSinglePdu = StaticVector; +using WriteFewRegistersPdu = StaticVector; /// Scratch space for packing coils into wire layout: one bit per coil, sized for the spec maximum. using CoilPackBuffer = StaticVector; @@ -521,6 +525,15 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, */ PduBuffer create_write_registers_pdu(uint16_t start_address, std::span values); +/** Create modbus write multiple registers command (function 0x10) on a right-sized stack buffer. + * Identical wire bytes to create_write_registers_pdu() for any accepted input. + * @param start_address modbus address of the first register to write + * @param values register values to write, at most MAX_FEW_REGISTERS (an over-long or empty set is + * rejected and an empty PDU is returned) + * @return PDU (function code + data, no address, no CRC) + */ +WriteFewRegistersPdu create_write_few_registers_pdu(uint16_t start_address, std::span values); + /** Create modbus read/write multiple registers command * Function 0x17 Read/Write Multiple Registers * Writes write_values then reads read_count registers in one transaction (write first, per Modbus 6.17); diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 1ccc833048..a0afb03124 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -290,6 +290,7 @@ template class StaticVector { } size_t size() const { return count_; } + static constexpr size_t capacity() { return N; } bool empty() const { return count_ == 0; } // Direct access to underlying data diff --git a/tests/components/modbus/modbus_helpers_test.cpp b/tests/components/modbus/modbus_helpers_test.cpp index 28573dc8a6..87af49710f 100644 --- a/tests/components/modbus/modbus_helpers_test.cpp +++ b/tests/components/modbus/modbus_helpers_test.cpp @@ -489,6 +489,28 @@ TEST(ModbusTypedBuilders, WriteRegistersPduRejectsOverLimit) { EXPECT_FALSE(create_write_registers_pdu(0x0000, values).empty()); } +TEST(ModbusTypedBuilders, WriteFewRegistersPduMatchesFullSizeBuilder) { + static_assert(sizeof(WriteFewRegistersPdu) < sizeof(PduBuffer) / 4, + "WriteFewRegistersPdu must be meaningfully smaller"); + const uint16_t values[] = {0x000B, 0x0016, 0xABCD, 0xFF00}; + for (size_t count = 1; count <= MAX_FEW_REGISTERS; count++) { + auto small = create_write_few_registers_pdu(0x0102, std::span(values, count)); + auto full = create_write_registers_pdu(0x0102, std::span(values, count)); + EXPECT_EQ(std::vector(small.begin(), small.end()), std::vector(full.begin(), full.end())) + << count << " registers"; + EXPECT_EQ(small.size(), 6u + 2 * count); + EXPECT_TRUE(is_client_pdu_standard(small.data(), small.size())); + } +} + +TEST(ModbusTypedBuilders, WriteFewRegistersPduRejectsInvalidInput) { + const uint16_t values[MAX_FEW_REGISTERS + 1] = {0xAAAA, 0xAAAA, 0xAAAA, 0xAAAA, 0xAAAA}; + EXPECT_TRUE(create_write_few_registers_pdu(0x0000, values).empty()); + EXPECT_FALSE(create_write_few_registers_pdu(0x0000, std::span(values, MAX_FEW_REGISTERS)).empty()); + EXPECT_TRUE(create_write_few_registers_pdu(0x0000, std::span()).empty()); + EXPECT_TRUE(create_write_few_registers_pdu(0xFFFF, std::span(values, 2)).empty()); +} + TEST(ModbusTypedBuilders, ReadWriteMultipleRegistersPduWireBytes) { const uint16_t write_values[] = {0x000B, 0x0016}; // Read 2 registers at 0x0010, write 2 registers at 0x0020. From cc41fd0beb6b6651ca2f46185409a98e75e694a4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 13:42:42 -0500 Subject: [PATCH 3/6] [core] Make rmtree tolerate missing paths and concurrent directory changes (#18846) --- esphome/helpers.py | 48 +++++++++++++++++---- tests/unit_tests/test_helpers.py | 74 +++++++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/esphome/helpers.py b/esphome/helpers.py index a38fcaf821..3ccf0fe65a 100644 --- a/esphome/helpers.py +++ b/esphome/helpers.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Iterable, MutableMapping +from collections.abc import Callable, Iterable, MutableMapping from contextlib import suppress import ipaddress import logging @@ -456,23 +456,55 @@ def add_git_ceiling_directory(env: MutableMapping[str, str], directory: Path) -> env["GIT_CEILING_DIRECTORIES"] = os.pathsep.join(parts) -def rmtree(path: Path | str) -> None: - """Remove a directory tree, handling read-only files on Windows. +# Deletion attempts when a directory keeps being repopulated mid-delete +RMTREE_MAX_ATTEMPTS = 3 - On Windows, git pack files and other files may be marked read-only, - causing shutil.rmtree to fail. This handles that by removing the - read-only flag and retrying. + +def rmtree(path: Path | str) -> None: + """Remove a directory tree, tolerating common filesystem races. + + Read-only files (e.g. git pack files on Windows) get the read-only flag + removed and are retried. Paths that are already gone, whether the target + itself or entries vanishing mid-delete, are treated as removed. + Directories repopulated mid-delete (e.g. Finder recreating .DS_Store on + macOS) are retried a few times. """ + import errno import shutil + import time - def _onexc(func, path, exc): + def _onexc(func: Callable[..., object], path: str | Path, exc: OSError) -> None: + if isinstance(exc, FileNotFoundError): + _LOGGER.debug("rmtree: %s already gone", path) + return if os.access(path, os.W_OK): raise exc Path(path).chmod(stat.S_IWUSR | stat.S_IRUSR) func(path) - shutil.rmtree(path, onexc=_onexc) + last_err: OSError | None = None + for attempt in range(RMTREE_MAX_ATTEMPTS - 1): + try: + shutil.rmtree(path, onexc=_onexc) + return + except OSError as err: + if err.errno not in (errno.ENOTEMPTY, errno.EEXIST): + raise + _LOGGER.debug( + "rmtree: %s repopulated mid-delete (attempt %d): %s", + path, + attempt + 1, + err, + ) + last_err = err + # Give the racing writer (e.g. Finder) time to settle + time.sleep(0.05 * (attempt + 1)) + try: + shutil.rmtree(path, onexc=_onexc) + except OSError as err: + # Keep the earlier races visible in the traceback + raise err from last_err def walk_files(path: Path): diff --git a/tests/unit_tests/test_helpers.py b/tests/unit_tests/test_helpers.py index 53c326e0d0..ff82fa3c80 100644 --- a/tests/unit_tests/test_helpers.py +++ b/tests/unit_tests/test_helpers.py @@ -1,3 +1,4 @@ +import errno import io import logging import os @@ -5,7 +6,7 @@ from pathlib import Path import socket import stat import types -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch from aioesphomeapi.host_resolver import AddrInfo, IPv4Sockaddr, IPv6Sockaddr from hypothesis import given, settings @@ -966,6 +967,77 @@ def test_copy_file_if_changed_nonexistent_source(tmp_path: Path) -> None: helpers.copy_file_if_changed(src, dst) +def test_rmtree_removes_tree(tmp_path: Path) -> None: + """Test rmtree removes a populated directory tree.""" + target = tmp_path / "target" + (target / "sub").mkdir(parents=True) + (target / "sub" / "file.txt").write_text("content") + + helpers.rmtree(target) + assert not target.exists() + + +def test_rmtree_nonexistent_path(tmp_path: Path) -> None: + """Test rmtree on an already-removed path is a no-op.""" + helpers.rmtree(tmp_path / "gone") + + +def test_rmtree_retries_when_directory_repopulated(tmp_path: Path) -> None: + """Test rmtree retries when a file appears mid-delete (Finder .DS_Store race).""" + target = tmp_path / "target" + (target / "sub").mkdir(parents=True) + real_rmdir = os.rmdir + repopulated = False + + def racy_rmdir(path, **kwargs): + nonlocal repopulated + if not repopulated and Path(path).name == "target": + repopulated = True + (target / ".DS_Store").write_text("x") # Finder wins the race + real_rmdir(path, **kwargs) + + with patch("os.rmdir", side_effect=racy_rmdir), patch("time.sleep"): + helpers.rmtree(target) + assert repopulated + assert not target.exists() + + +def test_rmtree_raises_after_retries_exhausted(tmp_path: Path) -> None: + """Test rmtree gives up on a persistent ENOTEMPTY once attempts run out.""" + target = tmp_path / "target" + target.mkdir() + errs = [ + OSError(errno.ENOTEMPTY, "Directory not empty", str(target)) + for _ in range(helpers.RMTREE_MAX_ATTEMPTS) + ] + + with ( + patch("shutil.rmtree", side_effect=errs) as mock_rmtree, + patch("time.sleep") as mock_sleep, + pytest.raises(OSError, match="Directory not empty") as excinfo, + ): + helpers.rmtree(target) + assert mock_rmtree.call_count == helpers.RMTREE_MAX_ATTEMPTS + assert mock_sleep.call_args_list == [call(0.05), call(0.1)] + # Final failure chains to the last retried race + assert excinfo.value is errs[-1] + assert excinfo.value.__cause__ is errs[-2] + + +def test_rmtree_does_not_retry_other_oserror(tmp_path: Path) -> None: + """Test rmtree raises non-ENOTEMPTY errors immediately.""" + target = tmp_path / "target" + target.mkdir() + err = OSError(errno.EACCES, "Permission denied", str(target)) + + with ( + patch("shutil.rmtree", side_effect=err) as mock_rmtree, + pytest.raises(OSError, match="Permission denied"), + ): + helpers.rmtree(target) + assert mock_rmtree.call_count == 1 + + def test_resolve_ip_address_sorting() -> None: """Test that results are sorted by preference.""" # Create multiple address infos with different preferences From 4333870590953b001e3a60340c2d6e4b1d7b675f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 13:43:48 -0500 Subject: [PATCH 4/6] [core] Use uv for the ESP-IDF Python environment when available (#18838) --- esphome/espidf/framework.py | 33 ++++++++++++++++------- tests/unit_tests/test_espidf_framework.py | 27 +++++++++++++++++++ 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index 6c2a285360..9373b5f569 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -1053,15 +1053,28 @@ def _check_esp_idf_python_env_install( constraint_file_path, ) - cmd_pip_install = [ - str(env_python_path), - "-m", - "pip", - "install", - "--upgrade", - "--constraint", - constraint_file_path, - ] + # uv (much faster than pip) when available, e.g. in the docker image + if uv_path := shutil.which("uv"): + cmd_pip_install = [ + uv_path, + "pip", + "install", + "--python", + str(env_python_path), + "--upgrade", + "--constraint", + str(constraint_file_path), + ] + else: + cmd_pip_install = [ + str(env_python_path), + "-m", + "pip", + "install", + "--upgrade", + "--constraint", + str(constraint_file_path), + ] _LOGGER.info("Installing ESP-IDF %s Python dependencies ...", version) cmd = cmd_pip_install + [ @@ -1135,6 +1148,8 @@ def check_esp_idf_install( env = {} env["IDF_TOOLS_PATH"] = str(get_idf_tools_path()) env["IDF_PATH"] = "" + # uv defaults to 3 HTTP retries; match the pioarduino penv's bump to 10 + env["UV_HTTP_RETRIES"] = os.environ.get("UV_HTTP_RETRIES", "10") # An explicit ESPHOME_IDF_DEFAULT_TARGETS wins over the caller's # per-variant request (builder-image pre-warm); otherwise the caller's diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index afa4433aa1..3eeace9914 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -520,6 +520,33 @@ def test_check_esp_idf_install_feature_failure(espidf_mocks: SimpleNamespace) -> check_esp_idf_install(_IDF_VERSION, force=True, features=["fb"]) +def test_python_deps_use_uv_when_available( + espidf_mocks: SimpleNamespace, monkeypatch: pytest.MonkeyPatch +) -> None: + """The python env installs go through uv when on the PATH, pip otherwise.""" + monkeypatch.delenv("UV_HTTP_RETRIES", raising=False) + with patch( + "esphome.espidf.framework.shutil.which", + # Keyed on the name: the same which() also probes the default tools + side_effect=lambda name: "/usr/bin/uv" if name == "uv" else None, + ): + check_esp_idf_install(_IDF_VERSION, force=True, features=["fb"]) + upgrade_call, feature_call = espidf_mocks.run_ok.call_args_list[1:3] + upgrade_cmd, feature_cmd = upgrade_call.args[0], feature_call.args[0] + assert upgrade_cmd[:3] == ["/usr/bin/uv", "pip", "install"] + assert "--python" in upgrade_cmd + assert feature_cmd[:3] == ["/usr/bin/uv", "pip", "install"] + assert upgrade_call.kwargs["env"]["UV_HTTP_RETRIES"] == "10" + + espidf_mocks.run_ok.reset_mock() + monkeypatch.setenv("UV_HTTP_RETRIES", "3") # an explicit user value wins + with patch("esphome.espidf.framework.shutil.which", return_value=None): + check_esp_idf_install(_IDF_VERSION, force=True, features=["fb"]) + upgrade_call = espidf_mocks.run_ok.call_args_list[1] + assert upgrade_call.args[0][1:4] == ["-m", "pip", "install"] + assert upgrade_call.kwargs["env"]["UV_HTTP_RETRIES"] == "3" + + def _mark_installed() -> None: """Create the extracted marker and python-env interpreter so the install check takes the already-installed path rather than force-installing.""" From 1623fe0852722b9c93e767aa2af5557389486d52 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 13:49:21 -0500 Subject: [PATCH 5/6] [esp32] Exclude the WiFi and Bluetooth stacks from builds that do not use them (#18599) --- esphome/components/esp32/__init__.py | 15 +++++++++ esphome/components/espnow/__init__.py | 5 +++ esphome/components/mdns/__init__.py | 8 +++++ esphome/components/zigbee/zigbee_esp32.py | 5 +++ .../config/exclusion_reincludes_espnow.yaml | 11 +++++++ .../config/exclusion_reincludes_wifi_ble.yaml | 13 ++++++++ .../esp32/config/network_ethernet_only.yaml | 2 ++ .../esp32/config/network_wifi_only.yaml | 2 ++ tests/component_tests/esp32/test_esp32.py | 33 ++++++++++++++++--- 9 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 tests/component_tests/esp32/config/exclusion_reincludes_espnow.yaml create mode 100644 tests/component_tests/esp32/config/exclusion_reincludes_wifi_ble.yaml diff --git a/esphome/components/esp32/__init__.py b/esphome/components/esp32/__init__.py index bc91f29a42..48bb7bf6a1 100644 --- a/esphome/components/esp32/__init__.py +++ b/esphome/components/esp32/__init__.py @@ -214,11 +214,13 @@ COMPILER_OPTIMIZATIONS = { # builds that need them. DEFAULT_EXCLUDED_IDF_COMPONENTS = ( "app_trace", # CPU trace/SystemView support - unused by ESPHome + "bt", # Bluetooth stack - re-included by request_bluetooth(); its REQUIRES pulls the WiFi stack back "cmock", # Unit testing mock framework - ESPHome doesn't use IDF's testing "console", # Console REPL - unused by ESPHome; espressif/mdns pulls it back when configured "driver", # Legacy driver shim - only needed by esp32_touch, esp32_can for legacy headers "esp-tls", # TLS wrapper - re-included by http_request, mqtt, web_server_idf "esp_adc", # ADC driver - only needed by adc component + "esp_coex", # WiFi/BT coexistence - re-included by esp32_ble_tracker, zigbee; esp_wifi/bt pull it back "esp_driver_cam", # Camera driver - the esp32-camera managed component pulls it back "esp_driver_dac", # DAC driver - only needed by esp32_dac component "esp_driver_gptimer", # General purpose timer - re-included by ac_dimmer, opentherm, Arduino BLE libs @@ -236,6 +238,7 @@ DEFAULT_EXCLUDED_IDF_COMPONENTS = ( "esp_driver_twai", # TWAI/CAN driver - only needed by esp32_can component "esp_eth", # Ethernet driver - only needed by ethernet component "esp_gdbstub", # GDB stub panic handler - unused by ESPHome; bt pulls it back + "esp_hal_ieee802154", # 802.15.4 HAL - ieee802154 pulls it back "esp_hid", # HID host/device support - ESPHome doesn't implement HID functionality "esp_http_client", # HTTP client - only needed by http_request component "esp_http_server", # HTTP server - re-included by web_server_idf, esp32_camera_web_server @@ -243,8 +246,11 @@ DEFAULT_EXCLUDED_IDF_COMPONENTS = ( "esp_https_server", # HTTPS server - ESPHome has its own web server "esp_lcd", # LCD controller drivers - only needed by display component "esp_local_ctrl", # Local control over HTTPS/BLE - ESPHome has native API + "esp_phy", # RF PHY - esp_wifi/bt/ieee802154 pull it back when they are in the build + "esp_wifi", # WiFi stack - re-included by request_wifi(), espnow; bt pulls it back for BLE builds "espcoredump", # Core dump support - ESPHome has its own debug component "fatfs", # FAT filesystem - ESPHome doesn't use filesystem storage + "ieee802154", # 802.15.4 radio - IDF openthread and the Zigbee libs pull it back "json", # cJSON library - ESPHome uses ArduinoJson instead "mqtt", # ESP-IDF MQTT library - ESPHome has its own MQTT implementation "nvs_sec_provider", # NVS encryption key provider - re-included when CONFIG_NVS_ENCRYPTION is set @@ -260,6 +266,7 @@ DEFAULT_EXCLUDED_IDF_COMPONENTS = ( "unity", # Unit testing framework - ESPHome doesn't use IDF's testing "wear_levelling", # Flash wear levelling for fatfs - unused since fatfs unused "wifi_provisioning", # WiFi provisioning - ESPHome uses its own improv implementation + "wpa_supplicant", # WPA supplicant - re-included by request_wifi() for esp_eap_client.h ) # Additional IDF managed components to exclude for Arduino framework builds @@ -709,6 +716,9 @@ def request_wifi(ap: bool = False) -> None: net.wifi = True if ap: net.wifi_ap = True + include_builtin_idf_component("esp_wifi") + # wifi_component.cpp includes esp_eap_client.h/esp_wpa2.h + include_builtin_idf_component("wpa_supplicant") def request_ethernet() -> None: @@ -720,11 +730,14 @@ def request_bluetooth() -> None: """Request the Bluetooth controller.""" net = _network_sdkconfig() net.bluetooth = True + include_builtin_idf_component("bt") def request_software_coexistence() -> None: """Request WiFi/BT software coexistence (only valid alongside WiFi).""" _network_sdkconfig().software_coexistence = True + # Callers include esp_coexist.h directly. + include_builtin_idf_component("esp_coex") def add_idf_component( @@ -2304,6 +2317,8 @@ async def _reconcile_network_sdkconfig() -> None: # WiFi stack: disable only when Ethernet is present and WiFi is not. WiFi # relies on the IDF default (enabled), so it is never written True here. + # esp_wifi is excluded by default on IDF, so this only matters for Arduino + # or when bt pulls it back. wifi_disabled = net.ethernet and not net.wifi if wifi_disabled: set_idf_sdkconfig_default("CONFIG_ESP_WIFI_ENABLED", False) diff --git a/esphome/components/espnow/__init__.py b/esphome/components/espnow/__init__.py index ee3732c406..5541a6ee97 100644 --- a/esphome/components/espnow/__init__.py +++ b/esphome/components/espnow/__init__.py @@ -155,6 +155,11 @@ async def to_code(config: ConfigType) -> None: cg.add_define("USE_ESPNOW") cg.add_define("USE_ESPNOW_MAX_PAYLOAD_SIZE", config[CONF_MAX_PAYLOAD_SIZE]) + if CORE.is_esp32: + from esphome.components.esp32 import include_builtin_idf_component + + include_builtin_idf_component("esp_wifi") + if CONF_WIFI in CORE.config: # Track the Wi-Fi channel via connect events instead of polling every loop wifi.request_wifi_connect_state_listener() diff --git a/esphome/components/mdns/__init__.py b/esphome/components/mdns/__init__.py index 64d7b9adc5..f039bb69f0 100644 --- a/esphome/components/mdns/__init__.py +++ b/esphome/components/mdns/__init__.py @@ -9,6 +9,7 @@ from esphome.const import ( CONF_PROTOCOL, CONF_SERVICE, CONF_SERVICES, + CONF_WIFI, PlatformFramework, ) from esphome.core import CORE, Lambda, coroutine_with_priority @@ -211,6 +212,13 @@ async def to_code(config: ConfigType) -> None: add_idf_component(name="espressif/mdns", ref="1.12.0") # ESPHome only advertises; the browse APIs are unused add_idf_sdkconfig_option("CONFIG_MDNS_ENABLE_BROWSE", False) + # The mdns console CLI is never used by ESPHome + add_idf_sdkconfig_option("CONFIG_MDNS_ENABLE_CONSOLE_CLI", False) + if CONF_WIFI not in CORE.config: + # Without WiFi the predefined STA/AP interface handlers are dead + # code; disabling them lets mdns build without the WiFi stack. + add_idf_sdkconfig_option("CONFIG_MDNS_PREDEF_NETIF_STA", False) + add_idf_sdkconfig_option("CONFIG_MDNS_PREDEF_NETIF_AP", False) cg.add_define("USE_MDNS") diff --git a/esphome/components/zigbee/zigbee_esp32.py b/esphome/components/zigbee/zigbee_esp32.py index ade45e8cc3..57fa3b2a00 100644 --- a/esphome/components/zigbee/zigbee_esp32.py +++ b/esphome/components/zigbee/zigbee_esp32.py @@ -9,6 +9,7 @@ from esphome.components.esp32 import ( add_idf_component, add_idf_sdkconfig_option, add_partition, + include_builtin_idf_component, require_vfs_select, ) import esphome.config_validation as cv @@ -288,6 +289,10 @@ async def esp32_to_code(config: ConfigType) -> "MockObj": ref="2.0.4", ) + if CONF_WIFI in CORE.config: + # zigbee_esp32.cpp uses esp_coexist.h when WiFi is present + include_builtin_idf_component("esp_coex") + # add sdkconfigs later so they can overwrite esp32 defaults CORE.add_job(_zigbee_add_sdkconfigs, config) diff --git a/tests/component_tests/esp32/config/exclusion_reincludes_espnow.yaml b/tests/component_tests/esp32/config/exclusion_reincludes_espnow.yaml new file mode 100644 index 0000000000..2ede6c42df --- /dev/null +++ b/tests/component_tests/esp32/config/exclusion_reincludes_espnow.yaml @@ -0,0 +1,11 @@ +esphome: + name: test + +esp32: + board: esp32dev + framework: + type: esp-idf + +espnow: + channel: 1 + auto_add_peer: true diff --git a/tests/component_tests/esp32/config/exclusion_reincludes_wifi_ble.yaml b/tests/component_tests/esp32/config/exclusion_reincludes_wifi_ble.yaml new file mode 100644 index 0000000000..883abdb5fa --- /dev/null +++ b/tests/component_tests/esp32/config/exclusion_reincludes_wifi_ble.yaml @@ -0,0 +1,13 @@ +esphome: + name: test + +esp32: + board: esp32dev + framework: + type: esp-idf + +wifi: + ssid: MySSID + password: password1 + +esp32_ble_tracker: diff --git a/tests/component_tests/esp32/config/network_ethernet_only.yaml b/tests/component_tests/esp32/config/network_ethernet_only.yaml index 73d11e0a13..4f357e40e6 100644 --- a/tests/component_tests/esp32/config/network_ethernet_only.yaml +++ b/tests/component_tests/esp32/config/network_ethernet_only.yaml @@ -6,6 +6,8 @@ esp32: framework: type: esp-idf +mdns: + ethernet: type: W5500 clk_pin: 19 diff --git a/tests/component_tests/esp32/config/network_wifi_only.yaml b/tests/component_tests/esp32/config/network_wifi_only.yaml index 61dfde3e03..3abc17e324 100644 --- a/tests/component_tests/esp32/config/network_wifi_only.yaml +++ b/tests/component_tests/esp32/config/network_wifi_only.yaml @@ -6,6 +6,8 @@ esp32: framework: type: esp-idf +mdns: + wifi: ssid: "test_ssid" password: "test_password" diff --git a/tests/component_tests/esp32/test_esp32.py b/tests/component_tests/esp32/test_esp32.py index 297844b4e6..c72c4c3a6b 100644 --- a/tests/component_tests/esp32/test_esp32.py +++ b/tests/component_tests/esp32/test_esp32.py @@ -24,6 +24,7 @@ from esphome.components.esp32 import ( ) from esphome.components.esp32.const import ( KEY_ESP32, + KEY_EXCLUDE_COMPONENTS, KEY_NETWORK_SDKCONFIG, KEY_SDKCONFIG_OPTIONS, KEY_VARIANT, @@ -298,6 +299,20 @@ def test_esp32_configuration_errors( ("esp-tls", "esp_http_client"), id="nextion", ), + pytest.param( + # esp_wifi/wpa_supplicant from request_wifi(), bt from + # request_bluetooth(), esp_coex from esp32_ble_tracker's software + # coexistence (defaults on with wifi). esp_phy stays excluded; + # IDF requirement expansion pulls it back via esp_wifi. + "exclusion_reincludes_wifi_ble.yaml", + ("esp_wifi", "wpa_supplicant", "bt", "esp_coex"), + id="wifi_ble", + ), + pytest.param( + "exclusion_reincludes_espnow.yaml", + ("esp_wifi",), + id="espnow", + ), ], ) def test_default_exclusions_reincluded_by_owning_components( @@ -309,8 +324,6 @@ def test_default_exclusions_reincluded_by_owning_components( """Components whose IDF driver is excluded by default must re-include it during codegen; a dropped include_builtin_idf_component() call would only surface as a missing-header failure in a full compile job.""" - from esphome.components.esp32.const import KEY_EXCLUDE_COMPONENTS - generate_main(component_config_path(config_file)) excluded = CORE.data[KEY_ESP32][KEY_EXCLUDE_COMPONENTS] @@ -329,8 +342,6 @@ def test_nvs_sec_provider_stays_excluded_when_encryption_is_off( component_config_path: Callable[[str], Path], ) -> None: """An explicit CONFIG_NVS_ENCRYPTION=n keeps nvs_sec_provider excluded.""" - from esphome.components.esp32.const import KEY_EXCLUDE_COMPONENTS - generate_main(component_config_path("exclusion_stays_nvs_sdkconfig_off.yaml")) assert "nvs_sec_provider" in CORE.data[KEY_ESP32][KEY_EXCLUDE_COMPONENTS] @@ -939,6 +950,14 @@ def test_network_wifi_only_reconciles_end_to_end( sdkconfig = CORE.data[KEY_ESP32][KEY_SDKCONFIG_OPTIONS] assert sdkconfig.get("CONFIG_ESP_WIFI_SOFTAP_SUPPORT") is False assert sdkconfig.get("CONFIG_LWIP_DHCPS") is False + # request_wifi() also puts the WiFi components back in the build set; + # esp_phy stays excluded, IDF requirement expansion pulls it back. + excluded = CORE.data[KEY_ESP32][KEY_EXCLUDE_COMPONENTS] + assert "esp_wifi" not in excluded + assert "wpa_supplicant" not in excluded + assert "esp_phy" in excluded + # With wifi present mdns keeps its predefined interfaces. + assert "CONFIG_MDNS_PREDEF_NETIF_STA" not in sdkconfig # WiFi stack stays enabled (no ethernet) and no Bluetooth requested. assert "CONFIG_ESP_WIFI_ENABLED" not in sdkconfig assert "CONFIG_BT_ENABLED" not in sdkconfig @@ -954,6 +973,12 @@ def test_network_ethernet_only_reconciles_end_to_end( sdkconfig = CORE.data[KEY_ESP32][KEY_SDKCONFIG_OPTIONS] assert sdkconfig.get("CONFIG_ESP_WIFI_ENABLED") is False assert sdkconfig.get("CONFIG_SW_COEXIST_ENABLE") is False + # The whole radio stack stays out of the build set as well. + excluded = CORE.data[KEY_ESP32][KEY_EXCLUDE_COMPONENTS] + assert {"esp_wifi", "wpa_supplicant", "esp_phy", "esp_coex", "bt"} <= excluded + # Without wifi, mdns drops its predefined STA/AP interfaces. + assert sdkconfig.get("CONFIG_MDNS_PREDEF_NETIF_STA") is False + assert sdkconfig.get("CONFIG_MDNS_PREDEF_NETIF_AP") is False def test_network_wifi_ble_coexistence_reconciles_end_to_end( From 0dce7f48459fd1a3f0b954b83126bcb4407ea34c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 13:50:52 -0500 Subject: [PATCH 6/6] [esp8266] Add the native library backend (#18558) --- esphome/arduino/__init__.py | 0 esphome/arduino/library.py | 531 ++++++++ esphome/build_gen/build_tool.py | 108 ++ esphome/platformio/library.py | 109 +- tests/unit_tests/build_gen/test_build_tool.py | 246 ++++ tests/unit_tests/test_arduino_library.py | 1162 +++++++++++++++++ tests/unit_tests/test_platformio_library.py | 209 ++- 7 files changed, 2355 insertions(+), 10 deletions(-) create mode 100644 esphome/arduino/__init__.py create mode 100644 esphome/arduino/library.py create mode 100644 esphome/build_gen/build_tool.py create mode 100644 tests/unit_tests/build_gen/test_build_tool.py create mode 100644 tests/unit_tests/test_arduino_library.py diff --git a/esphome/arduino/__init__.py b/esphome/arduino/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py new file mode 100644 index 0000000000..e224e62589 --- /dev/null +++ b/esphome/arduino/library.py @@ -0,0 +1,531 @@ +"""Arduino-core backend for the shared PlatformIO library converter. + +Bundled names build straight from the framework tree; everything else goes +through ``esphome.platformio.library``. Mirrors ``lib_ldf_mode=off``: each +library builds its own archive; all include dirs join one global path. + +Deviations from PlatformIO: flat-layout libraries get the recursive default +source filter; ``dot_a_linkage`` is honored; bundled libraries never run a +manifest ``extraScript``; manifest ``-I`` flags join the global include path; +``precompiled``/``ldflags`` properties are refused by name. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +import logging +from pathlib import Path +import re + +from esphome.core import CORE, EsphomeError, Library +from esphome.helpers import walk_files +from esphome.platformio.extra_script import apply_extra_script +from esphome.platformio.library import ( + DEFAULT_BUILD_INCLUDE_DIR, + DEFAULT_BUILD_SRC_FILTER, + ESPHOME_DATA_KEY, + ESPHOME_DATA_LINK_FLAGS_KEY, + LIBRARY_HEADER_SUFFIXES, + SRC_FILE_EXTENSIONS, + ConvertedLibrary, + IncompatiblePlatform, + InvalidLibrary, + LibraryBackend, + _url_or_none, + check_library_data, + collect_filtered_files, + convert_libraries, + ensure_list, + is_lib_ignored, + lex_build_flags, + lib_ignore_set, + normalize_dependencies, + parse_library_json, + parse_library_properties, + warn_properties_depends, +) + +_LOGGER = logging.getLogger(__name__) + + +@dataclass +class ArduinoLibrary: + """One resolved library, ready for the ninja generator.""" + + name: str + sources: list[Path] = field(default_factory=list) + include_dirs: list[Path] = field(default_factory=list) + # Extra compile flags private to this library's own sources + flags: list[str] = field(default_factory=list) + # PlatformIO's build.libArchive / Arduino's dot_a_linkage: when False the + # objects go to the linker directly (symbols nothing references survive) + lib_archive: bool = True + # Link inputs the library contributes (-L dirs / -l libs, e.g. from + # precompiled vendor blobs) and -Wl, options for the firmware link + link_dirs: list[Path] = field(default_factory=list) + link_libs: list[str] = field(default_factory=list) + link_flags: list[str] = field(default_factory=list) + + +# Source-like suffixes the case-sensitive suffix map rejects +_UNMAPPED_SOURCE_SUFFIXES = frozenset( + {s.lower() for s in SRC_FILE_EXTENSIONS} | {".ino"} +) + +# Filename-plain names: an allowlist excludes separators, drive colons, +# and dot-only names by shape +_SAFE_LIBRARY_NAME_RE = re.compile(r"[A-Za-z0-9_][A-Za-z0-9_. +-]*\Z") + + +def _is_safe_library_name(name: object) -> bool: + """Whether a name may be joined under the framework's libraries dir.""" + return isinstance(name, str) and _SAFE_LIBRARY_NAME_RE.fullmatch(name) is not None + + +def _manifest_build(name: str, data: object) -> dict: + """The manifest's ``build`` section; malformed manifests fail by name.""" + build = data.get("build", {}) if isinstance(data, dict) else None + if not isinstance(build, dict): + raise EsphomeError(f"Library {name} has a malformed manifest") + return build + + +def _resolve_src_dir(name: str, read_path: Path, build: dict) -> str: + """Resolve PIO's source dir: manifest srcDir, else src/Src, else the root.""" + if "srcDir" not in build: + return next((d for d in ("src", "Src") if (read_path / d).is_dir()), ".") + # A declared srcDir (falsy included) that does not resolve is a manifest error + src_dir = build["srcDir"] + if not (isinstance(src_dir, str) and src_dir and (read_path / src_dir).is_dir()): + raise EsphomeError( + f"Library {name} declares srcDir {src_dir!r} which does not exist" + ) + return src_dir + + +def _reject_unsupported_link_fields(name: str, data: dict) -> None: + # PIO honors these; ignoring them would fail at link with no stated + # cause. Property values are strings, so "false" is not a declaration. + precompiled = data.get("precompiled") + if precompiled and str(precompiled).strip().lower() != "false": + raise EsphomeError( + f"Library {name} declares precompiled, which this backend does not support" + ) + if data.get("ldflags"): + raise EsphomeError( + f"Library {name} declares ldflags, which this backend does not support" + ) + + +def _resolve_lib_archive(name: str, data: dict, build: dict) -> bool: + """build.libArchive, else dot_a_linkage (an Arduino IDE property PIO + ignores; a deliberate extra), else archive.""" + + # Strict parse: bool("false") is True + def _parse(key: str, raw: object) -> bool: + if isinstance(raw, bool): + return raw + value = str(raw).strip().lower() + if value in ("true", "false"): + return value == "true" + raise EsphomeError(f"Library {name} has a malformed {key} value {raw!r}") + + if "libArchive" in build: + return _parse("libArchive", build["libArchive"]) + if "dot_a_linkage" in data: + return _parse("dot_a_linkage", data["dot_a_linkage"]) + return True + + +def _classify_build_flags( + name: str, read_path: Path, lib: ArduinoLibrary, flag_tokens: list[str] +) -> list[str]: + """Route the lexed build.flags into the library's flag lists. + + Returns the ``-I`` arguments for the include-dir resolution. + """ + include_flags: list[str] = [] + for tok in flag_tokens: + if tok.startswith("-I"): + include_flags.append(tok[2:]) + elif tok.startswith("-L"): + link_dir = (read_path / tok[2:]).resolve() + if not link_dir.is_dir(): + # Kept (the linker ignores missing -L dirs); the warning + # names the culprit before a bare "cannot find -lfoo" + _LOGGER.warning( + "Library %s declares library dir %s which does not exist", + name, + tok[2:], + ) + lib.link_dirs.append(link_dir) + elif tok.startswith("-l"): + lib.link_libs.append(tok[2:]) + elif tok.startswith("-Wl,"): + lib.link_flags.append(tok) + else: + lib.flags.append(tok) + return include_flags + + +def _resolve_include_dirs( + name: str, + read_path: Path, + lib: ArduinoLibrary, + build: dict, + src_dir: str, + include_flags: list[str], +) -> None: + include_dir = build.get("includeDir", DEFAULT_BUILD_INCLUDE_DIR) + if not isinstance(include_dir, str): + raise EsphomeError(f"Library {name} has a malformed includeDir") + for d, explicit in [ + (include_dir, "includeDir" in build), + (src_dir, False), # _resolve_src_dir already validated it + *((flag, True) for flag in include_flags), + ]: + if (path := (read_path / d)).is_dir(): + lib.include_dirs.append(path.resolve()) + elif explicit: + # Warn-and-drop (unlike srcDir): a missing include dir is + # harmless until a header is needed, and the compile names it + _LOGGER.warning( + "Library %s declares include dir %s which does not exist", name, d + ) + + +def _collect_lib_sources( + name: str, + read_path: Path, + lib: ArduinoLibrary, + src_dir: str, + src_filter: list[str], +) -> None: + sources: list[Path] = [] + dropped: list[str] = [] + saw_header = False + for f in collect_filtered_files(read_path / src_dir, src_filter): + path = Path(f) + suffix = path.suffix + if suffix in SRC_FILE_EXTENSIONS: + # resolve() per file: srcFilter patterns may escape src_dir + sources.append(path.resolve()) + elif suffix.lower() in _UNMAPPED_SOURCE_SUFFIXES: + # A source-like suffix the case-sensitive map rejects (.CPP, + # .ino) is a dropped compilation unit; headers fall through + dropped.append(path.name) + elif suffix.lower() in LIBRARY_HEADER_SUFFIXES: + saw_header = True + lib.sources = sorted(sources) + if dropped: + _LOGGER.warning( + "Library %s: %d file(s) with unmapped source suffixes are not compiled: %s", + name, + len(dropped), + ", ".join(sorted(dropped)), + ) + if not lib.sources and not saw_header: + # Matched headers mean header-only; a filter matching nothing is + # a manifest/tree problem (a truly empty tree raises elsewhere) + _LOGGER.warning("Library %s: no source files matched", name) + + +def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: + """Resolve one library's sources, include dirs, and flags (PIO semantics).""" + build = _manifest_build(name, data) + _reject_unsupported_link_fields(name, data) + src_dir = _resolve_src_dir(name, read_path, build) + src_filter = ensure_list(build.get("srcFilter", DEFAULT_BUILD_SRC_FILTER)) + if not all(isinstance(entry, str) for entry in src_filter): + raise EsphomeError(f"Library {name} has a malformed srcFilter") + lib = ArduinoLibrary(name=name, lib_archive=_resolve_lib_archive(name, data, build)) + # PlatformIO shell-lexes each build.flags entry + include_flags = _classify_build_flags( + name, read_path, lib, lex_build_flags(build.get("flags", []), f"library {name}") + ) + _resolve_include_dirs(name, read_path, lib, build, src_dir, include_flags) + _collect_lib_sources(name, read_path, lib, src_dir, src_filter) + return lib + + +def _bundled_library(framework_path: Path, name: str) -> ArduinoLibrary: + """A library bundled with the Arduino core, read from the framework tree. + + ``library.json`` wins over ``library.properties`` when both exist, as in + PlatformIO's LibBuilderFactory; only the JSON manifest can carry a + ``build`` section (srcDir, srcFilter, flags). + """ + lib_dir = framework_path / "libraries" / name + manifest_json = lib_dir / "library.json" + if manifest_json.is_file(): + try: + data = parse_library_json(manifest_json) + except ValueError as err: # JSONDecodeError + raise EsphomeError( + f"Bundled library {name} has a corrupt library.json ({err}); " + "the framework install may be incomplete (run 'esphome clean-all')" + ) from err + elif (manifest := lib_dir / "library.properties").is_file(): + data = parse_library_properties(manifest) + else: + # Debug, not warning: the legacy manifest-less layout is legal and + # the 3.1.2 core ships one such library (FSTools), so a warning + # would be unactionable noise on every build using it + _LOGGER.debug("Bundled library %s has no manifest; using defaults", name) + data = {} + if isinstance(data, dict): + # Bundled manifest deps are never walked; make the skip visible + if data.get("dependencies"): + _LOGGER.warning( + "Bundled library %s declares dependencies, which are not " + "resolved automatically; add them with add_library() if needed", + name, + ) + warn_properties_depends(name, data) + build = data.get("build") + if isinstance(build, dict) and build.get("extraScript"): + # Scripts only run on the converted path; building without + # the script's flags would miscompile + raise EsphomeError( + f"Bundled library {name} declares an extraScript, which is " + "not run for bundled libraries" + ) + lib = _library_info(name, lib_dir, data) + _assert_tree_has_code( + name, + lib_dir, + "the framework install may be incomplete (run 'esphome clean-all')", + ) + return lib + + +def _assert_tree_has_code(name: str, root: Path, hint: str) -> None: + """An empty or half-extracted tree can never link; fail by name (a + warning would scroll away and resurface as undefined symbols).""" + if not any( + Path(p).suffix in SRC_FILE_EXTENSIONS + or Path(p).suffix.lower() in LIBRARY_HEADER_SUFFIXES + for p in walk_files(root) + ): + raise EsphomeError(f"Library {name} has no sources or headers; {hint}") + + +def _external_short_name(name: str) -> str: + """The short library name of a requested spec. + + "owner/Name" and plain names take the last path segment; "Name=" + takes the declared name. Git tails (".git", "#ref") are stripped like + the walk's URL normalization; the comparand is a manifest dependency + name, never a spec. + """ + head, sep, tail = name.partition("=") + if sep and "://" in tail: + return head + short = name.rsplit("/", maxsplit=1)[-1] + return short.partition("#")[0].removesuffix(".git") + + +def _check_unfulfilled_provides( + provided_requests: set[str], satisfied: set[str], still_requested: set[str] +) -> None: + """Fail by name when a walk-skipped dependency was never added. + + An unfulfilled provides() promise only surfaces as undefined symbols + at link. The walk records across re-resolutions, so a name no final + manifest still requests is stale state, never a failure. + """ + if missing := sorted((provided_requests & still_requested) - satisfied): + raise EsphomeError( + "provides() skipped these dependencies but nothing added them: " + f"{', '.join(missing)}; the build is missing libraries" + ) + + +def resolve_libraries( + framework_path: Path, *, pio_platform: str, board_mcu: str, cache_key: str +) -> list[ArduinoLibrary]: + """Resolve every ``cg.add_library()`` entry into an :class:`ArduinoLibrary`. + + ``pio_platform``/``board_mcu`` filter manifests the way PlatformIO would + for that core (e.g. ``espressif8266``/``esp8266``); ``cache_key`` keys the + shared converter's download cache. + + The returned list is not topologically sorted, so the caller must link + the archives inside one ``--start-group``/``--end-group`` pair (the + bundled-first grouping is incidental). + """ + bundled: list[ArduinoLibrary] = [] + external: list[Library] = [] + # PlatformIO's lib_ignore covers framework-bundled libraries too; the + # shared converter only filters the registry/git ones. + lib_ignore = lib_ignore_set() + # Exact directory names keep membership case-sensitive everywhere + # (an is_dir() probe would match "wire" on macOS/Windows and build + # the bundled Wire twice) + libraries_dir = framework_path / "libraries" + if not libraries_dir.is_dir(): + # A registry fallback would fail later with a misleading + # package-not-found error per bundled name + raise EsphomeError( + f"{libraries_dir} is missing; the framework install may be " + "incomplete (run 'esphome clean-all')" + ) + bundled_dir_names = frozenset(p.name for p in libraries_dir.iterdir() if p.is_dir()) + + def _provided(name: object) -> bool: + return _is_safe_library_name(name) and name in bundled_dir_names + + for library in CORE.platformio_libraries.values(): + if is_lib_ignored(library.name, lib_ignore): + continue + # Bundled only for a bare name with a matching framework dir; pinned + # or unmatched names resolve from the registry, as under PlatformIO. + if not library.repository and not library.version and _provided(library.name): + # Bundled manifest deps are not walked; _bundled_library warns + bundled.append(_bundled_library(framework_path, library.name)) + else: + external.append(library) + + converted: list[ArduinoLibrary] = [] + bundled_names = {lib.name for lib in bundled} + converted_manifest_names: set[str] = set() + # Bundled candidates skipped on purpose (platform filter); the + # provides() reconciliation must count them as satisfied + knowingly_skipped: set[str] = set() + # Dependency names of the manifests actually emitted; a walk recording + # for a since-re-resolved manifest must not fail the reconciliation + final_dep_names: set[str] = set() + # Ordered set of bundled dependency names to add once conversion is done + pending_bundled: dict[str, None] = {} + # Deps matching a separately-requested external are already in the build + # (a duplicate archive means duplicate-symbol link errors) + external_short_names = { + _external_short_name(lib.name) for lib in external if lib.name + } + + def _add_bundled_dependencies(component: ConvertedLibrary) -> None: + # A version-less bare name ("Hash") is a core-bundled library the + # shared converter cannot resolve from the registry + for dep in normalize_dependencies( + component.data.get("dependencies"), component.name + ): + # normalize_dependencies guarantees a non-empty str name + name = dep["name"] + final_dep_names.add(name) + if "/" in name: + owner, _, pkg = name.partition("/") + if _is_safe_library_name(owner) and _is_safe_library_name(pkg): + # Owner-qualified; the converter resolves it from the registry + continue + if not _is_safe_library_name(name): + # The name becomes a path component; never join a traversal + _LOGGER.warning( + "Ignoring malformed dependency entry %r of library %s", + dep, + component.name, + ) + continue + if name in external_short_names: + if _provided(name): + # A bundled copy is suppressed; a coincidental name + # collision would surface as link errors + _LOGGER.warning( + "Dependency %s of %s is assumed satisfied by a " + "requested external library; the bundled copy is " + "not added", + name, + component.name, + ) + else: + _LOGGER.debug( + "Dependency %s of %s assumed satisfied by a requested " + "external library", + name, + component.name, + ) + continue + if name in bundled_names or is_lib_ignored(name, lib_ignore): + continue + if _url_or_none(dep.get("version")) is not None: + # A URL names one specific source; never add the bundled copy + continue + if dep.get("owner") or not _provided(name): + # Only owner-less framework-tree names take the bundled + # copy (PIO's process_dependencies); the walk reports drops + continue + try: + # framework=None: the walk already warned for non-platform + # causes; debug keeps one fault from warning twice (pinned + # by test_nonplatform_rejection_warns_once_through_real_converter) + check_library_data(dep, pio_platform, None) + except IncompatiblePlatform as err: + # A knowing skip (platform filter), not a broken promise + knowingly_skipped.add(name) + _LOGGER.debug("Skip bundled candidate %s: %s", name, err) + continue + except InvalidLibrary as err: + # Malformed manifest data never counts as satisfied; the + # walk owns the warning (see the warns-once test above) + _LOGGER.debug("Skip malformed bundled candidate %s: %s", name, err) + continue + # Deferred: a later manifest name may satisfy this + pending_bundled.setdefault(name) + + def _emit(component: ConvertedLibrary) -> None: + apply_extra_script( + component, board_mcu=lambda: board_mcu, pio_platform=pio_platform + ) + _assert_tree_has_code( + component.get_require_name(), + component.source_dir, + "the download may be incomplete (run 'esphome clean-all')", + ) + if isinstance(manifest_name := component.data.get("name"), str): + converted_manifest_names.add(manifest_name) + lib = _library_info( + component.get_require_name(), component.source_dir, component.data + ) + # Extra-script LINKFLAGS travel outside build.flags; dropping + # them would link wrong with no stated cause + lib.link_flags.extend( + component.data.get(ESPHOME_DATA_KEY, {}).get( + ESPHOME_DATA_LINK_FLAGS_KEY, [] + ) + ) + converted.append(lib) + _add_bundled_dependencies(component) + + backend = LibraryBackend( + platform=pio_platform, + framework="arduino", + emit=_emit, + cache_key=cache_key, + # The walk must not resolve bundled names from the registry; + # _add_bundled_dependencies adds them after emit + provides=_provided, + ) + if external: + convert_libraries(external, backend) + for name in pending_bundled: + if name in converted_manifest_names: + # The converted library is this one; the bundled copy would + # double the archive. Warn like the external_short_names twin. + _LOGGER.warning( + "Dependency %s is assumed satisfied by a converted library's " + "manifest name; the bundled copy is not added", + name, + ) + continue + bundled_names.add(name) + bundled.append(_bundled_library(framework_path, name)) + + _check_unfulfilled_provides( + backend.provided_requests, + bundled_names + | converted_manifest_names + | external_short_names + | knowingly_skipped, + final_dep_names, + ) + + return bundled + converted diff --git a/esphome/build_gen/build_tool.py b/esphome/build_gen/build_tool.py new file mode 100644 index 0000000000..00aa1ec69d --- /dev/null +++ b/esphome/build_gen/build_tool.py @@ -0,0 +1,108 @@ +"""Tiny cross-platform build steps invoked from the generated ninja file. + +Plain script (not ``python -m``): it runs from ninja with whatever Python +started esphome and must not depend on the package being importable. + +Subcommands: + ar remove stale archive, then ``ar rcs`` + copy copy a file + +The ar rspfile carries one object path per line (the generating rule must +use ``$in_newline``, never ``$in``). +""" + +from pathlib import Path +import shutil +import subprocess +import sys + + +def _read_rspfile(rspfile: str) -> list[str]: + r"""The object paths listed in ``rspfile``, unquoted. + + GNU ar treats backslashes in response files as escapes (corrupts + Windows paths), so the caller expands the list into argv; strip the + simple surrounding quote ninja adds to special paths, then undo + ninja's POSIX escape for an embedded quote ('a'\\''b.o' -> a'b.o). + """ + return [ + line[1:-1].replace("'\\''", "'") + if len(line) >= 2 and line[0] == line[-1] and line[0] in "'\"" + else line + for line in Path(rspfile).read_text(encoding="utf-8").splitlines() + if line + ] + + +def _run_ar(ar: str, archive: str, rspfile: str) -> int: + # Remove first: ``ar rcs`` replaces members but never drops ones whose + # source was removed from the build, which would leak stale objects. + Path(archive).unlink(missing_ok=True) + objects = _read_rspfile(rspfile) + if not objects: + # An empty archive would "succeed" here and fail far away at link + print(f"ar: no objects listed in {rspfile} for {archive}", file=sys.stderr) + return 1 + # Batch by argv length: expanding the rspfile gives back the Windows + # 32767-char command-line limit it existed to avoid. "rcs" creates, + # "qs" appends; the s keeps the symbol index explicit on every ar. + op = "rcs" + ok = False + try: + while objects: + batch = [objects.pop(0)] + batch_len = len(batch[0]) + while objects and batch_len + len(objects[0]) < 25000: + batch_len += len(objects[0]) + 1 + batch.append(objects.pop(0)) + rc = subprocess.run( + [ar, op, archive, *batch], check=False, close_fds=False + ).returncode + if rc != 0: + return rc + op = "qs" + ok = True + return 0 + finally: + if not ok: + # Any failure (bad exit, missing ar binary, interrupt) must not + # leave a truncated archive behind + Path(archive).unlink(missing_ok=True) + + +def _run_copy(src: str, dst: str) -> int: + try: + shutil.copyfile(src, dst) + except OSError as err: + # Never leave a partially written output (e.g. a firmware image); + # SameFileError means dst IS src, where unlinking destroys the input + if not isinstance(err, shutil.SameFileError): + Path(dst).unlink(missing_ok=True) + print(f"copy: {src} -> {dst} failed: {err}", file=sys.stderr) + return 1 + return 0 + + +# mode -> (handler, expected operand count); surplus argv means a +# mis-specified ninja rule and must error, not silently drop operands +_MODES = {"ar": (_run_ar, 3), "copy": (_run_copy, 2)} + + +def main() -> int: + mode = sys.argv[1] if len(sys.argv) > 1 else "" + if entry := _MODES.get(mode): + handler, argc = entry + args = sys.argv[2:] + if len(args) != argc: + print( + f"build_tool {mode}: expected {argc} arguments, got {len(args)}", + file=sys.stderr, + ) + return 1 + return handler(*args) + print(f"unknown build_tool mode: {mode}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": # pragma: no cover + sys.exit(main()) diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 306f07854e..0402311a9a 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -74,6 +74,11 @@ SOURCE_KIND_FOR_SUFFIX: dict[str, str] = { ".ASM": "asm", } SRC_FILE_EXTENSIONS = list(SOURCE_KIND_FOR_SUFFIX) +# Suffixes that count as headers when probing whether a library has any +# usable files at all (compare against Path.suffix.lower()) +LIBRARY_HEADER_SUFFIXES = frozenset( + {".h", ".hpp", ".hh", ".hxx", ".inc", ".ipp", ".tcc"} +) DOMAIN = "pio_components" @@ -329,6 +334,11 @@ class LibraryBackend: framework: str emit: Callable[["ConvertedLibrary"], None] cache_key: str + # Owner-less names this returns True for are skipped by the walk; + # the backend supplies them itself (e.g. core-bundled libraries) and + # reconciles provided_requests after resolving + provides: Callable[[str], bool] | None = None + provided_requests: set[str] = field(default_factory=set) def ensure_list[T](obj: T | list[T]) -> list[T]: @@ -469,7 +479,7 @@ def _valid_manifest_shape(data: Any) -> bool: ) -def check_library_data(data: dict, platform: str | None, framework: str): +def check_library_data(data: dict, platform: str | None, framework: str | None): """ Check whether a library manifest is compatible with the target toolchain. @@ -486,7 +496,8 @@ def check_library_data(data: dict, platform: str | None, framework: str): for targets (e.g. Zephyr) where PIO manifests rarely declare the platform yet portable libraries still build. framework: The active framework name (e.g. ``espidf``, ``arduino``, - ``zephyr``) the manifest is expected to declare. + ``zephyr``) the manifest is expected to declare. ``None`` skips + the framework check (and its warning), mirroring ``platform``. Raises: InvalidLibrary: If the library does not support the target platform. @@ -517,7 +528,7 @@ def check_library_data(data: dict, platform: str | None, framework: str): # under the target framework, and there's no way to opt out of the check at # this layer. Warn instead of failing so the user isn't forced to fork the # library to fix the manifest. - valid_framework = "*" in frameworks or framework in frameworks + valid_framework = framework is None or "*" in frameworks or framework in frameworks if not valid_framework: _LOGGER.warning( @@ -914,6 +925,56 @@ def is_lib_ignored(name: str | None, lib_ignore: set[str]) -> bool: ) +def _reconcile_versionless_skips( + skipped_versionless: list[tuple[Any, Any, str]], + components: dict[str, ConvertedLibrary], + backend: LibraryBackend, +) -> None: + """Warn for version-less deps nothing satisfied, and record the + backend-provided ones in ``backend.provided_requests`` for its + post-emit reconciliation; a silent drop surfaces as link errors far + from the cause.""" + resolved_manifest_names = {c.data.get("name") for c in components.values()} + # A treeless backend can never supply a bundled name; noise for it + log = _LOGGER.warning if backend.provides is not None else _LOGGER.debug + warned: set[str] = set() + for dep_name, dep_owner, requester in skipped_versionless: + if not isinstance(dep_name, str) or not dep_name or dep_name in warned: + continue + if dep_name in components: + # A version-less dep's request key is the name itself + continue + if ( + not dep_owner + and backend.provides is not None + and backend.provides(dep_name) + ): + # provides() only satisfies owner-less names (same guard as + # the walk's skip); record for the post-emit reconciliation. + # Checked before the manifest-name evidence so the overlap + # case warns once, in the backend's own suppression loop + backend.provided_requests.add(dep_name) + continue + if dep_name in resolved_manifest_names: + # Name-only evidence: a coincidental collision must stay + # visible where the user could pin it + warned.add(dep_name) + log( + "Version-less dependency %s of %s assumed satisfied by a " + "resolved library's manifest name only", + dep_name, + requester, + ) + continue + warned.add(dep_name) + log( + "Dependency %s of %s has no version to resolve and nothing " + "provides it; skipping", + dep_name, + requester, + ) + + def _fetch_source( component: ConvertedLibrary, salt: str, @@ -1083,6 +1144,8 @@ def convert_libraries( components: dict[str, ConvertedLibrary] = {} resolved_requirements: dict[str, frozenset[str]] = {} top_level_keys = set(top_level) + # (name, owner, requester) reconciled against the final resolution set + skipped_versionless: list[tuple[Any, Any, str]] = [] worklist = deque(dict.fromkeys(top_level)) while worklist: # Drain the frontier sequentially (spec resolution mutates shared @@ -1187,13 +1250,23 @@ def convert_libraries( component.data.get("dependencies"), component.name ): if "version" not in dependency: - # Cannot resolve from the registry; common for bundled - # names (Wire, SPI) -- unactionable noise above debug + # Cannot resolve from the registry; the post-emit + # reconciliation owns the drop warning + dep_name = dependency.get("name") _LOGGER.debug( "Skip version-less dependency %r of %s", - dependency.get("name"), + dep_name, component.name, ) + if not is_lib_ignored( + dep_name, lib_ignore + ) and dependency_is_usable( + dependency, backend.platform, backend.framework, component.name + ): + # Filtered or ignored deps are deliberately absent + skipped_versionless.append( + (dep_name, dependency.get("owner"), component.name) + ) continue if not dependency_is_usable( dependency, backend.platform, backend.framework, component.name @@ -1205,11 +1278,31 @@ def convert_libraries( if is_lib_ignored(dep_name, lib_ignore): _LOGGER.debug("Skip ignored dependency %s", dep_name) continue - # The version field may actually be a URL (git/archive dependency). + # The version may be a URL (git/archive), which names one + # specific source; never substitute a bundled library for it dep_version = dependency["version"] dep_url = _url_or_none(dep_version) if dep_url is not None: dep_version = None + elif ( + backend.provides is not None + and not dependency.get("owner") + and backend.provides(dep_name) + ): + # The backend adds it from its own tree; resolving here + # would fetch a same-named registry package + if dep_version and dep_version != "*": + # The pin is discarded; make the substitution visible + _LOGGER.warning( + "Dependency %s pins version %s; using the library " + "bundled with the framework instead", + dep_name, + dep_version, + ) + else: + _LOGGER.debug("Skip backend-provided dependency %s", dep_name) + backend.provided_requests.add(dep_name) + continue dep_key = add_spec(dep_name, dep_version, dep_url) node.edges.add(dep_key) worklist.append(dep_key) @@ -1263,4 +1356,6 @@ def convert_libraries( for component in components.values(): backend.emit(component) + _reconcile_versionless_skips(skipped_versionless, components, backend) + return [components[key] for key in top_level if key in components] diff --git a/tests/unit_tests/build_gen/test_build_tool.py b/tests/unit_tests/build_gen/test_build_tool.py new file mode 100644 index 0000000000..b029c647ab --- /dev/null +++ b/tests/unit_tests/build_gen/test_build_tool.py @@ -0,0 +1,246 @@ +"""Tests for the ninja build-tool helper script.""" + +from __future__ import annotations + +from pathlib import Path +import subprocess +import sys +from unittest.mock import MagicMock, patch + +import pytest + +from esphome.build_gen import build_tool + + +def test_ar_removes_stale_archive(tmp_path: Path) -> None: + archive = tmp_path / "lib.a" + archive.write_text("stale") + rsp = tmp_path / "lib.a.rsp" + rsp.write_text("a.o\n") + with ( + patch.object( + build_tool.sys, + "argv", + ["build_tool", "ar", "ar-bin", str(archive), str(rsp)], + ), + patch.object( + build_tool.subprocess, "run", return_value=MagicMock(returncode=0) + ) as mock_run, + ): + assert build_tool.main() == 0 + assert not archive.exists() + # The rspfile is expanded by the shim (GNU ar would escape backslashes) + assert mock_run.call_args[0][0] == ["ar-bin", "rcs", str(archive), "a.o"] + + +def test_copy(tmp_path: Path) -> None: + src = tmp_path / "firmware.bin" + src.write_text("data") + dst = tmp_path / "firmware.factory.bin" + with patch.object( + build_tool.sys, "argv", ["build_tool", "copy", str(src), str(dst)] + ): + assert build_tool.main() == 0 + assert dst.read_text() == "data" + + +def test_unknown_mode(capsys: pytest.CaptureFixture[str]) -> None: + with patch.object(build_tool.sys, "argv", ["build_tool", "bogus"]): + assert build_tool.main() == 1 + assert "unknown build_tool mode" in capsys.readouterr().err + + +def test_runs_as_script(tmp_path: Path) -> None: + """The ninja rules invoke the file as a plain script.""" + + src = tmp_path / "a.bin" + src.write_text("x") + dst = tmp_path / "b.bin" + result = subprocess.run( + [sys.executable, build_tool.__file__, "copy", str(src), str(dst)], + check=False, + ) + assert result.returncode == 0 + assert dst.read_text() == "x" + + +def test_ar_expands_rspfile_without_escaping(tmp_path) -> None: + """Backslash paths survive: the shim expands the rspfile itself instead + of letting GNU ar treat backslashes as escapes.""" + rsp = tmp_path / "objs.rsp" + rsp.write_text("obj/a.o\nsub\\b.o\n") + with ( + patch.object( + build_tool.sys, + "argv", + ["build_tool", "ar", "ar-bin", str(tmp_path / "lib.a"), str(rsp)], + ), + patch.object( + build_tool.subprocess, "run", return_value=MagicMock(returncode=0) + ) as mock_run, + ): + assert build_tool.main() == 0 + assert mock_run.call_args[0][0] == [ + "ar-bin", + "rcs", + str(tmp_path / "lib.a"), + "obj/a.o", + "sub\\b.o", + ] + + +def test_ar_unquotes_ninja_escaped_paths(tmp_path: Path) -> None: + """The shim strips a simple surrounding quote, since ninja shell- + quotes special rsp paths, so ar sees the real filename.""" + rsp = tmp_path / "t.rsp" + rsp.write_text("'obj/a b.o'\nobj/c.o\n") + with ( + patch.object( + build_tool.sys, "argv", ["bt", "ar", "/usr/bin/ar", "lib.a", str(rsp)] + ), + patch.object(build_tool.subprocess, "run") as mock_run, + ): + mock_run.return_value.returncode = 0 + rc = build_tool.main() + assert rc == 0 + assert mock_run.call_args.args[0] == [ + "/usr/bin/ar", + "rcs", + "lib.a", + "obj/a b.o", + "obj/c.o", + ] + + +def test_ar_empty_object_list_fails( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A lost object list is an error here, not undefined symbols at link.""" + rsp = tmp_path / "t.rsp" + rsp.write_text("\n\n") + with patch.object( + build_tool.sys, "argv", ["bt", "ar", "/usr/bin/ar", "lib.a", str(rsp)] + ): + rc = build_tool.main() + assert rc == 1 + assert "no objects listed" in capsys.readouterr().err + + +def test_ar_batches_long_object_lists(tmp_path: Path) -> None: + """The expanded argv must stay under the Windows 32767-char limit: a + long object list creates with rcs, then appends with qs.""" + archive = tmp_path / "lib.a" + rsp = tmp_path / "lib.a.rsp" + objects = [f"dir/{'x' * 120}_{i}.o" for i in range(400)] + rsp.write_text("\n".join(objects) + "\n") + with ( + patch.object( + build_tool.sys, + "argv", + ["build_tool", "ar", "ar-bin", str(archive), str(rsp)], + ), + patch.object( + build_tool.subprocess, "run", return_value=MagicMock(returncode=0) + ) as mock_run, + ): + assert build_tool.main() == 0 + calls = [c[0][0] for c in mock_run.call_args_list] + assert len(calls) > 1 + assert calls[0][1] == "rcs" + assert all(c[1] == "qs" for c in calls[1:]) + assert [o for c in calls for o in c[3:]] == objects + assert all(sum(len(a) + 1 for a in c) < 32000 for c in calls) + + +def test_ar_batch_failure_stops(tmp_path: Path) -> None: + """A failing batch propagates its exit code without running the rest.""" + archive = tmp_path / "lib.a" + rsp = tmp_path / "lib.a.rsp" + rsp.write_text("\n".join(f"{'y' * 200}_{i}.o" for i in range(300)) + "\n") + with ( + patch.object( + build_tool.sys, + "argv", + ["build_tool", "ar", "ar-bin", str(archive), str(rsp)], + ), + patch.object( + build_tool.subprocess, + "run", + side_effect=lambda cmd, **kw: ( + archive.write_text("partial"), + MagicMock(returncode=3), + )[1], + ) as mock_run, + ): + assert build_tool.main() == 3 + assert mock_run.call_count == 1 + # The failed batch must not leave a truncated archive behind + assert not archive.exists() + + +def test_ar_exception_leaves_no_partial_archive(tmp_path: Path) -> None: + """A missing ar binary mid-loop must not leave a truncated archive from + earlier successful batches.""" + archive = tmp_path / "lib.a" + rsp = tmp_path / "lib.a.rsp" + rsp.write_text("a.o\n") + with ( + patch.object( + build_tool.sys, + "argv", + ["build_tool", "ar", "ar-bin", str(archive), str(rsp)], + ), + patch.object( + build_tool.subprocess, + "run", + side_effect=lambda cmd, **kw: ( + archive.write_text("partial"), + (_ for _ in ()).throw(FileNotFoundError("no ar")), + ), + ), + pytest.raises(FileNotFoundError), + ): + build_tool.main() + assert not archive.exists() + + +def test_surplus_arguments_error(capsys: pytest.CaptureFixture[str]) -> None: + """A mis-specified ninja rule passing extra operands errors instead of + silently dropping them.""" + with patch.object( + build_tool.sys, "argv", ["build_tool", "copy", "a", "b", "extra"] + ): + assert build_tool.main() == 1 + assert "expected 2 arguments, got 3" in capsys.readouterr().err + + +def test_copy_same_file_keeps_the_input( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A same-file copy (dst IS src) must not unlink the input, and fails + with a message and exit code like the other shim paths.""" + src = tmp_path / "firmware.bin" + src.write_bytes(b"image") + with patch.object( + build_tool.sys, "argv", ["build_tool", "copy", str(src), str(src)] + ): + assert build_tool.main() == 1 + assert src.read_bytes() == b"image" + assert "failed" in capsys.readouterr().err + + +def test_copy_failure_leaves_no_partial_output(tmp_path: Path) -> None: + """A failed copy unlinks the destination; a partial firmware image must + never be left on disk.""" + dst = tmp_path / "firmware.factory.bin" + dst.write_text("stale") + with ( + patch.object(build_tool.shutil, "copyfile", side_effect=OSError("disk full")), + patch.object( + build_tool.sys, + "argv", + ["build_tool", "copy", str(tmp_path / "src.bin"), str(dst)], + ), + ): + assert build_tool.main() == 1 + assert not dst.exists() diff --git a/tests/unit_tests/test_arduino_library.py b/tests/unit_tests/test_arduino_library.py new file mode 100644 index 0000000000..87de28cf32 --- /dev/null +++ b/tests/unit_tests/test_arduino_library.py @@ -0,0 +1,1162 @@ +"""Tests for esphome.arduino.library (Arduino-core library resolution).""" + +from __future__ import annotations + +from contextlib import contextmanager +import json +import logging +from pathlib import Path +from unittest.mock import patch + +import pytest + +from esphome.arduino import library as component +from esphome.const import KEY_CORE, KEY_TARGET_PLATFORM, PLATFORM_ESP8266 +from esphome.core import CORE, EsphomeError, Library +import esphome.platformio.library as pio_library +from esphome.platformio.library import ( + ConvertedLibrary, + IncompatiblePlatform, + InvalidLibrary, + LibraryBackend, +) + + +@pytest.fixture(autouse=True) +def _reset_libraries() -> None: + # conftest's reset_core fixture clears platformio_libraries after each test + CORE.data[KEY_CORE] = {KEY_TARGET_PLATFORM: PLATFORM_ESP8266} + + +def _add_library(name: str, version: str | None, repository: str | None = None) -> None: + CORE.add_library(Library(name=name, version=version, repository=repository)) + + +def _make_framework(tmp_path: Path) -> Path: + framework = tmp_path / "framework" + lib = framework / "libraries" / "ESP8266WiFi" / "src" + lib.mkdir(parents=True) + (lib / "ESP8266WiFi.cpp").write_text("") + (lib / "ESP8266WiFi.h").write_text("") + (lib.parent / "library.properties").write_text("name=ESP8266WiFi\nversion=1.0\n") + root_lib = framework / "libraries" / "Wire" + root_lib.mkdir(parents=True) + (root_lib / "Wire.cpp").write_text("") + (root_lib / "examples").mkdir() + (root_lib / "examples" / "scan.ino").write_text("") + return framework + + +@contextmanager +def _emitting_converter(*converted): + """Patch convert_libraries to emit the given components via the backend.""" + + def fake_convert(libraries: list, backend: LibraryBackend) -> list: + assert backend.platform == "espressif8266" + assert backend.framework == "arduino" + assert backend.cache_key == "arduino8266" + for c in converted: + backend.emit(c) + return list(converted) + + with ( + patch.object(component, "convert_libraries", side_effect=fake_convert), + patch.object(component, "apply_extra_script") as mock_extra, + ): + yield mock_extra + + +def _converted(name: str, source_dir: Path, data: dict) -> ConvertedLibrary: + converted = ConvertedLibrary(name, "1.0.0", source=None) + converted.path = source_dir + converted.data = data + return converted + + +def _resolve(framework: Path) -> list[component.ArduinoLibrary]: + return component.resolve_libraries( + framework, + pio_platform="espressif8266", + board_mcu="esp8266", + cache_key="arduino8266", + ) + + +def _webserver(tmp_path: Path, data: dict) -> ConvertedLibrary: + """Register ESPAsyncWebServer and return its converted stand-in.""" + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + lib_dir = tmp_path / "converted" / "webserver" + (lib_dir / "src").mkdir(parents=True) + (lib_dir / "src" / "server.cpp").write_text("") + return _converted("esp32async__ESPAsyncWebServer", lib_dir, data) + + +def _local_lib(tmp_path: Path, dependencies: dict | list) -> None: + """Register a local file:// library declaring the given dependencies.""" + local_lib = tmp_path / "locallib" + (local_lib / "src").mkdir(parents=True) + (local_lib / "src" / "local.cpp").write_text("") + (local_lib / "library.json").write_text( + json.dumps( + {"name": "LocalLib", "version": "1.0.0", "dependencies": dependencies} + ) + ) + # as_uri() forms a valid file:// URL on every platform (file:///C:/... + # on Windows; a bare f-string would embed backslashes) + _add_library(local_lib.as_uri(), None) + + +def _ws_tcp_pair(tmp_path: Path) -> tuple[ConvertedLibrary, ConvertedLibrary]: + """Build ESPAsyncWebServer (depending on ESPAsyncTCP) plus resolved TCP.""" + ws_dir = tmp_path / "converted" / "webserver" + (ws_dir / "src").mkdir(parents=True) + (ws_dir / "src" / "server.cpp").write_text("") + tcp_dir = tmp_path / "converted" / "tcp" + (tcp_dir / "src").mkdir(parents=True) + (tcp_dir / "src" / "tcp.cpp").write_text("") + ws = _converted( + "esp32async__ESPAsyncWebServer", + ws_dir, + {"build": {}, "dependencies": [{"name": "ESPAsyncTCP"}]}, + ) + tcp = _converted("esp32async__ESPAsyncTCP", tcp_dir, {"build": {}}) + return ws, tcp + + +def test_library_info_src_layout(tmp_path: Path) -> None: + framework = _make_framework(tmp_path) + lib = component._bundled_library(framework, "ESP8266WiFi") + assert lib.name == "ESP8266WiFi" + assert [p.name for p in lib.sources] == ["ESP8266WiFi.cpp"] + assert lib.include_dirs == [(framework / "libraries/ESP8266WiFi/src").resolve()] + + +def test_library_info_root_layout_excludes_examples(tmp_path: Path) -> None: + framework = _make_framework(tmp_path) + lib = component._bundled_library(framework, "Wire") + assert [p.name for p in lib.sources] == ["Wire.cpp"] + assert lib.include_dirs == [(framework / "libraries/Wire").resolve()] + + +def test_library_info_flags_parsing(tmp_path: Path) -> None: + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "a.cpp").write_text("") + (read_path / "inc").mkdir() + (read_path / "blobs").mkdir() + data = { + "build": { + "flags": [ + "-DFOO=1 -I inc", + "-lalgobsec", + "-fno-lto", + "-Wl,--wrap=malloc", + # Bare flags join their argument within one entry only, as + # ParseFlags lexes each entry independently + "-l m", + "-L blobs", + ], + } + } + lib = component._library_info("x", read_path, data) + assert lib.flags == ["-DFOO=1", "-fno-lto"] + assert lib.include_dirs == [ + (read_path / "src").resolve(), + (read_path / "inc").resolve(), + ] + assert lib.link_dirs == [(read_path / "blobs").resolve()] + assert lib.link_libs == ["algobsec", "m"] + assert lib.link_flags == ["-Wl,--wrap=malloc"] + + +def test_library_info_missing_link_dir_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + read_path = tmp_path / "lib" + read_path.mkdir() + data = {"build": {"flags": ["-Lmissing_blobs"]}} + lib = component._library_info("x", read_path, data) + assert "declares library dir missing_blobs which does not exist" in caplog.text + # Kept anyway: the linker ignores missing -L dirs + assert lib.link_dirs == [(read_path / "missing_blobs").resolve()] + + +def test_library_info_declared_filter_matches_nothing_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + data = {"build": {"srcFilter": ["+"]}} + lib = component._library_info("x", read_path, data) + assert not lib.sources + assert "no source files matched" in caplog.text + + +def test_empty_converted_tree_raises_at_emit(tmp_path: Path) -> None: + """A converted tree with no sources and no headers is a broken download; + fail by name like the bundled case.""" + framework = _make_framework(tmp_path) + _add_library("Some/Empty", "1.0.0") + lib_dir = tmp_path / "converted" / "empty" + (lib_dir / "src").mkdir(parents=True) + converted = _converted("some__Empty", lib_dir, {"build": {}}) + with ( + _emitting_converter(converted), + pytest.raises(EsphomeError, match="no sources or headers; the download"), + ): + _resolve(framework) + + +def test_library_info_no_src_dir(tmp_path: Path) -> None: + read_path = tmp_path / "empty" + read_path.mkdir() + lib = component._library_info("x", read_path, {}) + # With no manifest hints the source dir falls back to the library root + assert lib.sources == [] + assert lib.include_dirs == [read_path.resolve()] + + +def test_resolve_libraries_bundled(tmp_path: Path) -> None: + framework = _make_framework(tmp_path) + _add_library("ESP8266WiFi", None) + libs = _resolve(framework) + assert [lib.name for lib in libs] == ["ESP8266WiFi"] + + +@pytest.mark.parametrize("version", [None, "1.1.0"]) +def test_resolve_libraries_registry_name_is_external( + tmp_path: Path, version: str | None +) -> None: + """A name that is not bundled reaches the converter, bare or pinned.""" + framework = _make_framework(tmp_path) + _add_library("pngle", version) + with patch.object(component, "convert_libraries", return_value=[]) as mock_convert: + _resolve(framework) + (libraries, _backend), _ = mock_convert.call_args + assert [lib.name for lib in libraries] == ["pngle"] + + +def test_resolve_libraries_external_and_bundled_deps(tmp_path: Path) -> None: + framework = _make_framework(tmp_path) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + + lib_dir = tmp_path / "converted" / "webserver" + (lib_dir / "src").mkdir(parents=True) + (lib_dir / "src" / "server.cpp").write_text("") + converted = _converted( + "esp32async__ESPAsyncWebServer", + lib_dir, + { + "build": {}, + "dependencies": [ + # Version-less bundled dependency: resolved from the framework + {"name": "Wire", "platforms": "espressif8266"}, + # Wrong platform: skipped + {"name": "ESP8266WiFi", "platforms": "espressif32"}, + # Registry dependency with a version: handled by the converter + {"name": "ESPAsyncTCP", "owner": "ESP32Async", "version": "^2.0.0"}, + # Not bundled: skipped + {"name": "NotBundled"}, + ], + }, + ) + + with _emitting_converter(converted) as mock_extra: + libs = _resolve(framework) + + mock_extra.assert_called_once() + assert mock_extra.call_args.args == (converted,) + assert mock_extra.call_args.kwargs["pio_platform"] == "espressif8266" + # board_mcu is passed lazily, as the shared helper requires + assert mock_extra.call_args.kwargs["board_mcu"]() == "esp8266" + assert [lib.name for lib in libs] == [ + "Wire", + "esp32async__ESPAsyncWebServer", + ] + + +def test_resolve_libraries_bundled_dep_already_present(tmp_path: Path) -> None: + framework = _make_framework(tmp_path) + _add_library("Wire", None) + _add_library("Some/External", "1.0.0") + + lib_dir = tmp_path / "converted" / "external" + lib_dir.mkdir(parents=True) + (lib_dir / "main.cpp").write_text("") + converted = _converted( + "some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} + ) + + with _emitting_converter(converted): + libs = _resolve(framework) + + # Wire appears once (from the explicit registration), not twice + assert [lib.name for lib in libs] == ["Wire", "some__External"] + + +def test_library_info_trailing_bare_flag_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + lib = component._library_info("x", read_path, {"build": {"flags": ["-DA=1 -l"]}}) + assert lib.flags == ["-DA=1"] + assert lib.link_libs == [] + assert "Ignoring trailing '-l'" in caplog.text + + +def test_library_info_missing_explicit_include_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + lib = component._library_info("x", read_path, {"build": {"flags": ["-Inope"]}}) + assert lib.include_dirs == [(read_path / "src").resolve()] + assert "include dir nope which does not exist" in caplog.text + + +def test_library_info_missing_declared_src_dir_raises(tmp_path: Path) -> None: + """An explicitly declared srcDir that does not exist is a manifest error.""" + read_path = tmp_path / "lib" + read_path.mkdir() + with pytest.raises(EsphomeError, match="srcDir 'nosrc' which does not exist"): + component._library_info("x", read_path, {"build": {"srcDir": "nosrc"}}) + + +def test_library_info_missing_declared_include_dir_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + read_path = tmp_path / "lib" + read_path.mkdir() + component._library_info("x", read_path, {"build": {"includeDir": "noinc"}}) + assert "include dir noinc which does not exist" in caplog.text + + +def test_resolve_libraries_lib_ignore_covers_bundled(tmp_path: Path) -> None: + """lib_ignore applies to framework-bundled libraries, as under PlatformIO.""" + framework = _make_framework(tmp_path) + _add_library("ESP8266WiFi", None) + _add_library("Wire", None) + CORE.platformio_options = {"lib_ignore": ["Wire"]} + libs = _resolve(framework) + assert [lib.name for lib in libs] == ["ESP8266WiFi"] + + +def test_resolve_libraries_lib_ignore_covers_bundled_dependencies( + tmp_path: Path, +) -> None: + framework = _make_framework(tmp_path) + _add_library("Some/External", "1.0.0") + CORE.platformio_options = {"lib_ignore": ["Wire"]} + + lib_dir = tmp_path / "converted" / "external" + lib_dir.mkdir(parents=True) + (lib_dir / "main.cpp").write_text("") + converted = _converted( + "some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} + ) + + with _emitting_converter(converted): + libs = _resolve(framework) + + assert [lib.name for lib in libs] == ["some__External"] + + +def test_bundled_library_prefers_library_json(tmp_path: Path) -> None: + """A bundled library.json wins over library.properties (PIO semantics); + its build section is honored.""" + framework = _make_framework(tmp_path) + lib_dir = framework / "libraries" / "GDBStub" + (lib_dir / "custom").mkdir(parents=True) + (lib_dir / "custom" / "gdb.cpp").write_text("") + (lib_dir / "library.properties").write_text("name=GDBStub\n") + (lib_dir / "library.json").write_text( + '{"name": "GDBStub", "build": {"srcDir": "custom"}}' + ) + lib = component._bundled_library(framework, "GDBStub") + assert [s.name for s in lib.sources] == ["gdb.cpp"] + + +def test_library_info_lib_archive_flag(tmp_path: Path) -> None: + """Both libArchive (library.json) and dot_a_linkage (properties) reach + the generator's contract; default is archive.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + assert component._library_info("x", read_path, {}).lib_archive is True + assert ( + component._library_info( + "x", read_path, {"build": {"libArchive": False}} + ).lib_archive + is False + ) + assert ( + component._library_info("x", read_path, {"dot_a_linkage": "false"}).lib_archive + is False + ) + assert ( + component._library_info("x", read_path, {"dot_a_linkage": "true"}).lib_archive + is True + ) + + +def test_resolve_libraries_dep_warnings( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A nameless dependency entry warns in the shared normalizer; an + owner-without-version entry is left to the walk's reconciliation.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, + { + "build": {}, + "dependencies": [ + {"owner": "someone"}, + {"name": "Orphan", "owner": "someone"}, + ], + }, + ) + with _emitting_converter(converted): + _resolve(framework) + assert "Ignoring unrecognized dependency entry" in caplog.text + assert "Orphan" not in caplog.text + + +def test_bundled_dependency_nonplatform_rejection_is_silent_here( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The shared walk owns the rejection warning; the backend-side filter + stays at debug so one manifest fault never warns twice.""" + framework = _make_framework(tmp_path) + converted = _webserver(tmp_path, {"build": {}, "dependencies": [{"name": "Wire"}]}) + with ( + _emitting_converter(converted), + patch.object( + component, + "check_library_data", + side_effect=InvalidLibrary("manifest is corrupt"), + ), + ): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + assert "manifest is corrupt" not in caplog.text + + +def test_nonplatform_rejection_warns_once_through_real_converter( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """One manifest fault produces exactly one warning across the walk and + the backend-side bundled filter.""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, [{"name": "Wire"}]) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + real = pio_library.check_library_data + + def flaky(data, platform, framework_name): + if data.get("name") == "Wire": + raise InvalidLibrary("manifest is corrupt") + return real(data, platform, framework_name) + + monkeypatch.setattr(pio_library, "check_library_data", flaky) + monkeypatch.setattr(component, "check_library_data", flaky) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + _resolve(framework) + assert caplog.text.count("manifest is corrupt") == 1 + + +def test_url_pinned_bundled_name_not_doubled(tmp_path: Path) -> None: + """A URL-pinned dependency names one specific source; the bundled copy + of the same short name must never be added on top of the fork.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, + { + "build": {}, + "dependencies": [ + {"name": "Wire", "version": "https://github.com/x/wire-fork.git"} + ], + }, + ) + with _emitting_converter(converted): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + + +def test_versioned_bundled_candidate_fault_warns_once( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A versioned bundled-name dependency with a manifest fault warns once, + from the walk's usability filter; the backend-side re-check stays quiet.""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, [{"name": "Wire", "version": "*"}]) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + real = pio_library.check_library_data + + def flaky(data, platform, framework_name): + if data.get("name") == "Wire": + raise InvalidLibrary("manifest is corrupt") + return real(data, platform, framework_name) + + monkeypatch.setattr(pio_library, "check_library_data", flaky) + monkeypatch.setattr(component, "check_library_data", flaky) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + assert caplog.text.count("manifest is corrupt") == 1 + + +def test_short_name_collision_with_bundled_name_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Suppressing a genuinely bundled name on a short-name match warns; + an accidental collision would otherwise surface at link.""" + framework = _make_framework(tmp_path) + _add_library("Someone/Wire", "1.0.0") + converted = _converted( + "someone__Wire", + tmp_path / "conv", + {"build": {}, "dependencies": [{"name": "Wire"}]}, + ) + (tmp_path / "conv" / "src").mkdir(parents=True) + (tmp_path / "conv" / "src" / "a.cpp").write_text("") + with _emitting_converter(converted): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + assert "assumed satisfied by a requested external library" in caplog.text + assert any(r.levelname == "WARNING" for r in caplog.records) + + +def test_missing_libraries_dir_is_a_broken_install(tmp_path: Path) -> None: + """A framework tree without libraries/ must fail by name, not silently + reroute every bundled name to the registry.""" + framework = tmp_path / "framework" + framework.mkdir() + _add_library("Wire", None) + with pytest.raises(EsphomeError, match="framework install may be incomplete"): + _resolve(framework) + + +def test_provided_is_case_sensitive(tmp_path: Path) -> None: + """Membership uses the exact on-disk names, so a case-insensitive + filesystem cannot add the same bundled library twice.""" + framework = _make_framework(tmp_path) + converted = _webserver(tmp_path, {"build": {}, "dependencies": [{"name": "wire"}]}) + with _emitting_converter(converted): + libs = _resolve(framework) + assert "wire" not in [lib.name for lib in libs] + assert "Wire" not in [lib.name for lib in libs] + + +@pytest.mark.parametrize("declared", ["", None]) +def test_library_info_falsy_declared_src_dir_raises( + tmp_path: Path, declared: str | None +) -> None: + """A declared-but-falsy srcDir must not silently fall back to the probe.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + with pytest.raises(EsphomeError, match="does not exist"): + component._library_info("x", read_path, {"build": {"srcDir": declared}}) + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + (False, False), + ("false", False), + ("False", False), + ("true", True), + ], +) +def test_library_info_lib_archive_parse( + tmp_path: Path, + value: object, + expected: bool, +) -> None: + """bool("false") is True; the string forms must parse, not coerce.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + lib = component._library_info("x", read_path, {"build": {"libArchive": value}}) + assert lib.lib_archive is expected + + +def test_library_info_unsupported_link_fields_raise(tmp_path: Path) -> None: + """precompiled/ldflags properties are not supported; refuse by name.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + with pytest.raises(EsphomeError, match="declares precompiled"): + component._library_info("x", read_path, {"precompiled": "true", "build": {}}) + with pytest.raises(EsphomeError, match="declares ldflags"): + component._library_info("x", read_path, {"ldflags": "-lfoo", "build": {}}) + + +@pytest.mark.parametrize("value", ["false", "False", " false ", "", False, None]) +def test_library_info_precompiled_opt_out_accepted( + tmp_path: Path, value: object +) -> None: + """Manifest values are strings; precompiled=false is the spec's + explicit opt-out, not a declaration.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + data = {"build": {}} + if value is not None: + data["precompiled"] = value + component._library_info("x", read_path, data) + + +@pytest.mark.parametrize("value", ["full", True, "weird"]) +def test_library_info_precompiled_set_raises(tmp_path: Path, value: object) -> None: + """Both full (Arduino's other legal value) and unknown spellings fail safe.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + with pytest.raises(EsphomeError, match="declares precompiled"): + component._library_info("x", read_path, {"precompiled": value, "build": {}}) + + +def test_library_info_default_filter_matching_nothing_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The empty-match warning is not gated on a declared srcFilter/srcDir; + a default-filter src/ holding only inert files warns too.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "keywords.txt").write_text("") + lib = component._library_info("x", read_path, {"build": {}}) + assert not lib.sources + assert "no source files matched" in caplog.text + + +def test_library_info_unmapped_sources_warn( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Source-like files the case-sensitive suffix map rejects are named, + even when other sources compiled (a partial drop links with undefined + symbols far from the cause).""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "impl.CPP").write_text("") + (read_path / "src" / "sketch.ino").write_text("") + (read_path / "src" / "ok.cpp").write_text("") + lib = component._library_info("x", read_path, {"build": {}}) + assert [s.name for s in lib.sources] == ["ok.cpp"] + assert "not compiled: impl.CPP, sketch.ino" in caplog.text + + +def test_library_info_inert_only_filter_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A declared srcFilter matching only inert files (no sources, no + headers) warns like one matching nothing at all.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "keywords.txt").write_text("") + component._library_info("x", read_path, {"build": {"srcFilter": ["+<*>"]}}) + assert "no source files matched" in caplog.text + + +def test_library_info_declared_filter_matching_headers_stays_quiet( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A declared filter matching real headers is a header-only library.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "api.h").write_text("") + component._library_info("x", read_path, {"build": {"srcFilter": ["+<*>"]}}) + assert "no source files matched" not in caplog.text + + +def test_library_info_header_only_src_stays_quiet( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A header-only library (real headers in src/) is routine, not a + warning (the default +<*> filter matches the headers too).""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "ArduinoJson.h").write_text("") + (read_path / "keywords.txt").write_text("") + lib = component._library_info("x", read_path, {"build": {}}) + assert lib.sources == [] + assert "not compiled" not in caplog.text + assert "srcFilter" not in caplog.text + + +def test_library_info_lib_archive_malformed_raises(tmp_path: Path) -> None: + """A typo'd libArchive fails by name like the other build fields.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + with pytest.raises(EsphomeError, match="malformed libArchive value 'archive-me'"): + component._library_info("x", read_path, {"build": {"libArchive": "archive-me"}}) + + +def test_bundled_dependency_dict_shorthand_prefers_bundled(tmp_path: Path) -> None: + """The {"Wire": "*"} dict shorthand resolves to the bundled library.""" + framework = _make_framework(tmp_path) + converted = _webserver(tmp_path, {"build": {}, "dependencies": {"Wire": "*"}}) + with _emitting_converter(converted): + libs = _resolve(framework) + assert "Wire" in [lib.name for lib in libs] + + +def test_unfulfilled_provides_promise_raises(tmp_path: Path) -> None: + """A provides()-skipped dependency nothing added can only surface as + undefined symbols at link, so it fails here by name; satisfied ones + pass silently.""" + with pytest.raises(EsphomeError, match="Wire") as err: + component._check_unfulfilled_provides( + {"Wire", "Hash"}, {"Hash"}, {"Wire", "Hash"} + ) + assert str(err.value).count("Wire") == 1 + assert "Hash" not in str(err.value) + component._check_unfulfilled_provides({"Hash"}, {"Hash"}, {"Hash"}) + # A recording for a since-re-resolved manifest is stale walk state, + # never a failure: no final manifest still requests Wire + component._check_unfulfilled_provides({"Wire"}, set(), set()) + + +def test_extra_script_link_flags_reach_the_library(tmp_path: Path) -> None: + """LINKFLAGS captured by an extra script travel outside build.flags and + must reach the library's link flags, matching the ESP-IDF backend.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, + { + "build": {}, + component.ESPHOME_DATA_KEY: { + component.ESPHOME_DATA_LINK_FLAGS_KEY: ["-Wl,--wrap=foo"] + }, + }, + ) + with _emitting_converter(converted): + libs = _resolve(framework) + (webserver,) = (lib for lib in libs if "ESPAsyncWebServer" in lib.name) + assert "-Wl,--wrap=foo" in webserver.link_flags + + +def test_bundled_dependency_platform_rejection_is_debug( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The typed IncompatiblePlatform (the routine cross-platform skip) + stays at debug regardless of message wording.""" + framework = _make_framework(tmp_path) + converted = _webserver(tmp_path, {"build": {}, "dependencies": [{"name": "Wire"}]}) + with ( + _emitting_converter(converted), + patch.object( + component, + "check_library_data", + side_effect=IncompatiblePlatform("nothing about the p-word here"), + ), + ): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + assert "Skipping dependency Wire" not in caplog.text + + +@pytest.mark.parametrize("data", [{"build": "src"}, [], "nope"]) +def test_library_info_malformed_manifest_is_named(tmp_path: Path, data: object) -> None: + """A malformed manifest names the library, never an AttributeError.""" + read_path = tmp_path / "lib" + read_path.mkdir() + with pytest.raises(EsphomeError, match="Library x has a malformed manifest"): + component._library_info("x", read_path, data) + + +def test_bundled_library_with_declared_dependencies_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A bundled manifest that declares dependencies is visible, not + silently skipped (a no-op for the ESP8266 core, not for every core).""" + framework = _make_framework(tmp_path) + wire = framework / "libraries" / "Wire" + (wire / "library.json").write_text( + '{"name": "Wire", "dependencies": [{"name": "SPI"}]}' + ) + _add_library("Wire", None) + _resolve(framework) + assert "Bundled library Wire declares dependencies" in caplog.text + + +@pytest.mark.parametrize( + ("build", "match"), + [ + ({"includeDir": ["a", "b"]}, "malformed includeDir"), + ({"srcFilter": [123]}, "malformed srcFilter"), + ], +) +def test_library_info_malformed_build_fields_are_named( + tmp_path: Path, build: dict, match: str +) -> None: + """Malformed includeDir/srcFilter fail naming the library like srcDir.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + with pytest.raises(EsphomeError, match=match): + component._library_info("x", read_path, {"build": build}) + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("true", True), + ("False", False), + ], +) +def test_library_info_dot_a_linkage_parses_strictly( + tmp_path: Path, + value: str, + expected: bool, +) -> None: + """The dot_a_linkage property uses the same strict table as libArchive.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + lib = component._library_info("x", read_path, {"dot_a_linkage": value, "build": {}}) + assert lib.lib_archive is expected + + +def test_library_info_dot_a_linkage_malformed_raises(tmp_path: Path) -> None: + """A typo'd dot_a_linkage must not silently flip link semantics.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") + with pytest.raises(EsphomeError, match="malformed dot_a_linkage value 'yes'"): + component._library_info("x", read_path, {"dot_a_linkage": "yes", "build": {}}) + + +def test_bundled_library_properties_depends_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The library.properties depends= spelling reaches the visibility + warning too; the shared parser returns it raw.""" + framework = _make_framework(tmp_path) + wire = framework / "libraries" / "Wire" + (wire / "library.properties").write_text("name=Wire\nversion=1.0\ndepends=SPI\n") + _add_library("Wire", None) + caplog.set_level("INFO") + _resolve(framework) + assert "Library Wire declares dependencies via library.properties" in caplog.text + + +def test_bundled_library_extra_script_raises(tmp_path: Path) -> None: + """A bundled manifest relying on an extraScript would miscompile; + refuse by name.""" + framework = _make_framework(tmp_path) + wire = framework / "libraries" / "Wire" + (wire / "library.json").write_text( + '{"name": "Wire", "build": {"extraScript": "extra.py"}}' + ) + _add_library("Wire", None) + with pytest.raises(EsphomeError, match="Wire declares an extraScript"): + _resolve(framework) + + +def test_dependency_requested_top_level_is_not_a_drop( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A version-less manifest dependency the config separately requests is + already in the build; it is not probed as a bundled library.""" + framework = _make_framework(tmp_path) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + _add_library("ESP32Async/ESPAsyncTCP", "2.0.0") + ws, tcp = _ws_tcp_pair(tmp_path) + with _emitting_converter(ws, tcp): + libs = _resolve(framework) + # Exactly the two converted libraries; no bundled stand-in was added + assert [lib.name for lib in libs] == [ + "esp32async__ESPAsyncWebServer", + "esp32async__ESPAsyncTCP", + ] + assert "Skipping" not in caplog.text + + +def test_bundled_library_non_dict_manifest_skips_probes_and_raises( + tmp_path: Path, +) -> None: + """A bundled library.json that is a JSON array skips the dependency and + extraScript probes and fails in _library_info naming the library.""" + framework = _make_framework(tmp_path) + wire = framework / "libraries" / "Wire" + (wire / "library.json").write_text('["not", "a", "manifest"]') + with pytest.raises(EsphomeError, match="Library Wire has a malformed manifest"): + component._bundled_library(framework, "Wire") + + +def test_bundled_missing_manifest_is_debug_only( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The legacy manifest-less layout is legal (the core ships FSTools + without one), so the diagnostic must stay below warning level.""" + framework = _make_framework(tmp_path) + with caplog.at_level(logging.DEBUG): + component._bundled_library(framework, "Wire") + record = next(r for r in caplog.records if "has no manifest" in r.message) + assert record.levelno == logging.DEBUG + + +def test_bundled_corrupt_library_json_fails_by_name(tmp_path: Path) -> None: + """A truncated bundled library.json fails with the library name and the + clean-all hint, not a raw JSONDecodeError.""" + framework = _make_framework(tmp_path) + (framework / "libraries" / "Wire" / "library.json").write_text("{truncated") + with pytest.raises(EsphomeError, match="Wire has a corrupt library.json"): + component._bundled_library(framework, "Wire") + + +def test_dict_shorthand_dependency_skips_registry_through_real_converter( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """{"Wire": "*"} resolves to the bundled copy without touching the + registry (real converter).""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, {"Wire": "*"}) + # Pin the component cache to tmp_path (data_dir honors an ambient + # ESPHOME_DATA_DIR otherwise) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + libs = _resolve(framework) + names = [lib.name for lib in libs] + assert "Wire" in names + assert any("locallib" in n.lower() for n in names) + # The walk populated provided_requests for the skip; the backend added + # the bundled copy, so the reconciliation passed without raising + + +def test_versionless_provides_skip_is_reconciled_through_real_converter( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A truly version-less bare-name dependency the walk skips on the + backend's promise is recorded and fulfilled by the bundled copy.""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, ["Wire"]) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + libs = _resolve(framework) + assert "Wire" in [lib.name for lib in libs] + + +def test_platform_filtered_bundled_candidate_does_not_break_reconciliation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A bundled candidate the backend knowingly skips (platform filter) + counts as satisfied; the promise reconciliation must not raise.""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, [{"name": "Wire", "platforms": ["espressif32"]}]) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + + +@pytest.mark.parametrize( + ("bad_name", "message"), + [ + # A non-string name never leaves the shared normalizer + (1, "Ignoring unrecognized dependency entry"), + ("../escape", "Ignoring malformed dependency entry"), + ("..", "Ignoring malformed dependency entry"), + ], +) +def test_bundled_dependency_bad_name_is_malformed( + tmp_path: Path, bad_name: object, message: str, caplog: pytest.LogCaptureFixture +) -> None: + """A dependency name becomes a path component; a traversal or a + non-string is a malformed entry, never joined.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, {"build": {}, "dependencies": [{"name": bad_name}]} + ) + with _emitting_converter(converted): + _resolve(framework) + assert message in caplog.text + + +def test_owner_qualified_dependency_is_silent( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The owner-qualified dependency spelling (PIO's Owner/Pkg) resolves via + the converter; it must not draw the malformed-entry warning.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, + { + "build": {}, + "dependencies": [{"name": "ESP32Async/AsyncTCP", "version": "^3.0"}], + }, + ) + with _emitting_converter(converted): + _resolve(framework) + assert "malformed" not in caplog.text + + +def test_bundled_dependency_string_list_form(tmp_path: Path) -> None: + """The bare string-list dependency form (PIO-legal) resolves to the + bundled library instead of vanishing in normalization.""" + framework = _make_framework(tmp_path) + converted = _webserver(tmp_path, {"build": {}, "dependencies": ["Wire"]}) + with _emitting_converter(converted): + libs = _resolve(framework) + assert "Wire" in [lib.name for lib in libs] + + +def test_pinned_bundled_dependency_substitution_warns( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A non-* version pin on a backend-provided dependency is discarded + for the bundled copy; the substitution must be visible.""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, {"Wire": "^2.0.0"}) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + libs = _resolve(framework) + assert "Wire" in [lib.name for lib in libs] + assert "pins version ^2.0.0; using the library bundled" in caplog.text + + +def test_transitively_resolved_dependency_does_not_warn( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A dependency the walk already resolved does not warn.""" + framework = _make_framework(tmp_path) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + ws, tcp = _ws_tcp_pair(tmp_path) + with _emitting_converter(ws, tcp): + libs = _resolve(framework) + assert [lib.name for lib in libs] == [ + "esp32async__ESPAsyncWebServer", + "esp32async__ESPAsyncTCP", + ] + assert "Skipping" not in caplog.text + + +@pytest.mark.parametrize( + ("spec", "expected"), + [ + ("owner/Name", "Name"), + ("Name", "Name"), + ("Foo=file:///srv/Wire", "Foo"), + ("Foo=https://github.com/x/Wire", "Foo"), + # An "=" without a URL is a registry name, not the custom-name form + ("FOO=BAR", "FOO=BAR"), + ("https://github.com/x/Wire", "Wire"), + # Git tails are stripped like the walk's URL normalization + ("https://github.com/x/Wire.git", "Wire"), + ("git+https://github.com/x/Wire.git#v1", "Wire"), + ], +) +def test_external_short_name(spec: str, expected: str) -> None: + assert component._external_short_name(spec) == expected + + +def test_converted_manifest_name_suppresses_bundled_dependency( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A name a converted library's manifest provides is not also added + from the framework tree, even when the provider emits later; the + suppression warns like its external_short_names twin.""" + framework = _make_framework(tmp_path) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + # Requested under a different short name; only the manifest says "Wire" + _add_library("Someone/WireLib", "9.9.9") + ws_dir = tmp_path / "converted" / "webserver" + (ws_dir / "src").mkdir(parents=True) + (ws_dir / "src" / "stub.cpp").write_text("") + wire_dir = tmp_path / "converted" / "wire" + (wire_dir / "src").mkdir(parents=True) + (wire_dir / "src" / "wire.cpp").write_text("") + ws = _converted( + "esp32async__ESPAsyncWebServer", + ws_dir, + {"build": {}, "dependencies": [{"name": "Wire"}]}, + ) + registry_wire = _converted( + "someone__WireLib", wire_dir, {"name": "Wire", "build": {}} + ) + with _emitting_converter(ws, registry_wire): + libs = _resolve(framework) + # The bundled Wire is not added alongside the registry-resolved one + assert [lib.name for lib in libs] == [ + "esp32async__ESPAsyncWebServer", + "someone__WireLib", + ] + assert "Dependency Wire is assumed satisfied by a converted" in caplog.text + + +def test_bundled_library_root_headers_pass_the_probe(tmp_path: Path) -> None: + """Headers anywhere in the bundled tree (uncommon suffixes and case + included) prove the install is intact, even with an empty src dir.""" + framework = _make_framework(tmp_path) + lib_dir = framework / "libraries" / "HeaderOnly" + (lib_dir / "src").mkdir(parents=True) + (lib_dir / "impl.HXX").write_text("") + lib = component._bundled_library(framework, "HeaderOnly") + assert lib.sources == [] + + +def test_empty_bundled_library_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A bundled directory with no sources or headers is a broken install + that can never link; fail by name instead of warning into it.""" + framework = _make_framework(tmp_path) + (framework / "libraries" / "Empty").mkdir() + _add_library("Empty", None) + with pytest.raises(EsphomeError, match="Library Empty has no sources or headers"): + _resolve(framework) + + +def test_versionless_dependency_with_provider_stays_quiet( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """With a provides backend the version-less skip is routine (debug) and + the bundled copy is picked up after emit.""" + framework = _make_framework(tmp_path) + _local_lib(tmp_path, [{"name": "Wire"}]) + monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) + with patch.object( + pio_library, + "_resolve_registry_version", + side_effect=AssertionError("registry touched"), + ): + libs = _resolve(framework) + assert "Wire" in [lib.name for lib in libs] + assert "has no version to resolve" not in caplog.text diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 0c873dc3fe..0a16b118fc 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -10,7 +10,7 @@ from pathlib import Path import pytest -from esphome.core import EsphomeError, Library +from esphome.core import CORE, EsphomeError, Library import esphome.platformio.library as lib from esphome.platformio.library import ( SOURCE_KIND_FOR_SUFFIX, @@ -29,9 +29,13 @@ from esphome.platformio.library import ( ) -def _backend(emit=lambda component: None) -> LibraryBackend: +def _backend(emit=lambda component: None, provides=None) -> LibraryBackend: return LibraryBackend( - platform="espressif32", framework="espidf", emit=emit, cache_key="idf" + platform="espressif32", + framework="espidf", + emit=emit, + cache_key="idf", + provides=provides, ) @@ -952,3 +956,202 @@ def test_source_kind_map_shape() -> None: assert SOURCE_KIND_FOR_SUFFIX[".S"] == "aspp" assert SOURCE_KIND_FOR_SUFFIX[".c"] == "c" assert SOURCE_KIND_FOR_SUFFIX[".cpp"] == "cxx" + # SCons's case-sensitive C++ suffixes: PIO compiles .C as C++ + assert SOURCE_KIND_FOR_SUFFIX[".C"] == "cxx" + assert SOURCE_KIND_FOR_SUFFIX[".C++"] == "cxx" + + +def test_versionless_platform_filtered_dependency_stays_quiet( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A version-less dependency the platform filter excludes is + deliberately absent, not a drop to warn about.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "Hash", "platforms": "espressif8266"}], + } + }, + ) + convert_libraries([Library("esphome/A", None, None)], _backend()) + assert "has no version to resolve" not in caplog.text + + +def test_versionless_ignored_dependency_stays_quiet( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A lib_ignore'd version-less dependency is deliberately excluded, not + a drop; no reconciliation warning.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": {"name": "A", "dependencies": [{"name": "Hash"}]}}, + ) + CORE.platformio_options = {"lib_ignore": ["Hash"]} + convert_libraries([Library("esphome/A", None, None)], _backend()) + assert "has no version to resolve" not in caplog.text + + +def test_versionless_dependency_without_provider_warns( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A backend whose tree could supply the name warns on the drop; one + without provides() can never act on it, so it stays at debug.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + # The duplicate entry warns once (reconciliation dedup) + "dependencies": [{"name": "Hash"}, {"name": "Hash"}], + } + }, + ) + convert_libraries( + [Library("esphome/A", None, None)], _backend(provides=lambda name: False) + ) + assert ( + caplog.text.count( + "Hash of esphome/A has no version to resolve and nothing provides it" + ) + == 1 + ) + caplog.clear() + with caplog.at_level(logging.DEBUG): + convert_libraries([Library("esphome/A", None, None)], _backend()) + records = [ + r + for r in caplog.records + if "has no version to resolve and nothing provides it" in r.message + ] + assert records and all(r.levelno == logging.DEBUG for r in records) + + +def test_url_version_dependency_is_not_substituted_by_provides( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A URL-valued version names one specific source; the backend-provided + skip must not replace it with the bundled copy.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [ + {"name": "Hash", "version": "https://github.com/o/Hash.git"} + ], + }, + "o/Hash": {"name": "Hash"}, + }, + ) + emitted: list[str] = [] + convert_libraries( + [Library("esphome/A", "1.0.0", None)], + _backend(emit=lambda c: emitted.append(c.name), provides=lambda name: True), + ) + assert "Skip backend-provided" not in caplog.text + assert "using the library bundled" not in caplog.text + assert any("o/hash" in n.lower() for n in emitted) + + +def test_versionless_owner_qualified_dependency_warns_despite_provides( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """An owner-qualified version-less dependency is not satisfied by + provides(); it must still warn.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "Wire", "owner": "Foo"}], + } + }, + ) + convert_libraries( + [Library("esphome/A", None, None)], + _backend(provides=lambda name: name == "Wire"), + ) + assert "Wire of esphome/A has no version to resolve" in caplog.text + + +def test_versionless_provided_dependency_stays_quiet( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """An owner-less version-less dependency the backend provides is added + by the backend after emit; no reconciliation warning.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": {"name": "A", "dependencies": [{"name": "Wire"}]}}, + ) + convert_libraries( + [Library("esphome/A", None, None)], + _backend(provides=lambda name: name == "Wire"), + ) + assert "has no version to resolve" not in caplog.text + + +def test_versionless_dependency_requested_top_level_stays_quiet( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A version-less dependency the config also requests top-level is in + the build; no drop warning even without a provides backend.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": {"name": "A", "dependencies": [{"name": "Hash"}]}, + "Hash": {"name": "Hash"}, + }, + ) + convert_libraries( + [Library("esphome/A", None, None), Library("Hash", None, None)], + _backend(), + ) + assert "has no version to resolve" not in caplog.text + + +def test_versionless_url_ish_dependency_name_warns_cleanly( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A malformed URL-ish dependency name falls to the drop warning, never + a RuntimeError out of the key parser.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": {"name": "A", "dependencies": [{"name": "file://"}]}}, + ) + convert_libraries( + [Library("esphome/A", None, None)], _backend(provides=lambda name: False) + ) + assert ( + "file:// of esphome/A has no version to resolve and nothing provides it" + in caplog.text + ) + + +def test_versionless_dependency_matching_resolved_manifest_name_stays_quiet( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A bare name satisfied by an owner-qualified component's manifest + name is not a drop.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": {"name": "A", "dependencies": [{"name": "B"}]}, + "esphome/B": {"name": "B"}, + }, + ) + convert_libraries( + [Library("esphome/A", None, None), Library("esphome/B", None, None)], + _backend(), + ) + assert "has no version to resolve" not in caplog.text