Fix thread-safety of commands.

Author: Mysticial

SerialPrograms/Source/Controllers/SuperscalarScheduler.cpp

@@ -29,7 +29,7 @@ SuperscalarScheduler::SuperscalarScheduler(
 
 
 void SuperscalarScheduler::clear() noexcept{
-    SpinLockGuard lg(m_lock);
+//    SpinLockGuard lg(m_lock);
     m_logger.log("Clearing schedule...");
     WallClock now = current_time();
     m_local_start = now;
@@ -54,54 +54,49 @@ void SuperscalarScheduler::clear() noexcept{
 bool SuperscalarScheduler::iterate_schedule(const Cancellable* cancellable){
     cancellable->throw_if_cancelled();
 
-    WallDuration duration;
-    {
-        SpinLockGuard lg(m_lock);
-
-        if (m_state_changes.empty()){
-            m_device_sent_time = m_device_issue_time;
-            return false;
-        }
+    if (m_state_changes.empty()){
+        m_device_sent_time = m_device_issue_time;
+        return false;
+    }
 
-        auto iter = m_state_changes.begin();
-
-        WallClock next_state_change;
-        if (m_device_sent_time < *iter){
-            next_state_change = *iter;
-        }else{
-            auto next = iter;
-            ++next;
-            next_state_change = next == m_state_changes.end()
-                ? m_device_issue_time
-                : *next;
-        }
+    auto iter = m_state_changes.begin();
+
+    WallClock next_state_change;
+    if (m_device_sent_time < *iter){
+        next_state_change = *iter;
+    }else{
+        auto next = iter;
+        ++next;
+        next_state_change = next == m_state_changes.end()
+            ? m_device_issue_time
+            : *next;
+    }
 
-        //  Things get complicated if we overshoot the issue time.
-        if (next_state_change > m_device_issue_time){
+    //  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);
-            next_state_change = m_device_issue_time;
-        }
+        next_state_change = m_device_issue_time;
+    }
 
-        duration = next_state_change - m_device_sent_time;
-        if (duration == WallDuration::zero()){
+    WallDuration duration = next_state_change - m_device_sent_time;
+    if (duration == WallDuration::zero()){
 //            m_logger.log("SuperscalarScheduler: Scheduler is stalled.", COLOR_RED);
-            return false;
-        }
+        return false;
+    }
 
-        //  Compute the resource state at this timestamp.
-        for (ExecutionResource* resource : m_resources){
-            resource->m_is_busy = resource->m_busy_time <= m_device_sent_time && m_device_sent_time < resource->m_done_time;
-        }
+    //  Compute the resource state at this timestamp.
+    for (ExecutionResource* resource : m_resources){
+        resource->m_is_busy = resource->m_busy_time <= m_device_sent_time && m_device_sent_time < resource->m_done_time;
+    }
 
-        m_device_sent_time = next_state_change;
-        if (next_state_change >= *iter){
-            m_state_changes.erase(iter);
-        }
+    m_device_sent_time = next_state_change;
+    if (next_state_change >= *iter){
+        m_state_changes.erase(iter);
     }
 
     this->push_state(cancellable, duration);
 
-    SpinLockGuard lg(m_lock);
+//    SpinLockGuard lg(m_lock);
 
     WallClock now = current_time();
     m_local_last_activity = now;
@@ -142,16 +137,13 @@ void SuperscalarScheduler::issue_wait_for_all(const Cancellable* cancellable){
         clear();
         return;
     }
-    {
-        SpinLockGuard lg(m_lock);
-//        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()
-//             << ", max_free_time = " << std::chrono::duration_cast<Milliseconds>((m_max_free_time - m_local_start)).count()
-//             << endl;
-        m_device_issue_time = std::max(m_device_issue_time, m_max_free_time);
-        m_max_free_time = m_device_issue_time;
-    }
+//    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()
+//         << ", max_free_time = " << std::chrono::duration_cast<Milliseconds>((m_max_free_time - m_local_start)).count()
+//         << endl;
+    m_device_issue_time = std::max(m_device_issue_time, m_max_free_time);
+    m_max_free_time = m_device_issue_time;
     process_schedule(cancellable);
 }
 void SuperscalarScheduler::issue_nop(const Cancellable* cancellable, WallDuration delay){
@@ -161,18 +153,15 @@ void SuperscalarScheduler::issue_nop(const Cancellable* cancellable, WallDuratio
     if (m_pending_clear.load(std::memory_order_acquire)){
         clear();
     }
-    {
-        SpinLockGuard lg(m_lock);
-//        cout << "issue_nop(): " << 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()
-//             << ", max_free_time = " << std::chrono::duration_cast<Milliseconds>((m_max_free_time - m_local_start)).count()
-//             << endl;
-        m_state_changes.insert(m_device_issue_time);
-        m_device_issue_time += delay;
-        m_max_free_time = std::max(m_max_free_time, m_device_issue_time);
-        m_local_last_activity = current_time();
-    }
+//    cout << "issue_nop(): " << 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()
+//         << ", max_free_time = " << std::chrono::duration_cast<Milliseconds>((m_max_free_time - m_local_start)).count()
+//         << endl;
+    m_state_changes.insert(m_device_issue_time);
+    m_device_issue_time += delay;
+    m_max_free_time = std::max(m_max_free_time, m_device_issue_time);
+    m_local_last_activity = current_time();
     process_schedule(cancellable);
 }
 void SuperscalarScheduler::wait_for_resource(const Cancellable* cancellable, ExecutionResource& resource){
@@ -185,14 +174,11 @@ void SuperscalarScheduler::wait_for_resource(const Cancellable* cancellable, Exe
 //         << ", sent_time = " << std::chrono::duration_cast<Milliseconds>((m_device_sent_time - m_local_start)).count()
 //         << ", free_time = " << std::chrono::duration_cast<Milliseconds>((resource.m_free_time - m_local_start)).count()
 //         << endl;
-    {
-        SpinLockGuard lg(m_lock);
-        //  Resource is not ready yet. Stall until it is.
-        if (resource.m_free_time > m_device_sent_time){
-//            cout << "stall = " << std::chrono::duration_cast<Milliseconds>(resource.m_free_time - m_device_sent_time).count() << endl;
-            m_device_issue_time = resource.m_free_time;
-            m_local_last_activity = current_time();
-        }
+    //  Resource is not ready yet. Stall until it is.
+    if (resource.m_free_time > m_device_sent_time){
+//        cout << "stall = " << std::chrono::duration_cast<Milliseconds>(resource.m_free_time - m_device_sent_time).count() << endl;
+        m_device_issue_time = resource.m_free_time;
+        m_local_last_activity = current_time();
     }
     process_schedule(cancellable);
 }
@@ -204,39 +190,36 @@ void SuperscalarScheduler::issue_to_resource(
         clear();
     }
 
-    {
-        SpinLockGuard lg(m_lock);
-        //  Resource is busy.
-        if (m_device_sent_time < resource.m_free_time){
-            throw InternalProgramError(
-                nullptr, PA_CURRENT_FUNCTION,
-                "Attempted to issue resource that isn't ready."
-            );
-        }
+    //  Resource is busy.
+    if (m_device_sent_time < resource.m_free_time){
+        throw InternalProgramError(
+            nullptr, PA_CURRENT_FUNCTION,
+            "Attempted to issue resource that isn't ready."
+        );
+    }
 
-//        cout << "issue_to_resource(): delay = " << std::chrono::duration_cast<Milliseconds>(delay).count()
-//             << ", hold = " << std::chrono::duration_cast<Milliseconds>(hold).count()
-//             << ", cooldown = " << std::chrono::duration_cast<Milliseconds>(cooldown).count()
-//             << 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()
-//             << endl;
+//    cout << "issue_to_resource(): delay = " << std::chrono::duration_cast<Milliseconds>(delay).count()
+//         << ", hold = " << std::chrono::duration_cast<Milliseconds>(hold).count()
+//         << ", cooldown = " << std::chrono::duration_cast<Milliseconds>(cooldown).count()
+//         << 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()
+//         << endl;
 
-        WallClock release_time = m_device_issue_time + hold;
-        WallClock free_time = release_time + cooldown;
+    WallClock release_time = m_device_issue_time + hold;
+    WallClock free_time = release_time + cooldown;
 
-        m_state_changes.insert(m_device_issue_time);
-        m_state_changes.insert(release_time);
+    m_state_changes.insert(m_device_issue_time);
+    m_state_changes.insert(release_time);
 
-        resource.m_busy_time = m_device_issue_time;
-        resource.m_done_time = release_time;
-        resource.m_free_time = free_time;
+    resource.m_busy_time = m_device_issue_time;
+    resource.m_done_time = release_time;
+    resource.m_free_time = free_time;
 
-        m_device_issue_time += delay;
-        m_max_free_time = std::max(m_max_free_time, free_time);
-        m_max_free_time = std::max(m_max_free_time, m_device_issue_time);
-        m_local_last_activity = current_time();
-    }
+    m_device_issue_time += delay;
+    m_max_free_time = std::max(m_max_free_time, free_time);
+    m_max_free_time = std::max(m_max_free_time, m_device_issue_time);
+    m_local_last_activity = current_time();
 
     process_schedule(cancellable);
 }

SerialPrograms/Source/Controllers/SuperscalarScheduler.h

@@ -11,7 +11,6 @@
 #include <atomic>
 #include "Common/Cpp/AbstractLogger.h"
 #include "Common/Cpp/CancellableScope.h"
-#include "Common/Cpp/Concurrency/SpinLock.h"
 
 namespace PokemonAutomation{
 
@@ -72,7 +71,7 @@ class SuperscalarScheduler{
     //  returned to the ready state.
     void issue_wait_for_all(const Cancellable* cancellable);
 
-    //  Issue a no-nothing command for the specified delay.
+    //  Issue a do-nothing command for the specified delay.
     //  This will advance the issue timestamp.
     void issue_nop(const Cancellable* cancellable, WallDuration delay);
 
@@ -107,8 +106,6 @@ class SuperscalarScheduler{
 
     std::atomic<bool> m_pending_clear;
 
-    SpinLock m_lock;
-
     //  The construction time of this object. This is only used for debugging
     //  purposes since it lets you print wall times relative to this.
     WallClock m_local_start;

SerialPrograms/Source/NintendoSwitch/Controllers/NintendoSwitch_SerialPABotBase.cpp

@@ -231,7 +231,10 @@ void SwitchController_SerialPABotBase::update_status_string(){
 
 void SwitchController_SerialPABotBase::wait_for_all(const Cancellable* cancellable){
 //    cout << "wait_for_all() - enter" << endl;
-    this->issue_wait_for_all(cancellable);
+    {
+        WriteSpinLock lg(m_lock);
+        this->issue_wait_for_all(cancellable);
+    }
     m_serial.wait_for_all_requests(cancellable);
 //    cout << "wait_for_all() - exit" << endl;
 }
@@ -279,6 +282,7 @@ void SwitchController_SerialPABotBase::issue_barrier(const Cancellable* cancella
     if (m_logging_suppress.load(std::memory_order_relaxed) == 0){
         m_logger.log("issue_barrier()", COLOR_DARKGREEN);
     }
+    WriteSpinLock lg(m_lock);
     this->issue_wait_for_all(cancellable);
 }
 void SwitchController_SerialPABotBase::issue_nop(const Cancellable* cancellable, Milliseconds duration){
@@ -291,6 +295,7 @@ void SwitchController_SerialPABotBase::issue_nop(const Cancellable* cancellable,
             COLOR_DARKGREEN
         );
     }
+    WriteSpinLock lg(m_lock);
     this->SuperscalarScheduler::issue_nop(cancellable, WallDuration(duration));
 }
 void SwitchController_SerialPABotBase::issue_buttons(
@@ -310,6 +315,7 @@ void SwitchController_SerialPABotBase::issue_buttons(
             COLOR_DARKGREEN
         );
     }
+    WriteSpinLock lg(m_lock);
     for (size_t c = 0; c < 14; c++){
         uint16_t mask = (uint16_t)1 << c;
         if (button & mask){
@@ -325,7 +331,7 @@ void SwitchController_SerialPABotBase::issue_buttons(
             );
         }
     }
-    this->issue_nop(cancellable, delay);
+    this->SuperscalarScheduler::issue_nop(cancellable, delay);
 }
 void SwitchController_SerialPABotBase::issue_dpad(
     const Cancellable* cancellable,
@@ -344,6 +350,7 @@ void SwitchController_SerialPABotBase::issue_dpad(
             COLOR_DARKGREEN
         );
     }
+    WriteSpinLock lg(m_lock);
     this->wait_for_resource(cancellable, m_dpad);
     m_dpad.position = position;
     this->issue_to_resource(cancellable, m_dpad, delay, hold, cooldown);
@@ -365,6 +372,7 @@ void SwitchController_SerialPABotBase::issue_left_joystick(
             COLOR_DARKGREEN
         );
     }
+    WriteSpinLock lg(m_lock);
     this->wait_for_resource(cancellable, m_left_joystick);
     m_left_joystick.x = x;
     m_left_joystick.y = y;
@@ -388,6 +396,7 @@ void SwitchController_SerialPABotBase::issue_right_joystick(
             COLOR_DARKGREEN
         );
     }
+    WriteSpinLock lg(m_lock);
     this->wait_for_resource(cancellable, m_right_joystick);
     m_right_joystick.x = x;
     m_right_joystick.y = y;

SerialPrograms/Source/NintendoSwitch/Controllers/NintendoSwitch_SerialPABotBase.h

@@ -212,6 +212,7 @@ class SwitchController_SerialPABotBase :
     BotBaseController& m_serial;
 
     std::atomic<size_t> m_logging_suppress;
+    SpinLock m_lock;
 };