Minor refactor of controllers.

1. Separate out a public controller-state function that properly goes through the command queue.
2. Split the lock inside ProControllerWithScheduler to remove some async uncertainties on the cancellation paths.
3. Make the controller implementations directly implement "push_state()".

Author: Mysticial

Common/Cpp/Concurrency/ReverseLockGuard.h

@@ -0,0 +1,40 @@
+/*  Reverse Lock Guard
+ *
+ *  From: https://github.com/PokemonAutomation/Arduino-Source
+ *
+ */
+
+#ifndef PokemonAutomation_ReverseLockGuard_H
+#define PokemonAutomation_ReverseLockGuard_H
+
+#include "Common/Cpp/Exceptions.h"
+
+namespace PokemonAutomation{
+
+
+template <class Mutex>
+class ReverseLockGuard{
+public:
+    ReverseLockGuard(Mutex& lock)
+        : m_lock(lock)
+    {
+        if (lock.try_lock()){
+            throw InternalProgramError(
+                nullptr, PA_CURRENT_FUNCTION,
+                "Attempted to unlock an already unlocked lock."
+            );
+        }
+        lock.unlock();
+    }
+    ~ReverseLockGuard(){
+        m_lock.lock();
+    }
+
+
+private:
+    Mutex& m_lock;
+};
+
+
+}
+#endif

SerialPrograms/CMakeLists.txt

