Hygenic tracing hooks #1085
Draft
Hygenic tracing hooks #1085
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xQueueGenericReset does not feature any tracing hooks besides the API ENTER and EXIT calls. This introduces two new tracing hooks that, in the same style as all other queue APIs, inform a tracer that a queue has been reset.
This commit adds extended versions of all queue send trace hooks (including FROM_ISR and FAILED variants) that also hygenically expose the value of xCopyPosition. This enables tracers to identify the type of queue send and therefor queue state after the operation, which was not possible without accessing scope variables before.
Adds an extended version of traceCREATE_COUNTING_SEMAPHORE that also exposes the handle of the semaphore. This provides the tracer with the initial semaphore count which is set after the call to traceQUEUE_CREATE and was not hygienically exposed without this hook.
The old traceTASK_NOTIFY_TAKE hook was always called (no matter the success or failure of the action) and did not hygienically expose enough information for the tracer to determine the state of the task notification after it has been taken. The new traceTASK_NOTIFY_TAKE_EXT hook is called only if the notification was taken successfully and hygienically expose xClearCountOnExit. The new traceTASK_NOTIFY_TAKE_FAILED hook is called if the notification could not be taken. This matches how the tracing hooks for all other APIs that attempt to receive/take a resource and my block function: First, if the task blocks, a BLOCK or BLOCKING trace hook is called. Then, either a normal or FAILED trace hook is called, indicating if the operation timed out or succeeded. Both hooks fall back on the old traceTASK_NOTIFY_TAKE, preserving its functionality for backwards compatibility.
Both xTaskGenericNotifyStateClear and ulTaskGenericNotifyValueClear did not feature any tracing hooks besides the ENTER and EXIT calls. This adds the relevant hooks to inform a tracer that the notification state/value has changed.
The previous versions of these macros did not hygienically expose what task was being notified, and how/if the notification value was notified.
The old traceTASK_NOTIFY_WAIT hook was always called (no matter the success or failure of the action) and did not hygienically expose enough information for the tracer to determine the state of the task notification after it has been taken. The new traceTASK_NOTIFY_WAIT_EXT hook is called only if the notification was taken successfully and hygienically expose ulBitsToClearOnExit. The new traceTASK_NOTIFY_WAIT_FAILED hook is called if the notification could not be taken. This matches how the tracing hooks for all other APIs that attempt to receive/take a resource and my block function: First, if the task blocks, a BLOCK or BLOCKING trace hook is called. Then, either a normal or FAILED trace hook is called, indicating if the operation timed out or succeeded. Both hooks fall back on the old traceTASK_NOTIFY_WAIT, preserving its functionality for backwards compatibility. Not that there is a very slight breaking change in this commit: traceTASK_NOTIFY_WAIT is now called after the out-var pulNotificationValue is set. Because this pointer was in an unknown/user-set state when the tracing hook was previously called, it is highly unlikely that there are any tracers that rely on this.
…OTIFICATION_INDEX hooks. This enables tracers to track both the trigger level and notification index of a stream buffer if they are changed during operation.
Adds an extended version of traceTASK_DELAY that also exposes the number of ticks to delay.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
pxCurrentTCBI 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
_EXTversion that falls back on the "legacy" hook which should behaveidentically. 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 issuch 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
_EXTversion.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 anOPERATIONif successfully andOPERATION_FAILEDhook 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_ENDmacro 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:
xTicksToDelayDoes 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,
xTaskChekForTimeOutwill figure out how many ticks of timeout are left, updatexTicksToWaitand again block, presumably triggering a second call to theBLOCKING_ONmacro. It seems therefor reasonable to have theBLOCKING_ONmacro 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:
Checklist:
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.