Add timer/callback functionality to OSystem.

Previously this was done by SDL_AddTimer, but the new approach is much better:
- it is proper C++ (using std::thread and lambdas)
- It is not tied to the SDL toolkit, which is C-based
Also, re-added logic to reset events ~0.5 seconds after starting ROM emulation; this resets 'hold' events

Changes.txt

@@ -49,6 +49,9 @@
 
   * Fixed bug in autodetecting Genesis controllers.
 
+  * Fixed bug with 'hold' events; they are now released a short time after
+    starting a ROM.
+
   * When starting Stella for the first time, the first ROM selected will
     determine which path to use by default for subsequent runs.
 

src/common/TimerManager.cxx

@@ -0,0 +1,260 @@
+//============================================================================
+//
+//   SSSS    tt          lll  lll
+//  SS  SS   tt           ll   ll
+//  SS     tttttt  eeee   ll   ll   aaaa
+//   SSSS    tt   ee  ee  ll   ll      aa
+//      SS   tt   eeeeee  ll   ll   aaaaa  --  "An Atari 2600 VCS Emulator"
+//  SS  SS   tt   ee      ll   ll  aa  aa
+//   SSSS     ttt  eeeee llll llll  aaaaa
+//
+// Copyright (c) 1995-2018 by Bradford W. Mott, Stephen Anthony
+// and the Stella Team
+//
+// See the file "License.txt" for information on usage and redistribution of
+// this file, and for a DISCLAIMER OF ALL WARRANTIES.
+//============================================================================
+
+#include <cassert>
+#include "TimerManager.hxx"
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager::TimerManager()
+  : nextId(no_timer + 1),
+    queue(),
+    done(false)
+{
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager::~TimerManager()
+{
+  ScopedLock lock(sync);
+
+  // The worker might not be running
+  if (worker.joinable())
+  {
+    done = true;
+    lock.unlock();
+    wakeUp.notify_all();
+
+    // If a timer handler is running, this
+    // will make sure it has returned before
+    // allowing any deallocations to happen
+    worker.join();
+
+    // Note that any timers still in the queue
+    // will be destructed properly but they
+    // will not be invoked
+  }
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager::TimerId TimerManager::addTimer(
+    millisec msDelay,
+    millisec msPeriod,
+    const TFunction& func)
+{
+  ScopedLock lock(sync);
+
+  // Lazily start thread when first timer is requested
+  if (!worker.joinable())
+    worker = std::thread(&TimerManager::timerThreadWorker, this);
+
+  // Assign an ID and insert it into function storage
+  auto id = nextId++;
+  auto iter = active.emplace(id, Timer(id, Clock::now() + Duration(msDelay),
+      Duration(msPeriod), std::move(func)));
+
+  // Insert a reference to the Timer into ordering queue
+  Queue::iterator place = queue.emplace(iter.first->second);
+
+  // We need to notify the timer thread only if we inserted
+  // this timer into the front of the timer queue
+  bool needNotify = (place == queue.begin());
+
+  lock.unlock();
+
+  if (needNotify)
+    wakeUp.notify_all();
+
+  return id;
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+bool TimerManager::clear(TimerId id)
+{
+  ScopedLock lock(sync);
+  auto i = active.find(id);
+  return destroy_impl(lock, i, true);
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+void TimerManager::clear()
+{
+  ScopedLock lock(sync);
+  while (!active.empty())
+    destroy_impl(lock, active.begin(), queue.size() == 1);
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+std::size_t TimerManager::size() const noexcept
+{
+  ScopedLock lock(sync);
+  return active.size();
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+bool TimerManager::empty() const noexcept
+{
+  ScopedLock lock(sync);
+  return active.empty();
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager& TimerManager::global()
+{
+  static TimerManager singleton;
+  return singleton;
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+void TimerManager::timerThreadWorker()
+{
+  ScopedLock lock(sync);
+
+  while (!done)
+  {
+    if (queue.empty())
+    {
+      // Wait for done or work
+      wakeUp.wait(lock, [this] { return done || !queue.empty(); });
+      continue;
+    }
+
+    auto queueHead = queue.begin();
+    Timer& timer = *queueHead;
+    auto now = Clock::now();
+    if (now >= timer.next)
+    {
+      queue.erase(queueHead);
+
+      // Mark it as running to handle racing destroy
+      timer.running = true;
+
+      // Call the handler outside the lock
+      lock.unlock();
+      timer.handler();
+      lock.lock();
+
+      if (timer.running)
+      {
+        timer.running = false;
+
+        // If it is periodic, schedule a new one
+        if (timer.period.count() > 0)
+        {
+          timer.next = timer.next + timer.period;
+          queue.emplace(timer);
+        }
+        else
+        {
+          // Not rescheduling, destruct it
+          active.erase(timer.id);
+        }
+      }
+      else
+      {
+        // timer.running changed!
+        //
+        // Running was set to false, destroy was called
+        // for this Timer while the callback was in progress
+        // (this thread was not holding the lock during the callback)
+        // The thread trying to destroy this timer is waiting on
+        // a condition variable, so notify it
+        timer.waitCond->notify_all();
+
+        // The clearTimer call expects us to remove the instance
+        // when it detects that it is racing with its callback
+        active.erase(timer.id);
+      }
+    }
+    else
+    {
+      // Wait until the timer is ready or a timer creation notifies
+      Timestamp next = timer.next;
+      wakeUp.wait_until(lock, next);
+    }
+  }
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+// NOTE: if notify is true, returns with lock unlocked
+bool TimerManager::destroy_impl(ScopedLock& lock, TimerMap::iterator i,
+                                bool notify)
+{
+  assert(lock.owns_lock());
+
+  if (i == active.end())
+    return false;
+
+  Timer& timer = i->second;
+  if (timer.running)
+  {
+    // A callback is in progress for this Timer,
+    // so flag it for deletion in the worker
+    timer.running = false;
+
+    // Assign a condition variable to this timer
+    timer.waitCond.reset(new ConditionVar);
+
+    // Block until the callback is finished
+    if (std::this_thread::get_id() != worker.get_id())
+      timer.waitCond->wait(lock);
+  }
+  else
+  {
+    queue.erase(timer);
+    active.erase(i);
+
+    if (notify)
+    {
+      lock.unlock();
+      wakeUp.notify_all();
+    }
+  }
+
+  return true;
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+// TimerManager::Timer implementation
+//
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager::Timer::Timer(TimerId tid)
+  : id(tid),
+    running(false)
+{
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager::Timer::Timer(Timer&& r) noexcept
+  : id(std::move(r.id)),
+    next(std::move(r.next)),
+    period(std::move(r.period)),
+    handler(std::move(r.handler)),
+    running(std::move(r.running))
+{
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TimerManager::Timer::Timer(TimerId tid, Timestamp tnext, Duration tperiod,
+                           const TFunction& func) noexcept
+  : id(tid),
+    next(tnext),
+    period(tperiod),
+    handler(std::move(func)),
+    running(false)
+{
+}

src/common/TimerManager.hxx

@@ -0,0 +1,193 @@
+//============================================================================
+//
+//   SSSS    tt          lll  lll
+//  SS  SS   tt           ll   ll
+//  SS     tttttt  eeee   ll   ll   aaaa
+//   SSSS    tt   ee  ee  ll   ll      aa
+//      SS   tt   eeeeee  ll   ll   aaaaa  --  "An Atari 2600 VCS Emulator"
+//  SS  SS   tt   ee      ll   ll  aa  aa
+//   SSSS     ttt  eeeee llll llll  aaaaa
+//
+// Copyright (c) 1995-2018 by Bradford W. Mott, Stephen Anthony
+// and the Stella Team
+//
+// See the file "License.txt" for information on usage and redistribution of
+// this file, and for a DISCLAIMER OF ALL WARRANTIES.
+//============================================================================
+
+#ifndef TIMER_MANAGER_HXX
+#define TIMER_MANAGER_HXX
+
+#include <algorithm>
+#include <functional>
+#include <chrono>
+#include <unordered_map>
+#include <set>
+#include <cstdint>
+#include <thread>
+#include <mutex>
+#include <condition_variable>
+
+#include "bspf.hxx"
+
+/**
+  This class provides a portable periodic/one-shot timer infrastructure
+  using worker threads and generic C++11 code.
+
+  @author  Doug Gale (doug65536)
+           From "Code Review"
+           https://codereview.stackexchange.com/questions/127552/portable-periodic-one-shot-timer-thread-follow-up
+
+           Modifications and cleanup for Stella by Stephen Anthony
+*/
+class TimerManager
+{
+  public:
+    // Each Timer is assigned a unique ID of type TimerId
+    using TimerId = uInt64;
+
+    // Function object we actually use
+    using TFunction = std::function<void()>;
+
+    // Values that are a large-range millisecond count
+    using millisec = uInt64;
+
+    // Constructor does not start worker until there is a Timer.
+    explicit TimerManager();
+
+    // Destructor is thread safe, even if a timer callback is running.
+    // All callbacks are guaranteed to have returned before this
+    // destructor returns.
+    ~TimerManager();
+
+    /**
+      Create a new timer using milliseconds, and add it to the internal queue.
+
+      @param msDelay  Callback starts firing this many milliseconds from now
+      @param msPeriod If non-zero, callback is fired again after this period
+      @param func     The callback to run at the specified interval
+
+      @return  Id used to identify the timer for later use
+    */
+    TimerId addTimer(millisec msDelay, millisec msPeriod, const TFunction& func);
+
+    /**
+      Convenience function; setInterval API like browser javascript.
+
+      Call function every 'period' ms, starting 'period' ms from now.
+    */
+    TimerId setInterval(const TFunction& func, millisec period) {
+      return addTimer(period, period, std::move(func));
+    }
+
+    /**
+      Convenience function; setTimeout API like browser javascript.
+
+      Call function once 'timeout' ms from now.
+    */
+    TimerId setTimeout(const TFunction& func, millisec timeout) {
+      return addTimer(timeout, 0, std::move(func));
+    }
+
+    /**
+      Destroy the specified timer.
+
+      Synchronizes with the worker thread if the callback for this timer
+      is running, which guarantees that the handler for that callback is
+      not running before clear() returns.
+
+      You are not required to clear any timers. You can forget their
+      TimerId if you do not need to cancel them.
+
+      The only time you need this is when you want to stop a timer that
+      has a repetition period, or you want to cancel a timeout that has
+      not fired yet.
+    */
+    bool clear(TimerId id);
+
+    /**
+      Destroy all timers, but preserve id uniqueness.
+      This carefully makes sure every timer is not executing its callback
+      before destructing it.
+    */
+    void clear();
+
+    // Peek at current state
+    std::size_t size() const noexcept;
+    bool empty() const noexcept;
+
+    // Returns lazily initialized singleton
+    static TimerManager& global();
+
+  private:
+    using Lock = std::mutex;
+    using ScopedLock = std::unique_lock<Lock>;
+    using ConditionVar = std::condition_variable;
+
+    using Clock = std::chrono::steady_clock;
+    using Timestamp = std::chrono::time_point<Clock>;
+    using Duration = std::chrono::milliseconds;
+
+    struct Timer
+    {
+      explicit Timer(TimerId id = 0);
+      Timer(Timer&& r) noexcept;
+      Timer& operator=(Timer&& r) noexcept;
+
+      Timer(TimerId id, Timestamp next, Duration period, const TFunction& func) noexcept;
+
+      // Never called
+      Timer(Timer const& r) = delete;
+      Timer& operator=(Timer const& r) = delete;
+
+      TimerId id;
+      Timestamp next;
+      Duration period;
+      TFunction handler;
+
+      // You must be holding the 'sync' lock to assign waitCond
+      std::unique_ptr<ConditionVar> waitCond;
+
+      bool running;
+    };
+
+    // Comparison functor to sort the timer "queue" by Timer::next
+    struct NextActiveComparator
+    {
+      bool operator()(Timer const& a, Timer const& b) const noexcept
+      {
+        return a.next < b.next;
+      }
+    };
+
+    // Queue is a set of references to Timer objects, sorted by next
+    using QueueValue = std::reference_wrapper<Timer>;
+    using Queue = std::multiset<QueueValue, NextActiveComparator>;
+    using TimerMap = std::unordered_map<TimerId, Timer>;
+
+    void timerThreadWorker();
+    bool destroy_impl(ScopedLock& lock, TimerMap::iterator i, bool notify);
+
+    // Inexhaustible source of unique IDs
+    TimerId nextId;
+
+    // The Timer objects are physically stored in this map
+    TimerMap active;
+
+    // The ordering queue holds references to items in 'active'
+    Queue queue;
+
+    // One worker thread for an unlimited number of timers is acceptable
+    // Lazily started when first timer is started
+    // TODO: Implement auto-stopping the timer thread when it is idle for
+    // a configurable period.
+    mutable Lock sync;
+    ConditionVar wakeUp;
+    std::thread worker;
+    bool done;
+
+    // Valid IDs are guaranteed not to be this value
+    static TimerId constexpr no_timer = TimerId(0);
+};
+
+#endif // TIMERTHREAD_H

src/common/module.mk

@@ -15,6 +15,7 @@ MODULE_OBJS := \
 	src/common/RewindManager.o \
 	src/common/SoundSDL2.o \
 	src/common/StateManager.o \
+	src/common/TimerManager.o \
 	src/common/ZipHandler.o \
 	src/common/AudioQueue.o \
 	src/common/AudioSettings.o \

src/emucore/EventHandler.cxx

@@ -41,6 +41,7 @@
 #include "Settings.hxx"
 #include "Sound.hxx"
 #include "StateManager.hxx"
+#include "TimerManager.hxx"
 #include "Switches.hxx"
 #include "M6532.hxx"
 #include "MouseControl.hxx"
@@ -120,6 +121,12 @@ void EventHandler::reset(EventHandlerState state)
   setState(state);
   myOSystem.state().reset();
   myOSystem.png().setContinuousSnapInterval(0);
+
+  // Reset events almost immediately after starting emulation mode
+  // We wait a little while (0.5s), since 'hold' events may be present,
+  // and we want time for the ROM to process them
+  if(state == EventHandlerState::EMULATION)
+    myOSystem.timer().setTimeout([&ev = myEvent]() { ev.clear(); }, 500);
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

src/emucore/OSystem.cxx

@@ -50,6 +50,7 @@
 #include "Random.hxx"
 #include "SerialPort.hxx"
 #include "StateManager.hxx"
+#include "TimerManager.hxx"
 #include "Version.hxx"
 #include "TIA.hxx"
 #include "DispatchResult.hxx"
@@ -140,12 +141,13 @@ bool OSystem::create()
   myCheatManager->loadCheatDatabase();
 #endif
 
-  // Create menu and launcher GUI objects
+  // Create various subsystems (menu and launcher GUI objects, etc)
   myMenu = make_unique<Menu>(*this);
   myCommandMenu = make_unique<CommandMenu>(*this);
   myTimeMachine = make_unique<TimeMachine>(*this);
   myLauncher = make_unique<Launcher>(*this);
   myStateManager = make_unique<StateManager>(*this);
+  myTimerManager = make_unique<TimerManager>();
 
   // Create the sound object; the sound subsystem isn't actually
   // opened until needed, so this is non-blocking (on those systems

src/emucore/OSystem.hxx

@@ -37,6 +37,7 @@ class SerialPort;
 class Settings;
 class Sound;
 class StateManager;
+class TimerManager;
 class VideoDialog;
 class EmulationWorker;
 
@@ -170,6 +171,13 @@ class OSystem
     */
     StateManager& state() const { return *myStateManager; }
 
+    /**
+      Get the timer/callback manager of the system.
+
+      @return The timermanager object
+    */
+    TimerManager& timer() const { return *myTimerManager; }
+
     /**
       Get the PNG handler of the system.
 
@@ -466,6 +474,9 @@ class OSystem
     // Pointer to the StateManager object
     unique_ptr<StateManager> myStateManager;
 
+    // Pointer to the TimerManager object
+    unique_ptr<TimerManager> myTimerManager;
+
     // PNG object responsible for loading/saving PNG images
     unique_ptr<PNGLibrary> myPNGLib;