Some comments and minor fixes

particle-iot · Jan 18, 2018 · 95628c0 · 95628c0
1 parent a266d1d
commit 95628c0
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 12 deletions.
diff --git a/hal/src/stm32f2xx/concurrent_hal.cpp b/hal/src/stm32f2xx/concurrent_hal.cpp
@@ -134,11 +134,12 @@ static BaseType_t os_thread_dump_helper(TaskStatus_t* const status, void* data)
 
 os_result_t os_thread_dump(os_thread_t thread, os_thread_dump_callback_t callback, void* ptr)
 {
+    // NOTE: callback is executed with thread scheduling disabled
+    // returning anything other than 0 in the callback will prevent further callback invocations,
+    // stopping iteration over the threads.
     TaskStatus_t status = {0};
     os_thread_dump_helper_t data = {callback, thread, ptr};
-    // vTaskSuspendAll();
     uxTaskGetSystemState(&status, 1, nullptr, os_thread_dump_helper, (void*)&data);
-    // xTaskResumeAll();
 
     return 0;
 }

diff --git a/platform/MCU/gcc/inc/platform_tracer.h b/platform/MCU/gcc/inc/platform_tracer.h
@@ -32,6 +32,8 @@ inline void* platform_get_current_pc(void) {
 #define __SW_RETURN_ADDRESS(i) case i: return __builtin_return_address(i)
 
 inline void* platform_get_return_address(int idx) {
+    // XXX: switch/case is intentional. DO NOT change to simple __builtin_return_address(idx)
+    // __builtin_return_address() needs to take a constant here, otherwise GCC is not happy
     switch(idx) {
         __SW_RETURN_ADDRESS(0);
         __SW_RETURN_ADDRESS(1);

diff --git a/services/inc/tracer_service.h b/services/inc/tracer_service.h
@@ -71,8 +71,8 @@ int tracer_set_callbacks_(tracer_callbacks_t* cb, void* reserved);
 int tracer_save_checkpoint(tracer_checkpoint_t* chkpt, uint32_t flags, void* reserved);
 int tracer_save_checkpoint_(tracer_checkpoint_t* chkpt, uint32_t flags, void* reserved);
 int tracer_save_checkpoint__(tracer_checkpoint_t* chkpt, uint32_t flags, void* reserved);
-size_t tracer_dump_saved(char* buf, size_t bufSize);
-size_t tracer_dump_current(char* buf, size_t bufSize);
+size_t tracer_dump_saved(char* buf, size_t bufSize, void* reserved);
+size_t tracer_dump_current(char* buf, size_t bufSize, void* reserved);
 
 #ifdef __cplusplus
 }

diff --git a/system/inc/system_dynalib.h b/system/inc/system_dynalib.h
@@ -89,8 +89,8 @@ DYNALIB_FN(BASE_IDX + 6, system, led_pattern_period, uint16_t(int, int, void*))
 DYNALIB_FN(BASE_IDX + 7, system, system_set_tester_handlers, int(system_tester_handlers_t*, void*))
 DYNALIB_FN(BASE_IDX + 8, system, system_format_diag_data, int(const uint16_t*, size_t, unsigned, appender_fn, void*, void*))
 DYNALIB_FN(BASE_IDX + 9, system, tracer_save_checkpoint, int(tracer_checkpoint_t*, uint32_t, void*))
-DYNALIB_FN(BASE_IDX + 10, system, tracer_dump_saved, size_t(char*, size_t))
-DYNALIB_FN(BASE_IDX + 11, system, tracer_dump_current, size_t(char*, size_t))
+DYNALIB_FN(BASE_IDX + 10, system, tracer_dump_saved, size_t(char*, size_t, void*))
+DYNALIB_FN(BASE_IDX + 11, system, tracer_dump_current, size_t(char*, size_t, void*))
 DYNALIB_END(system)
 
 #undef BASE_IDX

diff --git a/system/src/system_control.cpp b/system/src/system_control.cpp
@@ -331,9 +331,9 @@ void SystemControlInterface::processSystemRequest(void* data) {
 
   case USB_REQUEST_GET_TRACE: {
     if (req->value) {
-      req->reply_size = tracer_dump_current(req->data, USB_REQUEST_BUFFER_SIZE);
+      req->reply_size = tracer_dump_current(req->data, USB_REQUEST_BUFFER_SIZE, nullptr);
     } else {
-      req->reply_size = tracer_dump_saved(req->data, USB_REQUEST_BUFFER_SIZE);
+      req->reply_size = tracer_dump_saved(req->data, USB_REQUEST_BUFFER_SIZE, nullptr);
     }
     if (req->reply_size > USB_REQUEST_BUFFER_SIZE) {
       req->reply_size = USB_REQUEST_BUFFER_SIZE;

diff --git a/system/src/system_tracer.cpp b/system/src/system_tracer.cpp
@@ -64,8 +64,10 @@ int tracer_save_checkpoint(tracer_checkpoint_t* chkpt, uint32_t flags, void* res
     os_thread_t thread = os_thread_current();
     os_unique_id_t id = os_thread_unique_id(thread);
 
+    // We are always blocking interrupts instead of os thread scheduling here
+    // because tracer_save_checkpoint() may be called from an interrupt handler
     // bool isr = HAL_IsISR();
-    bool isr = true;
+    const bool isr = true;
 
     uintptr_t st = TracerService::lock(isr);
     auto tracer = TracerService::instance();
@@ -107,9 +109,11 @@ int tracer_save_checkpoint(tracer_checkpoint_t* chkpt, uint32_t flags, void* res
     return 0;
 }
 
-size_t tracer_dump_current(char* buf, size_t bufSize) {
+size_t tracer_dump_current(char* buf, size_t bufSize, void* reserved) {
+    // We are always blocking interrupts instead of os thread scheduling here
+    // because tracer_save_checkpoint() may be called from an interrupt handler
     // bool isr = HAL_IsISR();
-    bool isr = true;
+    const bool isr = true;
     uintptr_t st = TracerService::lock(isr);
     auto tracer = TracerService::instance();
     size_t sz = tracer->dumpCurrent(buf, bufSize);
@@ -118,7 +122,7 @@ size_t tracer_dump_current(char* buf, size_t bufSize) {
     return sz;
 }
 
-size_t tracer_dump_saved(char* buf, size_t bufSize) {
+size_t tracer_dump_saved(char* buf, size_t bufSize, void* reserved) {
     auto tracer = TracerService::instance();
     size_t sz = tracer->dumpSaved(buf, bufSize);