Hygenic tracing hooks

[schilkp]

Description

Hi! After the long back-and-forth on the forum (https://forums.freertos.org/t/tracing-improvements/20097), here is the PR that implements the changes to the tracing macro semantics discussed.

I have begun to referring to it as "hygienic" tracing macros, that provide all the information a tracer would reasonably require solely through:

• Explicit parameters
• pxCurrentTCB

I have gone through the tracing hooks of both systemview and tracealyzer and (hopefully) cover all the information that they previously accessed "un-hygienically".

Please note the individual commit messages: I have tried to justify and explain every change there.

Furthermore, I have marked this as a draft because I expect some discussion and changes. I already have a few points that I would like to raise:

• I have stuck to replacing hooks with an _EXT version that falls back on the "legacy" hook which should behave identically. Is this how you wish to proceed? Or is there any interest in introducing breaking changes to clean up the API significantly? Are you happy with this naming scheme?

• The only variable that the documentation explicitly allows accessing in an "unhyigienic"/not through explicit hook arguments is pxCurrentTCB. I have stuck with this, and not added that to any hooks. I presume that that is such a core part of the FreeRTOS kernel, that it would be OK to continue to allow tracers to explicitly depend on this. Or would you rather see this changed as well? I expect that to be very invasive, and most hooks to be replaced with an _EXT version.

• There is a large number of API operations that "may block and then succeed/fail" (queue receive/send/peek, streambuffer tx/rx, event group wait/sync....). Most of these operations featured 3 hooks:

  BLOCKING_ON_OPERATION (or similar), if the task blocks to complete, and then an OPERATION if successfully and OPERATION_FAILED hook if not. The exception here are APIs like EventGroups, which seemingly don't have a clear notion of what "success" would be and instead have a _END macro that also inidcates if a timeout occured.

  I have not yet updated any of the 'BLOCKING_ON_OPERATION' hooks because I am not decided what exactly they should expose.

  In my eyes, it makes sense to expose everything that relates to the reason the task would wait again:

  • The value of xTicksToDelay
  • In the case of stream buffers, the number of bytes to receive
  • In the case of event groups, if waiting for one of a set of events or all of a set of events to occur
  • etc...

  Does that seem reasonable?

  If so, I would like to ask for a bit of support because I don't 100% understand how the timeout mechanism for (for example) queues works. As far as I can tell, the task blocking on a queue can be woken before the queue is ready and the timeout occured? If so, how can this happen? In that case, xTaskChekForTimeOut will figure out how many ticks of timeout are left, update xTicksToWait and again block, presumably triggering a second call to the BLOCKING_ON macro. It seems therefor reasonable to have the BLOCKING_ON macro expose the current value of xTicksToWait? Or should it expose the initial value of xTicksToWait again?

  • On a first pass through the timer API and hooks I did not spot anything that seemed to require updates, but I will have another look.

• Should the stream buffer receive/send hooks expose both the amount of data to receive/send that was requested, and the amount that was actually received/sent?

Look forward to your feedback!

Test Steps

CMock tests continue to pass if the following tracing hooks are added to the duplicate FreeRTOS.h in the main FreeRTOS repo:

diff --git a/FreeRTOS/Test/CMock/tasks/tasks_freertos/FreeRTOS.h b/FreeRTOS/Test/CMock/tasks/tasks_freertos/FreeRTOS.h
index bd7b65c10..2833c6185 100644
--- a/FreeRTOS/Test/CMock/tasks/tasks_freertos/FreeRTOS.h
+++ b/FreeRTOS/Test/CMock/tasks/tasks_freertos/FreeRTOS.h
@@ -774,13 +774,20 @@
 #endif

 #ifndef traceTASK_DELAY_UNTIL
-    #define traceTASK_DELAY_UNTIL( x )
+    #define traceTASK_DELAY_UNTIL( xTimeToWake )
 #endif

 #ifndef traceTASK_DELAY
     #define traceTASK_DELAY()
 #endif

+#ifndef traceTASK_DELAY_EXT
+
+/* Extended version of traceTASK_DELAY that also exposes the number of ticks
+ * to delay for. */
+    #define traceTASK_DELAY_EXT( xTicksToDelay )    traceTASK_DELAY()
+#endif
+
 #ifndef traceTASK_PRIORITY_SET
     #define traceTASK_PRIORITY_SET( pxTask, uxNewPriority )
 #endif
@@ -893,6 +900,23 @@
     #define traceTASK_NOTIFY_TAKE( uxIndexToWait )
 #endif

+#ifndef traceTASK_NOTIFY_TAKE_EXT
+
+/* Extended version of traceTASK_NOTIFY_TAKE that also exposes value of
+ * xClearCountOnExit, informing the tracer of the state of this task
+ * notification after it has been taken. Note that this hook, unlike traceTASK_NOTIFY_TAKE,
+ * is only called if the notification was successfully taken. */
+    #define traceTASK_NOTIFY_TAKE_EXT( uxIndexToWait, xClearCountOnExit )    traceTASK_NOTIFY_TAKE( uxIndexToWait )
+#endif
+
+#ifndef traceTASK_NOTIFY_TAKE_FAILED
+
+/* Task notification take failed. For backwards-compatability, this macro falls
+ * back on traceTASK_NOTIFY_TAKE which was always called, no matter if
+ * successfull or not. */
+    #define traceTASK_NOTIFY_TAKE_FAILED( uxIndexToWait )    traceTASK_NOTIFY_TAKE( uxIndexToWait )
+#endif
+
 #ifndef traceTASK_NOTIFY_WAIT_BLOCK
     #define traceTASK_NOTIFY_WAIT_BLOCK( uxIndexToWait )
 #endif
@@ -901,18 +925,66 @@
     #define traceTASK_NOTIFY_WAIT( uxIndexToWait )
 #endif

+#ifndef traceTASK_NOTIFY_WAIT_EXT
+
+/* Extended version of traceTASK_NOTIFY_WAIT that also exposes value of
+ * ulBitsToClearOnExit, informing the tracer of the state of this task
+ * notification after it has been taken. Note that this hook, unlike
+ * traceTASK_NOTIFY_WAIT, is only called if the notification was successfully
+ * taken. */
+    #define traceTASK_NOTIFY_WAIT_EXT( uxIndexToWait, ulBitsToClearOnExit )    traceTASK_NOTIFY_WAIT( uxIndexToWait )
+#endif
+
+#ifndef traceTASK_NOTIFY_WAIT_FAILED
+
+/* Task notification wait failed. For backwards-compatability, this macro falls
+ * back on traceTASK_NOTIFY_WAIT which was always called, no matter if
+ * successfull or not. */
+    #define traceTASK_NOTIFY_WAIT_FAILED( uxIndexToWait )    traceTASK_NOTIFY_WAIT( uxIndexToWait )
+#endif
+
+
 #ifndef traceTASK_NOTIFY
     #define traceTASK_NOTIFY( uxIndexToNotify )
 #endif

+#ifndef traceTASK_NOTIFY_EXT
+
+/* Extended version of traceTASK_NOTIFY that also exposes the task being
+ * notified, and if/how the notification value was modified. */
+    #define traceTASK_NOTIFY_EXT( pxTCB, uxIndexToNotify, eAction, xReturn )    traceTASK_NOTIFY( uxIndexToNotify )
+#endif
+
 #ifndef traceTASK_NOTIFY_FROM_ISR
     #define traceTASK_NOTIFY_FROM_ISR( uxIndexToNotify )
 #endif

+#ifndef traceTASK_NOTIFY_FROM_ISR_EXT
+
+/* Extended version of traceTASK_NOTIFY_FROM_ISR that also exposes the task
+ * being notified, and if/how the notification value was modified. */
+    #define traceTASK_NOTIFY_FROM_ISR_EXT( pxTCB, uxIndexToNotify, eAction, xReturn )    traceTASK_NOTIFY_FROM_ISR( uxIndexToNotify )
+#endif
+
 #ifndef traceTASK_NOTIFY_GIVE_FROM_ISR
     #define traceTASK_NOTIFY_GIVE_FROM_ISR( uxIndexToNotify )
 #endif

+#ifndef traceTASK_NOTIFY_GIVE_FROM_ISR_EXT
+
+/* Extended version of traceTASK_NOTIFY_GIVE_FROM_ISR that also exposes the task
+ * being notified. */
+    #define traceTASK_NOTIFY_GIVE_FROM_ISR_EXT( pxTCB, uxIndexToNotify )    traceTASK_NOTIFY_GIVE_FROM_ISR( uxIndexToNotify )
+#endif
+
+#ifndef traceTASK_NOTIFY_STATE_CLEAR
+    #define traceTASK_NOTIFY_STATE_CLEAR( pxTCB, uxIndexToNotify )
+#endif
+
+#ifndef traceTASK_NOTIFY_VALUE_CLEAR
+    #define traceTASK_NOTIFY_VALUE_CLEAR( pxTCB, uxIndexToNotify, ulBitsToClear )
+#endif
+
 #ifndef traceISR_EXIT_TO_SCHEDULER
     #define traceISR_EXIT_TO_SCHEDULER()
 #endif

Checklist:

• [X] I have tested my changes. No regression in existing tests.
• [ ] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

NA

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sonarcloud[bot]]

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud