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.

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Add optional tracing to sleep manager lock/unlock
Add tracing output to console to track when drivers lock and unlock deep
sleep. Tracing output is enabled by configuring the
'SLEEP_PROFILING_ENABLED' at compile time.

- Wrapped sleep_manager_lock/sleep_manager_unlock in a macro to
conditionally call tracing functions when 'SLEEP_PROFILING_ENABLED' is
set.
- Define a global structure to track driver names and how many locks
they hold in the sleep manager.
  • Loading branch information
Steven Cartmell committed Mar 1, 2018
commit 0f6b73ae0f30d4aab8617fe62a6cc469bc5a352b
113 changes: 109 additions & 4 deletions hal/mbed_sleep_manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,123 @@
* 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 <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 SLEEP_PROFILING_ENABLED

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

typedef struct __PACKED sleep_statistic {
Copy link
Member

Choose a reason for hiding this comment

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

Remove the __PACKED it doesn't make any difference here and generally I'm sceptical about use of packed structures, it may have negative effects on performance and put pressure on the bus with unaligned accesses.

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)

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.

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 size_t sleep_tracker_find_index(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 i;
}
}

// If no driver was found currently in the structure, find the first array
// index that hasn't been used by a driver and fill it, then return the
// index.
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 i;
}
}

// Panic if there are no free indexes left to track with
MBED_ASSERT(true);

return -1;
}

static void sleep_tracker_print_stats(void)
{
for (int i = 0; i < STATISTIC_COUNT; ++i) {
if (sleep_stats[i].count == 0) {
continue;
}

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

printf("[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);
size_t index = sleep_tracker_find_index(stripped_path);

core_util_atomic_incr_u8(&sleep_stats[index].count, 1);

printf("LOCK: %s, ln: %i, lock count: %u\r\n", stripped_path, line, deep_sleep_lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this printf and not debug? If debug it could be stripped from a release build.

}

void sleep_tracker_unlock(const char* const filename, int line)
{
const char* const stripped_path = strip_path(filename);
size_t index = sleep_tracker_find_index(stripped_path);
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 - sleep_tracker_find_index functions adds the filename to sleep_stats if the entry is not found. Why should we add a new entry in unlock call? As of now, if somebody calls unlock without adding lock first we will still add the entry. Is that expected?

Copy link
Author

@ghost ghost Mar 1, 2018

Choose a reason for hiding this comment

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

It can be changed, though it's unlikely to cause problems at the moment. I could separate out the find and add code. If a driver is unlocking sleep manager without first locking it 's going to cause an error() eventually via the unlock code which checks for underflows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make that change, if somebody tried to call unlock without locking it should be treated as an error. We should not add a new entry and go with it which might cause inadvertent behaviors.


core_util_atomic_decr_u8(&sleep_stats[index].count, 1);

printf("UNLOCK: %s, ln: %i, lock count: %u\r\n", stripped_path, line, deep_sleep_lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this printf and not debug? If debug it could be stripped from a release build.

sleep_tracker_print_stats();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think its an overkill to print the stats on every unlock call, particularly when we are printing the LOCK/UNLOCK info in the sleep_tracker_lock/unlock calls. But, I'm not going to block this PR for that as its not a major blocker, but you may want to re-think on that.

printf("\r\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this printf inside sleep_tracker_print_stats, makes it look cleaner.

}

#endif // SLEEP_PROFILING_ENABLED

void sleep_manager_lock_deep_sleep_internal(void)
{
core_util_critical_section_enter();
if (deep_sleep_lock == USHRT_MAX) {
Expand All @@ -36,7 +141,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 Down Expand Up @@ -73,12 +178,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
117 changes: 31 additions & 86 deletions platform/mbed_power_mgmt.h
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 SLEEP_PROFILING_ENABLED

#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);

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

#else

#define sleep_manager_lock_deep_sleep() \
sleep_manager_lock_deep_sleep_internal()

#define sleep_manager_unlock_deep_sleep() \
sleep_manager_lock_deep_sleep_internal()

#endif // SLEEP_PROFILING_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,96 +113,13 @@ 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
*
* This function is IRQ and thread safe
*
* @note
* If MBED_DEBUG is defined, only hal_sleep is allowed. This ensures the debugger
* to be active for debug modes.
*
*/
void sleep_manager_sleep_auto(void);

/** Send the microcontroller to sleep
*
* @note This function can be a noop if not implemented by the platform.
* @note This function will be a noop in debug mode (debug build profile when MBED_DEBUG is defined).
* @note This function will be a noop while uVisor is in use.
* @note This function will be a noop if the following conditions are met:
* - The RTOS is present
* - The processor turn off the Systick clock during sleep
* - The target does not implement tickless mode
*
* The processor is setup ready for sleep, and sent to sleep using __WFI(). In this mode, the
* system clock to the core is stopped until a reset or an interrupt occurs. This eliminates
* dynamic power used by the processor, memory systems and buses. The processor, peripheral and
* memory state are maintained, and the peripherals continue to work and can generate interrupts.
*
* The processor can be woken up by any internal peripheral interrupt or external pin interrupt.
*
* @note
* The mbed interface semihosting is disconnected as part of going to sleep, and can not be restored.
* Flash re-programming and the USB serial port will remain active, but the mbed program will no longer be
* able to access the LocalFileSystem
*/
static inline void sleep(void)
{
#if !(defined(FEATURE_UVISOR) && defined(TARGET_UVISOR_SUPPORTED))
#if DEVICE_SLEEP
#if (MBED_CONF_RTOS_PRESENT == 0) || (DEVICE_STCLK_OFF_DURING_SLEEP == 0) || defined(MBED_TICKLESS)
sleep_manager_sleep_auto();
#endif /* (MBED_CONF_RTOS_PRESENT == 0) || (DEVICE_STCLK_OFF_DURING_SLEEP == 0) || defined(MBED_TICKLESS) */
#endif /* DEVICE_SLEEP */
#endif /* !(defined(FEATURE_UVISOR) && defined(TARGET_UVISOR_SUPPORTED)) */
}

/** Send the microcontroller to deep sleep
*
* @note This function can be a noop if not implemented by the platform.
* @note This function will be a noop in debug mode (debug build profile when MBED_DEBUG is defined)
* @note This function will be a noop while uVisor is in use.
*
* This processor is setup ready for deep sleep, and sent to sleep. This mode
* has the same sleep features as sleep plus it powers down peripherals and clocks. All state
* is still maintained.
*
* The processor can only be woken up by an external interrupt on a pin or a watchdog timer.
*
* @note
* The mbed interface semihosting is disconnected as part of going to sleep, and can not be restored.
* Flash re-programming and the USB serial port will remain active, but the mbed program will no longer be
* able to access the LocalFileSystem
*/

MBED_DEPRECATED_SINCE("mbed-os-5.6", "One entry point for an application, use sleep()")
static inline void deepsleep(void)
{
#if !(defined(FEATURE_UVISOR) && defined(TARGET_UVISOR_SUPPORTED))
#if DEVICE_SLEEP
sleep_manager_sleep_auto();
#endif /* DEVICE_SLEEP */
#endif /* !(defined(FEATURE_UVISOR) && defined(TARGET_UVISOR_SUPPORTED)) */
}

/** Resets the processor and most of the sub-system
*
* @note Does not affect the debug sub-system
*/
static inline void system_reset(void)
{
NVIC_SystemReset();
}

#ifdef __cplusplus
}
#endif
Expand Down