Closed HTRamsey closed 8 months ago
@HTRamsey thanks for the PR. I'll work on getting this reviewed and merged.
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
c565fd4
) 93.64% compared to head (4bab0fb
) 93.64%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Kudos, no new issues were introduced!
0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code
@AniruddhaKanhere I guess that's preference on if you still want to be able to use the traceENTER, traceEVENT, and traceEXIT macros for event groups separately from a generic call to xTimerPendFunctionCallFromISR
. I kept it that way because there are other functions that could have been macros instead as well (i.e. vEventGroupClearBitsCallback
& vEventGroupSetBitsCallback
, which by the way I am not sure these are ever used anywhere).
Clearing event group bits form an ISR isn't done within the ISR itself because its not deterministic - the operation is instead deferred to the timer task so it occurs outside of the ISR context. If you are not tracing, then xEventGroupsClearBitsFromISR() calls the function in the timer task directly (https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/include/event_groups.h#L443), but doing that when tracing obfuscates the reason for calling the timer task function. Hence, when tracing, xEventGroupsClearBitsFromISR() is a function in its own right. So the original code, before this PR, is the intentional behaviour. I recommend changing the PR to instead add a comment to the existing code explaining why it is so.
@RichardBarry from what I gathered from the rest of the code, the tracing macros (traceENTER, traceEVENT, traceEXIT, etc) are not dependent on configUSE_TRACE_FACILITY
. If those macros are undefined by the user, then this functionally becomes a direct call to xTimerPendFunctionCallFromISR
regardless of the value of configUSE_TRACE_FACILITY
. There are a handful of other functions that are like this as well (xQueueCreateSet
or xQueueSelectFromSetFromISR
just for example). It seemed redundant to change the definition based on configUSE_TRACE_FACILITY
, or at least inconsistent with other functions that contain a single operation as well.
Edit: Is the concern about the overhead of an extra function call on an unoptimized build? If so then I would suggest defining functions like xQueueSelectFromSetFromISR
in a similar manner.
Also an unrelated question, is there a method or reason behind the inconsistencies of certain function declarations being guarded by config checks and others only having checks around the function definitions? Example being xStreamBufferGenericCreate
doesn't have a configSUPPORT_DYNAMIC_ALLOCATION
check on its declaration but does for its definition while xQueueGenericCreate
does have a configSUPPORT_DYNAMIC_ALLOCATION
around its definition and declaration. Also xTaskCreateRestricted
doesn't check configSUPPORT_DYNAMIC_ALLOCATION
but xTaskCreateRestrictedStatic
does check configSUPPORT_STATIC_ALLOCATION
at their definitions. I would think it would be fine (cleaner and easier) to always declare functions and just declare based on the config values, because a linker error would occur if attempting to call a function that should be disabled otherwise.
Remove Trace Facility Requirement for Several Event Groups Functions
Description
There is no configUSE_TRACE_FACILITY requirement for these functions, and no mention in documentation of it only being used for debugging.
Test Steps
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.