From 9e1f717d9b56dd2c896df0ab40ec4c0f976f6358 Mon Sep 17 00:00:00 2001 From: Maarten ter Huurne Date: Fri, 9 Aug 2013 03:13:02 +0200 Subject: [PATCH] Made Clock destruction safe SDL does not guard against the final callback still executing when the call to remove the timer returns. So we have to make sure the object we access on the callback is not prematurely destructed. This commit uses shared_ptr and weak_ptr to ensure that. Note that we sort-of have a singleton again, only now it is private and safely destructed. --- src/clock.cpp | 130 +++++++++++++++++++++++++++++++++++--------------- src/clock.h | 30 +++++++----- 2 files changed, 110 insertions(+), 50 deletions(-) diff --git a/src/clock.cpp b/src/clock.cpp index c8fb89c..c422ebc 100644 --- a/src/clock.cpp +++ b/src/clock.cpp @@ -3,9 +3,46 @@ #include "debug.h" #include "inputmanager.h" +#include #include +class Clock::Timer { +public: + Timer(); + ~Timer(); + void start(); + void getTime(unsigned int &hours, unsigned int &minutes); + unsigned int callback(); + +private: + unsigned int update(); + + SDL_TimerID timerID; + int minutes, hours; +}; + +static std::weak_ptr globalTimer; + +/** + * Gets the global timer instance, or create it if it doesn't exist already. + * This function is not thread safe: only one thread at a time may call it. + */ +static std::shared_ptr globalTimerInstance() +{ + std::shared_ptr timer = globalTimer.lock(); + if (timer) { + return timer; + } else { + // Note: Separate start method is necessary because globalTimer must + // be written before callbacks can occur. + timer.reset(new Clock::Timer()); + globalTimer = timer; + timer->start(); + return timer; + } +} + static void notify() { SDL_UserEvent e = { @@ -20,47 +57,51 @@ static void notify() SDL_PushEvent((SDL_Event *) &e); } -extern "C" Uint32 clockCallbackFunc(Uint32 timeout, void *d); +extern "C" Uint32 callbackFunc(Uint32 /*timeout*/, void */*d*/) +{ + std::shared_ptr timer = globalTimer.lock(); + return timer ? timer->callback() : 0; +} -class Clock::Forwarder { - static unsigned int clockCallbackFunc(Clock *clock) - { - return clock->clockCallback(); +Clock::Timer::Timer() + : timerID(NULL) +{ + tzset(); +} + +Clock::Timer::~Timer() +{ + if (timerID) { + SDL_RemoveTimer(timerID); } - friend Uint32 clockCallbackFunc(Uint32 timeout, void *d); -}; - -extern "C" Uint32 clockCallbackFunc(Uint32 timeout, void *d) -{ - return Clock::Forwarder::clockCallbackFunc(static_cast(d)); } -unsigned int Clock::clockCallback() +void Clock::Timer::start() { + if (timerID) { + ERROR("SDL timer was already started\n"); + return; + } unsigned int ms = update(); - notify(); - return ms; + timerID = SDL_AddTimer(ms, callbackFunc, this); + if (!timerID) { + ERROR("Could not initialize SDL timer: %s\n", SDL_GetError()); + } } -std::string Clock::getTime(bool is24) +void Clock::Timer::getTime(unsigned int &hours, unsigned int &minutes) { - char buf[9]; - int h = hours; - bool pm = hours >= 12; - - if (!is24 && pm) - h -= 12; - - sprintf(buf, "%02i:%02i%s", h, minutes, is24 ? "" : (pm ? "pm" : "am")); - return std::string(buf); + hours = this->hours; + minutes = this->minutes; } -unsigned int Clock::update() +unsigned int Clock::Timer::update() { struct timeval tv; struct tm result; gettimeofday(&tv, NULL); localtime_r(&tv.tv_sec, &result); + // TODO: Store both in a single 32-bit write? minutes = result.tm_min; hours = result.tm_hour; DEBUG("Time updated: %02i:%02i:%02i\n", hours, minutes, result.tm_sec); @@ -69,28 +110,41 @@ unsigned int Clock::update() // We don't need high precision, but it is important that any deviation is // past the minute mark, so the fetched hour and minute number belong to // the freshly started minute. + // TODO: Does the SDL timer in fact guarantee we're never called early? + // "ms = t->interval - SDL_TIMESLICE;" worries me. // Clamping it at 1 sec both avoids overloading the system in case our // computation goes haywire and avoids passing 0 to SDL, which would stop // the recurring timer. return std::max(1, (60 - result.tm_sec)) * 1000; } -void Clock::addTimer(int timeout) +unsigned int Clock::Timer::callback() { - timer = SDL_AddTimer(timeout, clockCallbackFunc, this); - if (timer == NULL) - ERROR("Could not initialize SDLTimer: %s\n", SDL_GetError()); + unsigned int ms = update(); + notify(); + // TODO: SDL timer forgets adjusted interval if a timer was inserted or + // removed during the callback. So we should either fix that bug + // in SDL or ensure we don't insert/remove timers at runtime. + // The blanking timer is inserted/removed quite a lot at time moment, + // but it could be reprogrammed to adjust the interval instead. + return ms; +} + +std::string Clock::getTime(bool is24) +{ + unsigned int hours, minutes; + timer->getTime(hours, minutes); + + bool pm = hours >= 12; + if (!is24 && pm) + hours -= 12; + + char buf[9]; + sprintf(buf, "%02i:%02i%s", hours, minutes, is24 ? "" : (pm ? "pm" : "am")); + return std::string(buf); } Clock::Clock() + : timer(globalTimerInstance()) { - tzset(); - - unsigned int ms = update(); - addTimer(ms); -} - -Clock::~Clock() -{ - SDL_RemoveTimer(timer); } diff --git a/src/clock.h b/src/clock.h index 4a72d1f..5ddd7ef 100644 --- a/src/clock.h +++ b/src/clock.h @@ -1,26 +1,32 @@ #ifndef __CLOCK_H__ #define __CLOCK_H__ +#include #include -#include + +/** + * A keeper of wall clock time with minute accuracy. + * Sends a user event on every minute boundary, to force a repaint of the + * clock display. + */ class Clock { public: - Clock(); - ~Clock(); + /** + * Used by implementation; please ignore. + */ + class Timer; + Clock(); + + /** + * Gets a string representation of the current time. + * Uses 24-hour format if is24 is true, otherwise AM/PM. + */ std::string getTime(bool is24 = true); - class Forwarder; - friend Forwarder; - private: - void addTimer(int timeout); - unsigned int update(); - unsigned int clockCallback(); - - SDL_TimerID timer; - int minutes, hours; + std::shared_ptr timer; }; #endif /* __CLOCK_H__ */