diff --git a/esphome/components/script/script.h b/esphome/components/script/script.h index 5a573a9fe1..80afa154de 100644 --- a/esphome/components/script/script.h +++ b/esphome/components/script/script.h @@ -99,12 +99,12 @@ template class RestartScript : public Script { * Ring buffer implementation: * - num_queued_ tracks the number of queued (waiting) instances, NOT including the currently running one * - queue_front_ points to the next item to execute (read position) - * - Buffer size is max_queued_ (the maximum number that can be queued) - * - Write position is calculated as: (queue_front_ + num_queued_) % max_queued_ - * - When an item finishes, queue_front_ advances: (queue_front_ + 1) % max_queued_ + * - Buffer size is max_runs_ - 1 (max total instances minus the running one) + * - Write position is calculated as: (queue_front_ + num_queued_) % (max_runs_ - 1) + * - When an item finishes, queue_front_ advances: (queue_front_ + 1) % (max_runs_ - 1) * - First execute() runs immediately without queuing (num_queued_ stays 0) * - Subsequent executes while running are queued starting at position 0 - * - Maximum total instances = 1 running + max_queued_ queued + * - Maximum total instances = max_runs_ (includes 1 running + (max_runs_ - 1) queued) */ template class QueueingScript : public Script, public Component { public: @@ -113,9 +113,9 @@ template class QueueingScript : public Script, public Com if (this->is_action_running() || this->num_queued_ > 0) { // num_queued_ is the number of *queued* instances (waiting, not including currently running) - // max_queued_ is the maximum number that can be queued - // So we reject when num_queued_ >= max_queued_ - if (this->num_queued_ >= this->max_queued_) { + // max_runs_ is the maximum *total* instances (running + queued) + // So we reject when num_queued_ + 1 >= max_runs_ (queued + running >= max) + if (this->num_queued_ + 1 >= this->max_runs_) { this->esp_logw_(__LINE__, ESPHOME_LOG_FORMAT("Script '%s' maximum number of queued runs exceeded!"), LOG_STR_ARG(this->name_)); return; @@ -123,8 +123,8 @@ template class QueueingScript : public Script, public Com this->esp_logd_(__LINE__, ESPHOME_LOG_FORMAT("Script '%s' queueing new instance (mode: queued)"), LOG_STR_ARG(this->name_)); - // Ring buffer: write to (queue_front_ + num_queued_) % max_queued_ - size_t write_pos = (this->queue_front_ + this->num_queued_) % this->max_queued_; + // Ring buffer: write to (queue_front_ + num_queued_) % (max_runs_ - 1) + size_t write_pos = (this->queue_front_ + this->num_queued_) % (this->max_runs_ - 1); // Use reset() to replace the unique_ptr this->var_queue_[write_pos].reset(new std::tuple(std::make_tuple(x...))); this->num_queued_++; @@ -147,24 +147,22 @@ template class QueueingScript : public Script, public Com // Dequeue: decrement count, read from front, advance read position this->num_queued_--; auto &vars = *this->var_queue_[this->queue_front_]; - this->queue_front_ = (this->queue_front_ + 1) % this->max_queued_; + this->queue_front_ = (this->queue_front_ + 1) % (this->max_runs_ - 1); this->trigger_tuple_(vars, typename gens::type()); } } - // Note: Method named set_max_runs() for backward compatibility with existing configs, - // but internally uses max_queued_ to clarify that it sets the max *queued* instances - void set_max_runs(int max_runs) { max_queued_ = max_runs; } + void set_max_runs(int max_runs) { max_runs_ = max_runs; } protected: // Lazy init queue on first use - avoids setup() ordering issues and saves memory // if script is never executed during this boot cycle inline void lazy_init_queue_() { if (this->var_queue_.capacity() == 0) { - // Allocate max_queued_ slots for queued items (running item is separate) - this->var_queue_.init(this->max_queued_); + // Allocate max_runs_ - 1 slots for queued items (running item is separate) + this->var_queue_.init(this->max_runs_ - 1); // Initialize all unique_ptr slots to nullptr - for (int i = 0; i < this->max_queued_; i++) { + for (int i = 0; i < this->max_runs_ - 1; i++) { this->var_queue_.push_back(nullptr); } } @@ -175,7 +173,7 @@ template class QueueingScript : public Script, public Com } int num_queued_ = 0; // Number of queued instances (not including currently running) - int max_queued_ = 0; // Maximum number of queued instances (not including running) + int max_runs_ = 0; // Maximum total instances (running + queued) size_t queue_front_ = 0; // Ring buffer read position (next item to execute) FixedVector>> var_queue_; // Ring buffer of queued parameters }; diff --git a/tests/integration/fixtures/script_queued.yaml b/tests/integration/fixtures/script_queued.yaml index 426d1c7234..996dd6436f 100644 --- a/tests/integration/fixtures/script_queued.yaml +++ b/tests/integration/fixtures/script_queued.yaml @@ -7,7 +7,7 @@ api: # Test 1: Queue depth with default max_runs=5 - action: test_queue_depth then: - - logger.log: "=== TEST 1: Queue depth (max_runs=5 means 1 running + 5 queued = 6 total, reject 7) ===" + - logger.log: "=== TEST 1: Queue depth (max_runs=5 means 5 total, reject 6-7) ===" - script.execute: id: queue_depth_script value: 1 @@ -64,7 +64,7 @@ api: # Test 4: Verify rejection (max_runs=3) - action: test_rejection then: - - logger.log: "=== TEST 4: Verify rejection (max_runs=3 means 1 running + 3 queued = 4 total, reject 5-8) ===" + - logger.log: "=== TEST 4: Verify rejection (max_runs=3 means 3 total, reject 4-8) ===" - script.execute: id: rejection_script val: 1 diff --git a/tests/integration/test_script_queued.py b/tests/integration/test_script_queued.py index 9db5a34e9c..414a27ca2e 100644 --- a/tests/integration/test_script_queued.py +++ b/tests/integration/test_script_queued.py @@ -70,7 +70,7 @@ async def test_script_queued( if match := queue_end.search(line): item = int(match.group(1)) - if item == 6 and not test1_complete.done(): + if item == 5 and not test1_complete.done(): test1_complete.set_result(True) if queue_reject.search(line): @@ -115,7 +115,7 @@ async def test_script_queued( if match := reject_end.search(line): item = int(match.group(1)) - if item == 4 and not test4_complete.done(): + if item == 3 and not test4_complete.done(): test4_complete.set_result(True) if reject_reject.search(line): @@ -145,11 +145,11 @@ async def test_script_queued( await asyncio.sleep(0.1) # Give time for rejections # Verify Test 1 - assert sorted(test_results["queue_depth"]["processed"]) == [1, 2, 3, 4, 5, 6], ( - f"Test 1: Expected to process items 1-6 (max_runs=5 means 5 queued + 1 running), got {sorted(test_results['queue_depth']['processed'])}" + assert sorted(test_results["queue_depth"]["processed"]) == [1, 2, 3, 4, 5], ( + f"Test 1: Expected to process items 1-5 (max_runs=5 means 5 total), got {sorted(test_results['queue_depth']['processed'])}" ) - assert test_results["queue_depth"]["rejections"] > 0, ( - "Test 1: Expected at least one rejection warning (item 7 should be rejected)" + assert test_results["queue_depth"]["rejections"] >= 2, ( + "Test 1: Expected at least 2 rejection warnings (items 6-7 should be rejected)" ) # Test 2: Ring buffer order @@ -188,11 +188,11 @@ async def test_script_queued( await asyncio.sleep(0.1) # Give time for rejections # Verify Test 4 - assert sorted(test_results["rejection"]["processed"]) == [1, 2, 3, 4], ( - f"Test 4: Expected to process items 1-4 (max_runs=3 means 3 queued + 1 running), got {sorted(test_results['rejection']['processed'])}" + assert sorted(test_results["rejection"]["processed"]) == [1, 2, 3], ( + f"Test 4: Expected to process items 1-3 (max_runs=3 means 3 total), got {sorted(test_results['rejection']['processed'])}" ) - assert test_results["rejection"]["rejections"] == 4, ( - f"Test 4: Expected 4 rejections (items 5-8), got {test_results['rejection']['rejections']}" + assert test_results["rejection"]["rejections"] == 5, ( + f"Test 4: Expected 5 rejections (items 4-8), got {test_results['rejection']['rejections']}" ) # Test 5: No parameters