memfault / memfault-firmware-sdk

Memfault SDK for embedded systems. Observability, logging, crash reporting, and OTA all in one service. More information at https://docs.memfault.com.
https://memfault.com
Other
149 stars 75 forks source link

fix: compiler warnings #70

Closed fouge closed 5 months ago

fouge commented 5 months ago

explicit cast & unused variable

noahp commented 5 months ago

@fouge thanks for the patch! could you give an example of the compiler warning you see?

EDIT ah, reproduced:

ports/zephyr/common/memfault_zephyr_ram_regions.c: In function 'memfault_zephyr_get_task_regions':
ports/zephyr/common/memfault_zephyr_ram_regions.c:225:8: error: assignment to 'void *' from 'uintptr_t' {aka 'long unsigned int'} makes pointer from integer without a cast [-Werror=int-conversion]
  225 |     sp = thread->stack_info.start;
      |        ^
ports/zephyr/common/memfault_zephyr_ram_regions.c:220:20: error: unused variable 'stack_top' [-Werror=unused-variable]
  220 |     const uint32_t stack_top = thread->stack_info.start + thread->stack_info.size;
      |                    ^~~~~~~~~
cc1: all warnings being treated as errors
noahp commented 5 months ago

@fouge we'll publish an update with this patch:

diff --git a/ports/zephyr/common/memfault_zephyr_ram_regions.c b/ports/zephyr/common/memfault_zephyr_ram_regions.c
index 6b782649a7..804a9290d1 100644
--- a/ports/zephyr/common/memfault_zephyr_ram_regions.c
+++ b/ports/zephyr/common/memfault_zephyr_ram_regions.c
@@ -188,7 +188,7 @@ size_t memfault_zephyr_get_task_regions(sMfltCoredumpRegion *regions, size_t num
     // region, so we might be duplicating it here; it's a little wasteful, but
     // it's good to always prioritize the currently running stack, in case the
     // coredump is truncated due to lack of space.
-#if !CONFIG_MEMFAULT_COREDUMP_FULL_THREAD_STACKS
+#if !defined(CONFIG_MEMFAULT_COREDUMP_FULL_THREAD_STACKS)
     if ((uintptr_t)_kernel.cpus[0].current == (uintptr_t)thread) {
       // when collecting partial stacks, thread context is only valid when task
       // is _not_ running so we skip collecting it. just update the watermark
@@ -214,16 +214,15 @@ size_t memfault_zephyr_get_task_regions(sMfltCoredumpRegion *regions, size_t num
       ;

 #if defined(CONFIG_THREAD_STACK_INFO)
+  #if defined(CONFIG_MEMFAULT_COREDUMP_FULL_THREAD_STACKS)
+    // Capture the entire stack for this thread
+    size_t stack_size_to_collect = thread->stack_info.size;
+    sp = (void *)thread->stack_info.start;
+  #else
     // We know where the top of the stack is. Use that information to shrink
     // the area we need to collect if less than CONFIG_MEMFAULT_COREDUMP_STACK_SIZE_TO_COLLECT
     // is in use
     const uint32_t stack_top = thread->stack_info.start + thread->stack_info.size;
-
-  #if CONFIG_MEMFAULT_COREDUMP_FULL_THREAD_STACKS
-    // Capture the entire stack for this thread
-    size_t stack_size_to_collect = thread->stack_info.size;
-    sp = thread->stack_info.start;
-  #else
     size_t stack_size_to_collect =
       MEMFAULT_MIN(stack_top - (uint32_t)sp, CONFIG_MEMFAULT_COREDUMP_STACK_SIZE_TO_COLLECT);
   #endif
fouge commented 5 months ago

Thanks for checking this out @noahp Closing

noahp commented 5 months ago

FYI- this fix will be included in SDK v1.9.1, to be released later today. UPDATE It's available now: https://github.com/memfault/memfault-firmware-sdk/releases/tag/1.9.1