pw_thread_freertos: Fix peak stack usage capture size

Corrects the logic for the captured peak stack usage by the thread
iteration facade in pw_thread_freertos and adds an associated test to
verify correctness. uxTaskGetStackHighWaterMark() returns remaining free
space in words rather than bytes, so the returned value needs to be
multiplied by sizeof(StackType_t) to get the remaining unused stack
space in bytes.

Change-Id: Ifcd4289053c8aa91019475584f1206a12bd6cd37
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/110971
Pigweed-Auto-Submit: Armando Montanez <amontanez@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Reviewed-by: Ewout van Bekkum <ewout@google.com>

5 files changed, 113 insertions, 14 deletions

diff --git a/pw_thread_freertos/BUILD.bazel b/pw_thread_freertos/BUILD.bazel
index 2920f45f1..a81037ce6 100644
--- a/pw_thread_freertos/BUILD.bazel
+++ b/pw_thread_freertos/BUILD.bazel
@@ -186,6 +186,7 @@ pw_cc_library(
 pw_cc_library(
     name = "thread_iteration",
     srcs = [
+        "pw_thread_freertos_private/thread_iteration.h",
         "thread_iteration.cc",
     ],
     hdrs = [
@@ -207,13 +208,19 @@ pw_cc_library(
 pw_cc_test(
     name = "thread_iteration_test",
     srcs = [
+        "pw_thread_freertos_private/thread_iteration.h",
         "thread_iteration_test.cc",
     ],
     deps = [
-        "dir_pw_span",
+        ":freertos_tasktcb",
+        ":static_test_threads",
         ":thread_iteration",
+        "//pw_bytes",
+        "//pw_span",
+        "//pw_string:builder",
         "//pw_string:util",
         "//pw_sync:thread_notification",
+        "//pw_thread:test_threads",
         "//pw_thread:thread",
         "//pw_thread:thread_info",
         "//pw_thread:thread_iteration",
diff --git a/pw_thread_freertos/BUILD.gn b/pw_thread_freertos/BUILD.gn
index 652399841..8568d51d4 100644
--- a/pw_thread_freertos/BUILD.gn
+++ b/pw_thread_freertos/BUILD.gn
@@ -146,7 +146,10 @@ if (pw_thread_freertos_FREERTOS_TSKTCB_BACKEND != "") {
       dir_pw_span,
       dir_pw_status,
     ]
-    sources = [ "thread_iteration.cc" ]
+    sources = [
+      "pw_thread_freertos_private/thread_iteration.h",
+      "thread_iteration.cc",
+    ]
   }
 }
 
@@ -254,14 +257,20 @@ pw_test_group("tests") {
 if (pw_thread_freertos_FREERTOS_TSKTCB_BACKEND != "") {
   pw_test("thread_iteration_test") {
     enable_if = pw_thread_THREAD_BACKEND == "$dir_pw_thread_freertos:thread"
-    sources = [ "thread_iteration_test.cc" ]
+    sources = [
+      "pw_thread_freertos_private/thread_iteration.h",
+      "thread_iteration_test.cc",
+    ]
     deps = [
+      ":freertos_tsktcb",
       ":static_test_threads",
       ":thread_iteration",
+      "$dir_pw_bytes",
       "$dir_pw_span",
+      "$dir_pw_string:builder",
       "$dir_pw_string:util",
       "$dir_pw_sync:thread_notification",
-      "$dir_pw_system",
+      "$dir_pw_third_party/freertos",
       "$dir_pw_thread:test_threads",
       "$dir_pw_thread:thread",
       "$dir_pw_thread:thread_info",
diff --git a/pw_thread_freertos/pw_thread_freertos_private/thread_iteration.h b/pw_thread_freertos/pw_thread_freertos_private/thread_iteration.h
new file mode 100644
index 000000000..f82b8f702
--- /dev/null
+++ b/pw_thread_freertos/pw_thread_freertos_private/thread_iteration.h
@@ -0,0 +1,25 @@
+// Copyright 2022 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+
+#pragma once
+
+#include "FreeRTOS.h"
+#include "pw_thread/thread_iteration.h"
+
+namespace pw::thread::freertos {
+
+bool StackInfoCollector(TaskHandle_t current_thread,
+                        const pw::thread::ThreadCallback& cb);
+
+}  // namespace pw::thread::freertos
diff --git a/pw_thread_freertos/thread_iteration.cc b/pw_thread_freertos/thread_iteration.cc
index dcf699c21..b300879b7 100644
--- a/pw_thread_freertos/thread_iteration.cc
+++ b/pw_thread_freertos/thread_iteration.cc
@@ -11,7 +11,6 @@
 // WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 // License for the specific language governing permissions and limitations under
 // the License.
-// #include "pw_thread_freertos/thread_iteration.h"
 
 #include "pw_thread/thread_iteration.h"
 
@@ -24,9 +23,10 @@
 #include "pw_thread/thread_info.h"
 #include "pw_thread_freertos/freertos_tsktcb.h"
 #include "pw_thread_freertos/util.h"
+#include "pw_thread_freertos_private/thread_iteration.h"
 
 namespace pw::thread {
-namespace {
+namespace freertos {
 
 bool StackInfoCollector(TaskHandle_t current_thread,
                         const pw::thread::ThreadCallback& cb) {
@@ -46,11 +46,13 @@ bool StackInfoCollector(TaskHandle_t current_thread,
 // Walk through the stack from start to end to measure the current peak
 // using high-water marked stack data.
 #if (portSTACK_GROWTH > 0)
-  thread_info.set_stack_peak_addr(thread_info.stack_high_addr().value() -
-                                  uxTaskGetStackHighWaterMark(current_thread));
+  thread_info.set_stack_peak_addr(
+      thread_info.stack_high_addr().value() -
+      (sizeof(StackType_t) * uxTaskGetStackHighWaterMark(current_thread)));
 #else
-  thread_info.set_stack_peak_addr(thread_info.stack_low_addr().value() +
-                                  uxTaskGetStackHighWaterMark(current_thread));
+  thread_info.set_stack_peak_addr(
+      thread_info.stack_low_addr().value() +
+      (sizeof(StackType_t) * uxTaskGetStackHighWaterMark(current_thread)));
 #endif  // portSTACK_GROWTH > 0
 #endif  // INCLUDE_uxTaskGetStackHighWaterMark
 #endif  // configRECORD_STACK_HIGH_ADDRESS
@@ -58,14 +60,14 @@ bool StackInfoCollector(TaskHandle_t current_thread,
   return cb(thread_info);
 }
 
-}  // namespace
+}  // namespace freertos
 
 // This will disable the scheduler.
 Status ForEachThread(const pw::thread::ThreadCallback& cb) {
   pw::thread::freertos::ThreadCallback adapter_cb =
-      [&cb](TaskHandle_t current_thread, eTaskState) {
-        return StackInfoCollector(current_thread, cb);
-      };
+      [&cb](TaskHandle_t current_thread, eTaskState) -> bool {
+    return freertos::StackInfoCollector(current_thread, cb);
+  };
   // Suspend scheduler.
   vTaskSuspendAll();
   Status status = pw::thread::freertos::ForEachThread(adapter_cb);
diff --git a/pw_thread_freertos/thread_iteration_test.cc b/pw_thread_freertos/thread_iteration_test.cc
index e6aafe89a..05f4e3625 100644
--- a/pw_thread_freertos/thread_iteration_test.cc
+++ b/pw_thread_freertos/thread_iteration_test.cc
@@ -17,13 +17,18 @@
 #include <cstddef>
 #include <string_view>
 
+#include "FreeRTOS.h"
 #include "gtest/gtest.h"
+#include "pw_bytes/span.h"
 #include "pw_span/span.h"
+#include "pw_string/string_builder.h"
 #include "pw_string/util.h"
 #include "pw_sync/thread_notification.h"
 #include "pw_thread/test_threads.h"
 #include "pw_thread/thread.h"
 #include "pw_thread/thread_info.h"
+#include "pw_thread_freertos/freertos_tsktcb.h"
+#include "pw_thread_freertos_private/thread_iteration.h"
 
 namespace pw::thread::freertos {
 namespace {
@@ -100,5 +105,56 @@ TEST(ThreadIteration, ForkOneThread) {
   EXPECT_TRUE(temp_struct.thread_exists);
 }
 
+#if INCLUDE_uxTaskGetStackHighWaterMark
+#if configRECORD_STACK_HIGH_ADDRESS
+
+TEST(ThreadIteration, StackInfoCollector_PeakStackUsage) {
+  // This is the value FreeRTOS expects, but it's worth noting that there's no
+  // easy way to get this value directly from FreeRTOS.
+  constexpr uint8_t tskSTACK_FILL_BYTE = 0xa5U;
+  std::array<StackType_t, 128> stack;
+  ByteSpan stack_bytes(as_writable_bytes(span(stack)));
+  std::memset(stack_bytes.data(), tskSTACK_FILL_BYTE, stack_bytes.size_bytes());
+
+  tskTCB fake_tcb;
+  StringBuilder sb(fake_tcb.pcTaskName);
+  sb.append("FakeTCB");
+  fake_tcb.pxStack = stack.data();
+  fake_tcb.pxEndOfStack = stack.data() + stack.size();
+
+  // Clobber bytes as if they were used.
+  constexpr size_t kBytesRemaining = 96;
+#if portSTACK_GROWTH > 0
+  std::memset(stack_bytes.data(),
+              tskSTACK_FILL_BYTE ^ 0x2b,
+              stack_bytes.size() - kBytesRemaining);
+#else
+  std::memset(&stack_bytes[kBytesRemaining],
+              tskSTACK_FILL_BYTE ^ 0x2b,
+              stack_bytes.size() - kBytesRemaining);
+#endif  // portSTACK_GROWTH > 0
+
+  ThreadCallback cb = [kBytesRemaining](const ThreadInfo& info) -> bool {
+    EXPECT_TRUE(info.stack_high_addr().has_value());
+    EXPECT_TRUE(info.stack_low_addr().has_value());
+    EXPECT_TRUE(info.stack_peak_addr().has_value());
+
+#if portSTACK_GROWTH > 0
+    EXPECT_EQ(info.stack_high_addr().value() - info.stack_peak_addr().value(),
+              kBytesRemaining);
+#else
+    EXPECT_EQ(info.stack_peak_addr().value() - info.stack_low_addr().value(),
+              kBytesRemaining);
+#endif  // portSTACK_GROWTH > 0
+    return true;
+  };
+
+  EXPECT_TRUE(
+      StackInfoCollector(reinterpret_cast<TaskHandle_t>(&fake_tcb), cb));
+}
+
+#endif  // INCLUDE_uxTaskGetStackHighWaterMark
+#endif  // configRECORD_STACK_HIGH_ADDRESS
+
 }  // namespace
 }  // namespace pw::thread::freertos