Made corrections to TransmissionScheduler to fix some edge cases

Author: Tails86

src/coreLib/TransmissionScheduler.cpp

@@ -7,30 +7,33 @@ TransmittionScheduler::~TransmittionScheduler() {}
 
 uint32_t TransmittionScheduler::add(std::shared_ptr<Transmission> tx)
 {
-    std::list<std::shared_ptr<Transmission>>::const_iterator iter = mSchedule.cend();
-    if (iter != mSchedule.cbegin())
+    // Keep iterating until correct position is found
+    // For this to be the right position, it either needs to start and end before the next
+    // packet or starts before the next packet and is higher or same priority.
+    std::list<std::shared_ptr<Transmission>>::const_iterator iter = mSchedule.cbegin();
+    while(iter != mSchedule.cend())
     {
-        std::list<std::shared_ptr<Transmission>>::const_iterator iterNext = iter;
-        --iterNext;
-        // Keep iterating until correct position is found
-        // For this to be the right position, it either needs to start and end before the next
-        // packet or starts before the next packet and is higher or same priority.
-        while(iter != mSchedule.cbegin()
-              && ((*iterNext)->nextTxTimeUs > tx->getNextCompletionTime()
-                || (
-                    (*iterNext)->getNextCompletionTime() > tx->nextTxTimeUs
-                    && ((
-                            tx->nextTxTimeUs < (*iterNext)->nextTxTimeUs
-                            && (*iterNext)->priority == tx->priority
-                        )
-                        || (*iterNext)->priority > tx->priority)
-                )
-              )
-            )
+        // If this transmission starts before this element...
+        if (tx->nextTxTimeUs < (*iter)->nextTxTimeUs)
         {
-            --iter;
-            --iterNext;
+            // if this transmission also ends before this element starts
+            // or is of higher or same priority...
+            if (tx->getNextCompletionTime() < (*iter)->nextTxTimeUs
+                || tx->priority <= (*iter)->priority)
+            {
+                // Stop here
+                break;
+            }
         }
+        // If this transmission starts before this element completes
+        // and is higher priority...
+        else if (tx->nextTxTimeUs < (*iter)->getNextCompletionTime()
+                    && tx->priority < (*iter)->priority)
+        {
+            // Stop here
+            break;
+        }
+        ++iter;
     }
 
     mSchedule.insert(iter, tx);
@@ -87,7 +90,22 @@ std::shared_ptr<const TransmittionScheduler::Transmission> TransmittionScheduler
         }
         else
         {
-            // TODO: See if a item down the schedule could start now and end before mSchedule.begin()
+            // See if a item down the schedule could start now and end before mSchedule.begin().
+            // It's easier to check here than to rearrange schedule every time a higher priority
+            // transmission preempts a lower priority one.
+            std::list<std::shared_ptr<Transmission>>::iterator iter = mSchedule.begin();
+            ++iter;
+            while(iter != mSchedule.end())
+            {
+                if (time >= (*iter)->nextTxTimeUs
+                    && (*iter)->getNextCompletionTime() < (*mSchedule.begin())->nextTxTimeUs)
+                {
+                    item = (*iter);
+                    mSchedule.erase(iter);
+                    break;
+                }
+                ++iter;
+            }
         }
     }
     return item;

src/test/TransmissionSchedulerTests.cpp

@@ -203,10 +203,62 @@ TEST_F(TransmissionScheduleTest, multiAddBoundary2)
     EXPECT_EQ((*iter++)->transmissionId, 2);
 }
 
-class TransmissionSchedulePopTest : public TransmissionScheduleTest
+TEST_F(TransmissionScheduleTest, multiAddBoundary3)
+{
+    uint8_t priority = 255;
+    uint64_t txTime = 1;
+    MaplePacket packet1(0x11, 0x01, 0x99887766);
+    bool expectResponse = true;
+    uint32_t expectedResponseNumPayloadWords = 10;
+    uint32_t autoRepeatUs = 0;
+    uint32_t readTimeoutUs = 8675309;
+    uint32_t id1 = scheduler.add(priority,
+                    txTime,
+                    packet1,
+                    expectResponse,
+                    expectedResponseNumPayloadWords,
+                    autoRepeatUs,
+                    readTimeoutUs);
+    EXPECT_EQ(id1, 0);
+
+    txTime = 2;
+    expectedResponseNumPayloadWords = 3; // 267 us
+    MaplePacket packet2(0x22, 0x02, 0x99887766);
+    uint32_t id2 = scheduler.add(priority,
+                    txTime,
+                    packet2,
+                    expectResponse,
+                    expectedResponseNumPayloadWords,
+                    autoRepeatUs,
+                    readTimeoutUs);
+    EXPECT_EQ(id2, 1);
+
+    priority = 0;
+    txTime = 2 + 300;
+    expectedResponseNumPayloadWords = 0;
+    MaplePacket packet3(0x33, 0x02, 0x99887766);
+    uint32_t id3 = scheduler.add(priority,
+                    txTime,
+                    packet3,
+                    expectResponse,
+                    expectedResponseNumPayloadWords,
+                    autoRepeatUs,
+                    readTimeoutUs);
+    EXPECT_EQ(id3, 2);
+
+    const std::list<std::shared_ptr<TransmittionScheduler::Transmission>> schedule = scheduler.getSchedule();
+
+    ASSERT_EQ(schedule.size(), 3);
+    std::list<std::shared_ptr<TransmittionScheduler::Transmission>>::const_iterator iter = schedule.cbegin();
+    EXPECT_EQ((*iter++)->transmissionId, 2);
+    EXPECT_EQ((*iter++)->transmissionId, 0);
+    EXPECT_EQ((*iter++)->transmissionId, 1);
+}
+
+class TransmissionSchedulePopTestA : public TransmissionScheduleTest
 {
     public:
-        TransmissionSchedulePopTest() {}
+        TransmissionSchedulePopTestA() {}
 
     protected:
         virtual void SetUp()
@@ -247,7 +299,7 @@ class TransmissionSchedulePopTest : public TransmissionScheduleTest
         }
 };
 
