Remove Trace Facility Requirement for Several Event Groups Functions

[HTRamsey]

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:

• 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

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

[paulbartell]

@HTRamsey thanks for the PR. I'll work on getting this reviewed and merged.

[AniruddhaKanhere]

I am not sure I understand this change completely.

If we are removing the dependence of the function definitions and declarations from configUSE_TRACE_FACILITY then should we not use the #defined definitions instead of the ones which have tracing macro calls (i.e. traceENTER_xEventGroupClearBitsFromISR) in them?

Am I missing something? Can you please take a look at my question @HTRamsey?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c565fd4) 93.64% compared to head (4bab0fb) 93.64%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #969   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files           6        6           
  Lines        3194     3194           
  Branches      885      885           
=======================================
  Hits         2991     2991           
  Misses         91       91           
  Partials      112      112           

Flag	Coverage Δ
unittests	93.64% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sonarqubecloud]

Quality Gate passed

Kudos, no new issues were introduced!

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

See analysis details on SonarCloud

[HTRamsey]

@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).

[RichardBarry]

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.

[HTRamsey]

@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.