Skip to content

Add optional tracing to sleep manager lock/unlock #6142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from Mar 2, 2018
Merged

Add optional tracing to sleep manager lock/unlock #6142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0f6b73a
Add optional tracing to sleep manager lock/unlock
Feb 5, 2018
74bdf1c
Rename SLEEP_PROFILING_ENABLED to MBED_SLEEP_STATS_ENABLED
Feb 23, 2018
21e5f11
Remove '_PACKED' keyword from sleep statistic structure
Feb 26, 2018
5a4027b
Remove newline printf from sleep manager
Feb 27, 2018
bd23625
Rename SLEEP_STATS_ENABLED to SLEEP_TRACING_ENABLED
Feb 27, 2018
076548a
Split find/insert sleep tracker function into two seperate functions.
Mar 1, 2018
fed5115
Return pointer to sleep tracker array element instead of an index
Mar 1, 2018
206cc29
Fix merge conflict mishap
Mar 1, 2018
d6f57bc
Fix review comments and merge issues
bulislaw Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 129 additions & 4 deletions hal/mbed_sleep_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,140 @@
* limitations under the License.
*/

#include "mbed_assert.h"
#include "mbed_power_mgmt.h"
#include "mbed_critical.h"
#include "sleep_api.h"
#include "mbed_error.h"
#include "mbed_debug.h"
#include <limits.h>
#include <stdio.h>

#if DEVICE_SLEEP

// deep sleep locking counter. A target is allowed to deep sleep if counter == 0
static uint16_t deep_sleep_lock = 0U;

void sleep_manager_lock_deep_sleep(void)
#ifdef MBED_SLEEP_TRACING_ENABLED

// Length of the identifier extracted from the driver name to store for logging.
#define IDENTIFIER_WIDTH 15
// Number of drivers that can be stored in the structure
#define STATISTIC_COUNT 10

typedef struct sleep_statistic {
char identifier[IDENTIFIER_WIDTH];
uint8_t count;
} sleep_statistic_t;

static sleep_statistic_t sleep_stats[STATISTIC_COUNT];

static const char* strip_path(const char* const filename)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this runtime processing can be avoided, if we use proper compile time pre-processing macros (__MODULE__ for ARM and __BASE_FILE__ for GCC_ARM). Not blocking the release, but please update this in next PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine for ARM and GCC, but the only way to do it for IAR seems to be enabling --no_path_in_file_macros which seems excessive unless it can be applied selectively to this file.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--no_path_in_file_macros : I guess we will be adding this anyways in future for unified tracing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using filename can we use function name of the caller( the caller will pass __FUNCTION__ ), that will avoid all these processing as well, does that work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using __BASE_FILE__ would work better, it can be added when this is altered to use the tracing.

Using __FUNCTION__ might be problematic. The names of the functions that call lock and unlock are going to be different so tracking which unlock is associated with each lock would be complicated, multiple drivers are also likely to have the same function names that lock/unlock the sleep manager, and it's difficult to identify which driver locked the sleep manager based on the function name alone

{
char *output = strrchr(filename, '/');

if (output != NULL) {
return output + 1;
}

output = strrchr(filename, '\\');

if (output != NULL) {
return output + 1;
}

return filename;
}

static sleep_statistic_t* sleep_tracker_find(const char *const filename)
{
char temp[IDENTIFIER_WIDTH];
strncpy(temp, filename, IDENTIFIER_WIDTH);
temp[IDENTIFIER_WIDTH - 1] = '\0';

// Search for the a driver matching the current name and return it's index
for (int i = 0; i < STATISTIC_COUNT; ++i) {
if (strcmp(sleep_stats[i].identifier, temp) == 0) {
return &sleep_stats[i];
}
}

return NULL;
}

static sleep_statistic_t* sleep_tracker_add(const char* const filename)
{
char temp[IDENTIFIER_WIDTH];
strncpy(temp, filename, IDENTIFIER_WIDTH);
temp[IDENTIFIER_WIDTH - 1] = '\0';

for (int i = 0; i < STATISTIC_COUNT; ++i) {
if (sleep_stats[i].identifier[0] == '\0') {
core_util_critical_section_enter();
strncpy(sleep_stats[i].identifier, temp, sizeof(temp));
core_util_critical_section_exit();

return &sleep_stats[i];
}
}

debug("No free indexes left to use in mbed sleep tracker.\r\n");

return NULL;
}

static void sleep_tracker_print_stats(void)
{
debug("Sleep locks held:\r\n");
for (int i = 0; i < STATISTIC_COUNT; ++i) {
if (sleep_stats[i].count == 0) {
continue;
}

if (sleep_stats[i].identifier[0] == '\0') {
return;
}

debug("[id: %s, count: %u]\r\n", sleep_stats[i].identifier,
sleep_stats[i].count);
}
}

void sleep_tracker_lock(const char* const filename, int line)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scartmell-arm - Is it possible to modify this function to return the index of the entry in sleep_stats? And the unlock caller should pass that back into unlock function, so that we don't have to search for index when in unlock.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like that would require changing the user-facing API, and requiring anyone who uses the sleep tracker to keep track of the index. Currently, the sleep tracing is transparent to the user. I don't think that requiring them to track the index is worth the benefit of not having to look up the index during unlock, especially when this is meant intended primarily as an optional debugging feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding that sleep tracing is only available in debug mode and just the fact of using it will prevent the board from sleeping, so it's purely development time debug feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding that sleep tracing is only available in debug mode and just the fact of using it will prevent the board from sleeping, so it's purely development time debug feature