-TEST_F(TransmissionSchedulePopTest, popTestNull)
+TEST_F(TransmissionSchedulePopTestA, popTestNull)
 {
     EXPECT_EQ(scheduler.popNext(0), nullptr);
 
@@ -260,7 +312,7 @@ TEST_F(TransmissionSchedulePopTest, popTestNull)
     EXPECT_EQ((*iter++)->transmissionId, 2);
 }
 
-TEST_F(TransmissionSchedulePopTest, popTestAutoReload1)
+TEST_F(TransmissionSchedulePopTestA, popTestAutoReload1)
 {
     std::shared_ptr<const TransmittionScheduler::Transmission> item = scheduler.popNext(1);
     ASSERT_NE(item, nullptr);
@@ -279,7 +331,7 @@ TEST_F(TransmissionSchedulePopTest, popTestAutoReload1)
     EXPECT_EQ((*iter)->nextTxTimeUs, 16002);
 }
 
-TEST_F(TransmissionSchedulePopTest, popTestAutoReload2)
+TEST_F(TransmissionSchedulePopTestA, popTestAutoReload2)
 {
     std::shared_ptr<const TransmittionScheduler::Transmission> item = scheduler.popNext(1);
     ASSERT_NE(item, nullptr);
@@ -299,7 +351,62 @@ TEST_F(TransmissionSchedulePopTest, popTestAutoReload2)
     EXPECT_EQ((*iter)->nextTxTimeUs, 32002);
 }
 
-class TransmissionScheduleCancelTest : public TransmissionSchedulePopTest
+class TransmissionSchedulePopTestB : public TransmissionScheduleTest
+{
+    public:
+        TransmissionSchedulePopTestB() {}
+
+    protected:
+        virtual void SetUp()
+        {
+            uint8_t priority = 255;
+            uint64_t txTime = 1;
+            MaplePacket packet1(0x11, 0x01, 0x99887766);
+            bool expectResponse = true;
+            uint32_t expectedResponseNumPayloadWords = 10;
+            uint32_t autoRepeatUs = 0;
+            uint32_t readTimeoutUs = 8675309;
+            scheduler.add(priority,
+                          txTime,
+                          packet1,
+                          expectResponse,
+                          expectedResponseNumPayloadWords,
+                          autoRepeatUs,
+                          readTimeoutUs);
+            priority = 0;
+            txTime = 2 + 300;
+            expectedResponseNumPayloadWords = 0;
+            MaplePacket packet3(0x22, 0x02, 0x99887766);
+            scheduler.add(priority,
+                          txTime,
+                          packet3,
+                          expectResponse,
+                          expectedResponseNumPayloadWords,
+                          autoRepeatUs,
+                          readTimeoutUs);
+            priority = 255;
+            txTime = 2;
+            expectedResponseNumPayloadWords = 3; // 267 us
+            MaplePacket packet2(0x33, 0x02, 0x99887766);
+            scheduler.add(priority,
+                          txTime,
+                          packet2,
+                          expectResponse,
+                          expectedResponseNumPayloadWords,
+                          autoRepeatUs,
+                          readTimeoutUs);
+        }
+};
+
+TEST_F(TransmissionSchedulePopTestB, popTestAutoReload2)
+{
+    // Transmission 2 should be bumped up to be executed before 0 because 0 yeileded to 1
+    std::shared_ptr<const TransmittionScheduler::Transmission> item = scheduler.popNext(2);
+    ASSERT_NE(item, nullptr);
+    EXPECT_EQ(item->transmissionId, 2);
+}
+
+class TransmissionScheduleCancelTest : public TransmissionSchedulePopTestA
 {
     public:
         TransmissionScheduleCancelTest() {}