@@ -74,6 +74,7 @@ file(GLOB MAIN_SOURCES
     ../Common/Cpp/Concurrency/ParallelTaskRunner.h
     ../Common/Cpp/Concurrency/PeriodicScheduler.cpp
     ../Common/Cpp/Concurrency/PeriodicScheduler.h
+    ../Common/Cpp/Concurrency/ReverseLockGuard.h
     ../Common/Cpp/Concurrency/ScheduledTaskRunner.cpp
     ../Common/Cpp/Concurrency/ScheduledTaskRunner.h
     ../Common/Cpp/Concurrency/SpinLock.cpp

SerialPrograms/SerialPrograms.pro

@@ -1148,6 +1148,7 @@ HEADERS += \
     ../Common/Cpp/Concurrency/FireForgetDispatcher.h \
     ../Common/Cpp/Concurrency/ParallelTaskRunner.h \
     ../Common/Cpp/Concurrency/PeriodicScheduler.h \
+    ../Common/Cpp/Concurrency/ReverseLockGuard.h \
     ../Common/Cpp/Concurrency/ScheduledTaskRunner.h \
     ../Common/Cpp/Concurrency/SpinLock.h \
     ../Common/Cpp/Concurrency/SpinPause.h \

SerialPrograms/Source/Controllers/SuperscalarScheduler.cpp

@@ -30,7 +30,7 @@ SuperscalarScheduler::SuperscalarScheduler(
 
 void SuperscalarScheduler::clear() noexcept{
 //    SpinLockGuard lg(m_lock);
-    m_logger.log("Clearing schedule...");
+//    m_logger.log("Clearing schedule...");
     WallClock now = current_time();
     m_local_start = now;
     m_local_last_activity = now;
@@ -77,13 +77,17 @@ bool SuperscalarScheduler::iterate_schedule(const Cancellable* cancellable){
 
     //  Things get complicated if we overshoot the issue time.
     if (next_state_change > m_device_issue_time){
-//            m_logger.log("SuperscalarScheduler: Overshoot due to unfinished early return.", COLOR_RED);
+//        m_logger.log("SuperscalarScheduler: Overshoot due to unfinished early return.", COLOR_RED);
         next_state_change = m_device_issue_time;
     }
 
     WallDuration duration = next_state_change - m_device_sent_time;
     if (duration == WallDuration::zero()){
-//            m_logger.log("SuperscalarScheduler: Scheduler is stalled.", COLOR_RED);
+        //  This happens when the command has a delay of zero. A delay of zero
+        //  is almost always immediately followed by another command that is
+        //  intended to execute simultaneously. Thus we do not attempt to
+        //  execute the schedule any further.
+//        m_logger.log("SuperscalarScheduler: Scheduler is stalled.", COLOR_RED);
         return false;
     }
 
@@ -104,22 +108,35 @@ bool SuperscalarScheduler::iterate_schedule(const Cancellable* cancellable){
     WallClock now = current_time();
     m_local_last_activity = now;
 
-#if 1
-    //  If it's been long enough since the issue, we can assume a gap and
-    //  fast forward whatever is scheduled into the future.
-    if (m_device_sent_time < m_device_issue_time){
-        WallDuration time_since_last_activity = now - m_local_last_activity;
-        if (time_since_last_activity > m_flush_threshold){
-            m_logger.log("SuperscalarScheduler: A dangling early-return issue has gapped.", COLOR_RED);
-            m_device_issue_time += time_since_last_activity;
-
-            //  Since we're gapping, we might as well gap a bit more to we don't
-            //  re-enter here so quickly.
-            m_device_issue_time += m_flush_threshold;
+    //  If we are not dangling anything, we can return now.
+    if (m_device_sent_time >= m_device_issue_time){
+        return true;
+    }
 
-            m_max_free_time = std::max(m_max_free_time, m_device_issue_time);
-        }
+#if 1
+    //
+    //  We are currently dangling a command. We don't know whether the current
+    //  ongoing commands will run out, or if something new will join them in the
+    //  near future.
+    //
+    //  If it's been long enough since the last issue, we can assume that
+    //  nothing will join and thus can gap. (We do require that dangling
+    //  commands be resolved promptly or they may run out.)
+    //
+    //  Thus we can fast forward whatever is scheduled into the future.
+    //
+    WallDuration time_since_last_activity = now - m_local_last_activity;
+    if (time_since_last_activity > m_flush_threshold){
+        m_logger.log("SuperscalarScheduler: A dangling early-return issue has gapped.", COLOR_RED);
+        m_device_issue_time += time_since_last_activity;
+
+        //  Since we're gapping, we might as well gap a bit more to we don't
+        //  re-enter here so quickly.
+        m_device_issue_time += m_flush_threshold;
+
+        m_max_free_time = std::max(m_max_free_time, m_device_issue_time);
     }
+
 #endif
 
     return true;
@@ -140,6 +157,7 @@ void SuperscalarScheduler::issue_wait_for_all(const Cancellable* cancellable){
         clear();
         return;
     }
+//    m_logger.log("issue_wait_for_all(): states = " + std::to_string(m_state_changes.size()), COLOR_DARKGREEN);
 //    cout << "issue_wait_for_all(): " << m_state_changes.size() << endl;
 //    cout << "issue_time = " << std::chrono::duration_cast<Milliseconds>((m_device_issue_time - m_local_start)).count()
 //         << ", sent_time = " << std::chrono::duration_cast<Milliseconds>((m_device_sent_time - m_local_start)).count()
@@ -168,7 +186,7 @@ void SuperscalarScheduler::issue_nop(const Cancellable* cancellable, WallDuratio
     m_local_last_activity = current_time();
     process_schedule(cancellable);
 }
-void SuperscalarScheduler::wait_for_resource(const Cancellable* cancellable, ExecutionResource& resource){
+void SuperscalarScheduler::issue_wait_for_resource(const Cancellable* cancellable, ExecutionResource& resource){
     if (m_pending_clear.load(std::memory_order_acquire)){
         clear();
         return;

SerialPrograms/Source/Controllers/SuperscalarScheduler.h

@@ -64,8 +64,11 @@ class SuperscalarScheduler{
 
 
 public:
-    //  These are the standard commands.
-    //  They are not thread-safe with each other.
+    //  These are the standard "issue" commands.
+    //
+    //  These functions may or may not block.
+    //  These are not thread-safe with each other.
+    //
 
     //  Wait until the pipeline has completely cleared and all resources have
     //  returned to the ready state.
@@ -77,7 +80,7 @@ class SuperscalarScheduler{
 
     //  Wait until the specified resource is ready to be used.
     //  This will advance the issue timestamp until the resource is ready.
-    void wait_for_resource(const Cancellable* cancellable, ExecutionResource& resource);
+    void issue_wait_for_resource(const Cancellable* cancellable, ExecutionResource& resource);
 
     //  Issue a resource with the specified timing parameters.
     //  The resource must be ready to be used.
@@ -88,6 +91,17 @@ class SuperscalarScheduler{
 
 
 protected:
+    //
+    //  This class will automatically call "push_state()" when a state is ready.
+    //  The child class should send/queue this state to the device/Switch.
+    //
+    //  This function is allowed to block. In other words, it can wait until
+    //  there is space in the queue before returning.
+    //
+    //  This will always be called inside one of the above "issue_XXX()"
+    //  functions. Implementations of this method should be aware of this
+    //  re-entrancy when this gets called on them with respect to locking.
+    //
     virtual void push_state(const Cancellable* cancellable, WallDuration duration) = 0;
 
 

SerialPrograms/Source/NintendoSwitch/Commands/NintendoSwitch_Commands_PushButtons.cpp

@@ -91,7 +91,7 @@ void pbf_controller_state(
     uint8_t right_x, uint8_t right_y,
     uint16_t ticks
 ){
-    context->issue_controller_state(
+    context->issue_full_controller_state(
         &context,
         button, position,
         left_x, left_y,
@@ -107,7 +107,7 @@ void pbf_controller_state(
     uint8_t right_x, uint8_t right_y,
     Milliseconds duration
 ){
-    context->issue_controller_state(
+    context->issue_full_controller_state(
         &context,
         button, position,
         left_x, left_y,

SerialPrograms/Source/NintendoSwitch/Controllers/NintendoSwitch_ProController.cpp

@@ -72,6 +72,7 @@ class ProController::KeyboardManager : public KeyboardInputController{
     }
     virtual void send_state(const ControllerState& state) override{
         const SwitchControllerState& switch_state = static_cast<const SwitchControllerState&>(state);
+#if 0
         m_controller.logger().log(
             "VirtualController: (" + button_to_string(switch_state.buttons) +
             "), dpad(" + dpad_to_string(switch_state.dpad) +
@@ -80,7 +81,8 @@ class ProController::KeyboardManager : public KeyboardInputController{
             ")",
             COLOR_DARKGREEN
         );
-        m_controller.issue_controller_state(
+#endif
+        m_controller.issue_full_controller_state(
             nullptr,
             switch_state.buttons,
             switch_state.dpad,

SerialPrograms/Source/NintendoSwitch/Controllers/NintendoSwitch_ProController.h

@@ -88,10 +88,10 @@ class ProController : public AbstractController{
 
 
 public:
-    //  General Control
-
-    //  Wait for all unfinished commands to finish.
-    virtual void wait_for_all(const Cancellable* cancellable) = 0;
+    //
+    //  Cancellation
+    //  These are thread-safe with everything.
+    //
 
     //  Cancel all commands. This returns the controller to the neutral button
     //  state and clears the command queue.
@@ -103,7 +103,21 @@ class ProController : public AbstractController{
 
 
 public:
+    //
     //  Basic Commands
+    //
+    //  Commands not thread-safe with other commands. But they are thread-safe
+    //  with the cancellation functions above.
+    //
+    //  As of this writing, all implementations are thread-safe enough that
+    //  they will neither crash nor enter an invalid state with concurrent
+    //  commands. But the implied queuing semantics means that parallelizing
+    //  commands will not do what you want it to do.
+    //
+
+    //  Wait for all unfinished commands to finish. This will also wait out
+    //  hanging commands including their cooldown periods.
+    virtual void wait_for_all(const Cancellable* cancellable) = 0;
 
     //
     //  All commands are enqueued into a FIFO that the controller will execute
@@ -125,36 +139,6 @@ class ProController : public AbstractController{
     //  if the command can be enqueued into the FIFO. Otherwise, they will block
     //  until there is space in the FIFO.
 
-    //
-    //  Press all the following buttons/joysticks simultaneously for the
-    //  specified duration. No wait is added at the end. Thus you can issue
-    //  these back-to-back to simulate buttons being pressed and released
-    //  concurrently with other buttons being held down the whole time.
-    //
-    //  The behavior of this function is undefined if there are any unfinished
-    //  asynchronous commands in the controller's queue (including unfinished
-    //  cooldowns). It is the responsibility of the caller to ensure the
-    //  controller is idle by calling "this->wait_for_all_requests()".
-    //
-    //  The sole purpose of this function is for keyboard commands.
-    //  While it's technically possible to implement any button overlapping
-    //  sequence with this, doing so this way can lead to very inefficient
-    //  serial bandwidth usage if buttons are being rapidly pressed and released
-    //  in an arbitrary manner that leads to constant state changes.
-    //
-    //  If we need to support new Switch controller functionality
-    //  (such as Joycon gyro or new stuff in Switch 2), we can simply add
-    //  overloads to this and gate them behind features.
-    //
-    virtual void issue_controller_state(
-        const Cancellable* cancellable,
-        Button button,
-        DpadPosition position,
-        uint8_t left_x, uint8_t left_y,
-        uint8_t right_x, uint8_t right_y,
-        Milliseconds milliseconds
-    ) = 0;
-
     //  Temporary for refactor: Send custom requests for PABotBase's advanced
     //  RPCs.
     virtual void send_botbase_request(
@@ -168,16 +152,9 @@ class ProController : public AbstractController{
 
 
 public:
+    //
     //  Superscalar Commands (the "ssf" framework)
-
-    //  Tell the scheduler to wait for all pending commands to finish
-    //  (including cooldowns) before executing further instructions.
-    //  This is used to prevent hanging commands from overlapping with new
-    //  commands issued after this barrier.
-    virtual void issue_barrier(const Cancellable* cancellable) = 0;
-
-    //  Do nothing for this much time.
-    virtual void issue_nop(const Cancellable* cancellable, Milliseconds duration) = 0;
+    //
 
     //
     //  delay       Time to wait before moving onto the next command.
@@ -207,6 +184,15 @@ class ProController : public AbstractController{
     //  managing the timeline/scheduling.
     //
 
+    //  Tell the scheduler to wait for all pending commands to finish
+    //  (including cooldowns) before executing further instructions.
+    //  This is used to prevent hanging commands from overlapping with new
+    //  commands issued after this barrier.
+    virtual void issue_barrier(const Cancellable* cancellable) = 0;
+
+    //  Do nothing for this much time.
+    virtual void issue_nop(const Cancellable* cancellable, Milliseconds duration) = 0;
+
     //  Press all the buttons set in the bitfield simultaneously.
     //  This command will wait until all the selected buttons are ready to
     //  ensure that they are all dispatched simultaneously.
@@ -235,10 +221,39 @@ class ProController : public AbstractController{
         Milliseconds delay, Milliseconds hold, Milliseconds cooldown
     ) = 0;
 
+    //
+    //  Press all the following buttons/joysticks simultaneously for the
+    //  specified duration. No wait is added at the end. Thus you can issue
+    //  these back-to-back to simulate buttons being pressed and released
+    //  concurrently with other buttons being held down the whole time.
+    //
+    //  This command will wait until the controller is fully idle (including
+    //  cooldowns) before it starts. This ensures that everything is issued
+    //  simultaneously.
+    //
+    //  The sole purpose of this function is for keyboard commands.
+    //  While it's technically possible to implement any button overlapping
+    //  sequence with this, doing so this way can lead to very inefficient
+    //  serial bandwidth usage if buttons are being rapidly pressed and released
+    //  in an arbitrary manner that leads to constant state changes.
+    //
+    //  If we need to support new Switch controller functionality
+    //  (such as Joycon gyro or new stuff in Switch 2), we can simply add
+    //  overloads to this and gate them behind features.
+    //
+    virtual void issue_full_controller_state(
+        const Cancellable* cancellable,
+        Button button,
+        DpadPosition position,
+        uint8_t left_x, uint8_t left_y,
+        uint8_t right_x, uint8_t right_y,
+        Milliseconds hold
+    ) = 0;
+
 
 public:
+    //
     //  High speed Macros
-
     //
     //  It is currently unclear if these can be properly executed over wireless.
     //  If they can't, then it remains to be decided if we should gate these