I dont see in this change set where that would be the case. Can you point me at it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicitly by using UART.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont understand how that is possible given UART is available in develop and release profiles as well.

{
const char* const stripped_path = strip_path(filename);

sleep_statistic_t* stat = sleep_tracker_find(stripped_path);

// Entry for this driver does not exist, create one.
if (stat == NULL) {
stat = sleep_tracker_add(stripped_path);
}

core_util_atomic_incr_u8(&stat->count, 1);

debug("LOCK: %s, ln: %i, lock count: %u\r\n", stripped_path, line, deep_sleep_lock);
}

void sleep_tracker_unlock(const char* const filename, int line)
{
const char* const stripped_path = strip_path(filename);
sleep_statistic_t* stat = sleep_tracker_find(stripped_path);

// Entry for this driver does not exist, something went wrong.
if (stat == NULL) {
debug("Unlocking sleep for driver that was not previously locked: %s, ln: %i\r\n", stripped_path, line);
return;
}

core_util_atomic_decr_u8(&stat->count, 1);

debug("UNLOCK: %s, ln: %i, lock count: %u\r\n", stripped_path, line, deep_sleep_lock);
}

#endif // MBED_SLEEP_TRACING_ENABLED

void sleep_manager_lock_deep_sleep_internal(void)
{
core_util_critical_section_enter();
if (deep_sleep_lock == USHRT_MAX) {
Expand All @@ -36,7 +158,7 @@ void sleep_manager_lock_deep_sleep(void)
core_util_critical_section_exit();
}

void sleep_manager_unlock_deep_sleep(void)
void sleep_manager_unlock_deep_sleep_internal(void)
{
core_util_critical_section_enter();
if (deep_sleep_lock == 0) {
Expand All @@ -54,6 +176,9 @@ bool sleep_manager_can_deep_sleep(void)

void sleep_manager_sleep_auto(void)
{
#ifdef MBED_SLEEP_TRACING_ENABLED
sleep_tracker_print_stats();
#endif
core_util_critical_section_enter();
// debug profile should keep debuggers attached, no deep sleep allowed
#ifdef MBED_DEBUG
Expand All @@ -73,12 +198,12 @@ void sleep_manager_sleep_auto(void)
// locking is valid only if DEVICE_SLEEP is defined
// we provide empty implementation

void sleep_manager_lock_deep_sleep(void)
void sleep_manager_lock_deep_sleep_internal(void)
{

}

void sleep_manager_unlock_deep_sleep(void)
void sleep_manager_unlock_deep_sleep_internal(void)
{

}
Expand Down
35 changes: 32 additions & 3 deletions platform/mbed_power_mgmt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#ifndef MBED_POWER_MGMT_H
#define MBED_POWER_MGMT_H

#include "hal/sleep_api.h"
#include "sleep_api.h"
#include "mbed_toolchain.h"
#include <stdbool.h>

Expand Down Expand Up @@ -63,6 +63,34 @@ extern "C" {
* }
* @endcode
*/
#ifdef MBED_SLEEP_TRACING_ENABLED

void sleep_tracker_lock(const char *const filename, int line);
void sleep_tracker_unlock(const char *const filename, int line);

#define sleep_manager_lock_deep_sleep() \
do \
{ \
sleep_manager_lock_deep_sleep_internal(); \
sleep_tracker_lock(__FILE__, __LINE__); \
} while (0);

#define sleep_manager_unlock_deep_sleep() \
do \
{ \
sleep_manager_unlock_deep_sleep_internal(); \
sleep_tracker_unlock(__FILE__, __LINE__); \
} while (0);

#else

#define sleep_manager_lock_deep_sleep() \
sleep_manager_lock_deep_sleep_internal()

#define sleep_manager_unlock_deep_sleep() \
sleep_manager_unlock_deep_sleep_internal()

#endif // MBED_SLEEP_TRACING_ENABLED

/** Lock the deep sleep mode
*
Expand All @@ -76,7 +104,7 @@ extern "C" {
* The lock is a counter, can be locked up to USHRT_MAX
* This function is IRQ and thread safe
*/
void sleep_manager_lock_deep_sleep(void);
void sleep_manager_lock_deep_sleep_internal(void);

/** Unlock the deep sleep mode
*
Expand All @@ -85,14 +113,15 @@ void sleep_manager_lock_deep_sleep(void);
* The lock is a counter, should be equally unlocked as locked
* This function is IRQ and thread safe
*/
void sleep_manager_unlock_deep_sleep(void);
void sleep_manager_unlock_deep_sleep_internal(void);

/** Get the status of deep sleep allowance for a target
*
* @return true if a target can go to deepsleep, false otherwise
*/
bool sleep_manager_can_deep_sleep(void);


/** Enter auto selected sleep mode. It chooses the sleep or deeepsleep modes based
* on the deepsleep locking counter
*
Expand Down
1 change: 0 additions & 1 deletion platform/mbed_sleep.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef MBED_MBED_SLEEP_H
#define MBED_MBED_SLEEP_H

Expand Down