Parcourir la source

Extend single-threaded draw-lambda safety to every page, not just the first

Page 0's draw lambda already got this for free: update()'s caller sits
blocked on lambda_done_sem_ while it runs, so nothing else can race it.
Later pages (paged models, height_divisor > 1) run after update() has
already returned, with the caller back to doing its own thing - no
protection at all against racing whatever the lambda reads (a sensor,
a global, ...).

Cheaply close that gap with vTaskSuspend()/vTaskResume(): freeze the
caller task around each subsequent page's do_update_() call, exactly
long enough for that page's lambda to run, then resume it before the
(slow) hardware write for that page so the caller stays responsive
through the part that actually takes a while. No cooperation needed
from the caller task itself, which is what makes this affordable
without restructuring anything - there's no existing esphome pattern
for a component running its own draw callback off the main task to
lean on here.
ZiB il y a 1 mois
Parent
commit
bebba98fd3
1 fichiers modifiés avec 28 ajouts et 4 suppressions
  1. 28 4
      components/gxepd2/gxepd2_display.h

+ 28 - 4
components/gxepd2/gxepd2_display.h

@@ -147,14 +147,18 @@ class GxEPD2Display : public display::DisplayBuffer {
       ESP_LOGW(TAG, "Skipping update: previous display refresh is still running");
       return;
     }
+    // update_task_loop_() suspends this exact task around every later
+    // page's draw lambda too (see there) - it needs to know who to
+    // suspend, since after this call returns nothing keeps it waiting for
+    // update_task_ to signal it.
+    this->caller_task_ = xTaskGetCurrentTaskHandle();
     xTaskNotifyGive(this->update_task_);
     // Block only through this frame's draw lambda - not through the SPI
     // write / physical refresh that follows, which continues on
     // update_task_ after we return. For a paged (height_divisor > 1) model
-    // this only covers the first page: the lambda runs again per
-    // subsequent page, interleaved with that page's hardware write, fully
-    // in the background. Most models are single-page, where this is the
-    // entire draw phase.
+    // this covers only the first page's lambda call synchronously; later
+    // pages' lambda calls happen after we've returned, protected instead
+    // by briefly suspending this task around each one (below).
     xSemaphoreTake(this->lambda_done_sem_, portMAX_DELAY);
   }
 
@@ -218,6 +222,18 @@ class GxEPD2Display : public display::DisplayBuffer {
       this->driver_->firstPage();
       bool first_page = true;
       do {
+        if (!first_page) {
+          // update() already returned after the first page, so the caller
+          // task is off running its own thing again - nothing keeps this
+          // page's draw lambda from racing whatever it's doing (reading a
+          // sensor, updating a global, ...). Freeze it just long enough to
+          // draw this page too, so every page's lambda gets the same
+          // "nothing else is running" guarantee the first one had for
+          // free, not just the first. Resumed right after, before the
+          // (slow) hardware write below, so the caller stays responsive
+          // through the part that actually takes a while.
+          vTaskSuspend(this->caller_task_);
+        }
         this->do_update_();
         if (first_page) {
           // update() is waiting on exactly this: the draw lambda for the
@@ -226,6 +242,8 @@ class GxEPD2Display : public display::DisplayBuffer {
           // without it.
           xSemaphoreGive(this->lambda_done_sem_);
           first_page = false;
+        } else {
+          vTaskResume(this->caller_task_);
         }
       } while (this->driver_->nextPage());
       ESP_LOGD(TAG, "Display update took %u ms", (unsigned) (millis() - start));
@@ -234,6 +252,12 @@ class GxEPD2Display : public display::DisplayBuffer {
   }
 
   TaskHandle_t update_task_{nullptr};
+  // Set fresh by update() every call, before update_task_ can possibly read
+  // it (it's blocked on lambda_done_sem_ until the first page finishes) -
+  // safe without extra synchronization since update_running_ already
+  // guarantees only one update cycle, and thus only one writer/reader pair
+  // for this field, is ever in flight at a time.
+  TaskHandle_t caller_task_{nullptr};
   SemaphoreHandle_t lambda_done_sem_{nullptr};
   std::atomic<bool> update_running_{false};
 };