Fix ring buffer bug (#151)

Sts1CobcSw/Edu/ProgramStatusHistory.cpp

@@ -8,20 +8,20 @@ namespace sts1cobcsw::edu
 RODOS::RingBuffer<ProgramStatusHistoryEntry, programStatusHistorySize> programStatusHistory;
 
 
-// TODO: This should probably not return a copy but a reference or pointer so that the user code can
-// modify the entry.
-auto FindProgramStatusHistoryEntry(std::uint16_t programId, std::uint16_t queueId)
-    -> ProgramStatusHistoryEntry
+auto UpdateProgramStatusHistory(std::uint16_t programId,
+                                std::uint16_t queueId,
+                                ProgramStatus newStatus) -> void
 {
-    auto programStatusHistoryEntry = ProgramStatusHistoryEntry{};
-    do
-    {
-        programStatusHistory.get(programStatusHistoryEntry);
-        // RODOS::PRINTF("%d,%d vs %d,%d\n", eduProgramStatusHistoryEntry.programId,
-        // eduProgramStatusHistoryEntry.queueId, programId, queueId);
-    } while(programStatusHistoryEntry.queueId != queueId
-            or programStatusHistoryEntry.programId != programId);
+    // TODO: Check that there is only one entry matching program/queue ID, or should it be the case
+    // by construction ?
 
-    return programStatusHistoryEntry;
+    for(std::uint32_t i = 0; i < programStatusHistory.occupiedCnt; ++i)
+    {
+        if(programStatusHistory.vals[i].queueId == queueId
+           and programStatusHistory.vals[i].programId == programId)
+        {
+            programStatusHistory.vals[i].status = newStatus;
+        }
+    }
 }
 }

Sts1CobcSw/Edu/ProgramStatusHistory.hpp

@@ -41,6 +41,7 @@ inline constexpr auto programStatusHistorySize = 20;
 extern RODOS::RingBuffer<ProgramStatusHistoryEntry, programStatusHistorySize> programStatusHistory;
 
 
-auto FindProgramStatusHistoryEntry(std::uint16_t programId, std::uint16_t queueId)
-    -> ProgramStatusHistoryEntry;
+auto UpdateProgramStatusHistory(std::uint16_t programId,
+                                std::uint16_t queueId,
+                                ProgramStatus newStatus) -> void;
 }

Sts1CobcSw/EduListenerThread.cpp

@@ -79,17 +79,19 @@ class EduListenerThread : public RODOS::StaticThread<>
                         // Program has finished
                         // Find the correspongind queueEntry and update it, then resume edu queue
                         // thread
-                        auto eduProgramStatusHistoryEntry =
-                            edu::FindProgramStatusHistoryEntry(status.programId, status.queueId);
                         if(status.exitCode == 0)
                         {
-                            eduProgramStatusHistoryEntry.status =
-                                edu::ProgramStatus::programExecutionSucceeded;
+                            edu::UpdateProgramStatusHistory(
+                                status.programId,
+                                status.queueId,
+                                edu::ProgramStatus::programExecutionSucceeded);
                         }
                         else
                         {
-                            eduProgramStatusHistoryEntry.status =
-                                edu::ProgramStatus::programExecutionFailed;
+                            edu::UpdateProgramStatusHistory(
+                                status.programId,
+                                status.queueId,
+                                edu::ProgramStatus::programExecutionFailed);
                         }
                         ResumeEduProgramQueueThread();
                         break;
@@ -122,12 +124,9 @@ class EduListenerThread : public RODOS::StaticThread<>
                         }
                         // break;
 
-                        auto eduProgramStatusHistoryEntry =
-                            edu::FindProgramStatusHistoryEntry(status.programId, status.queueId);
-                        // TODO: Pretty sure that there is a .put() or something like that missing
-                        // here and the status is actually never updated in the ring buffer.
-                        eduProgramStatusHistoryEntry.status =
-                            edu::ProgramStatus::resultFileTransfered;
+                        edu::UpdateProgramStatusHistory(status.programId,
+                                                        status.queueId,
+                                                        edu::ProgramStatus::resultFileTransfered);
                         break;
                     }
                     case edu::StatusType::invalid:

Tests/GoldenTests/CMakeLists.txt

@@ -8,8 +8,18 @@ set(Sts1CobcSw "${CMAKE_SOURCE_DIR}/Sts1CobcSw")
 add_golden_test(
     TESTFILE "HelloWorld.test.cpp" SOURCE "${Sts1CobcSw}/TopicsAndSubscribers.cpp" LIB rodos::rodos
 )
+
 add_golden_test(TESTFILE "HelloDummy.test.cpp" LIB rodos::rodos Sts1CobcSw_Dummy)
 
+add_golden_test(
+    TESTFILE
+    "UpdateRingBuffer.test.cpp"
+    LIB
+    rodos::rodos
+    Sts1CobcSw_Edu
+    Sts1CobcSw_Utility
+)
+
 add_golden_test(
     TESTFILE
     "DispatchCommand.test.cpp"

Tests/GoldenTests/ExpectedOutputs/UpdateRingBuffer.txt

@@ -0,0 +1,13 @@
+Vals[0] = .id(1), .status(programExecutionFailed)
+Vals[1] = .id(2), .status(programRunning)
+Vals[2] = .id(3), .status(programRunning)
+Vals[3] = .id(4), .status(programRunning)
+readCnt unchanged     : 1
+writeCnt unchanged    : 0
+OccupiedCnt unchanged : 0
+Vals[0] = .id(1), .status(programExecutionFailed)
+Vals[1] = .id(2), .status(programExecutionSucceeded)
+Vals[2] = .id(3), .status(programRunning)
+Vals[3] = .id(4), .status(programExecutionFailed)
+Vals[4] = .id(5), .status(programExecutionSucceeded)
+hw_resetAndReboot() -> exit

Tests/GoldenTests/UpdateRingBuffer.test.cpp

@@ -0,0 +1,94 @@
+#include <Sts1CobcSw/Dummy.hpp>
+#include <Sts1CobcSw/Edu/ProgramStatusHistory.hpp>
+
+#include <type_safe/types.hpp>
+
+#include <rodos/support/support-libs/ringbuffer.h>
+#include <rodos_no_using_namespace.h>
+
+#include <cstdint>
+#include <string_view>
+
+
+std::uint32_t printfMask = 0;
+
+
+namespace sts1cobcsw
+{
+auto ToString(edu::ProgramStatus status) -> std::string_view
+{
+    switch(status)
+    {
+        case edu::ProgramStatus::programRunning:
+            return "programRunning";
+        case edu::ProgramStatus::programExecutionFailed:
+            return "programExecutionFailed";
+        case edu::ProgramStatus::programExecutionSucceeded:
+            return "programExecutionSucceeded";
+        default:
+            return "";
+    }
+}
+
+
+// Helper function for edu::programStatusHistory
+void PrintBuffer()
+{
+    for(std::uint32_t i = 0; i < edu::programStatusHistory.occupiedCnt; ++i)
+    {
+        RODOS::PRINTF("Vals[%d] = .id(%d), .status(%s)\n",
+                      i,
+                      edu::programStatusHistory.vals[i].programId.get(),
+                      ToString(edu::programStatusHistory.vals[i].status).data());
+    }
+}
+
+
+class UpdateRingBufferTest : public RODOS::StaticThread<>
+{
+    void run() override
+    {
+        using type_safe::operator""_u16;
+
+        printfMask = 1;
+
+        edu::programStatusHistory.put(
+            edu::ProgramStatusHistoryEntry{.programId = 1_u16,
+                                           .queueId = 1_u16,
+                                           .status = edu::ProgramStatus::programExecutionFailed});
+        edu::programStatusHistory.put(edu::ProgramStatusHistoryEntry{
+            .programId = 2_u16, .queueId = 1_u16, .status = edu::ProgramStatus::programRunning});
+        edu::programStatusHistory.put(edu::ProgramStatusHistoryEntry{
+            .programId = 3_u16, .queueId = 1_u16, .status = edu::ProgramStatus::programRunning});
+        edu::programStatusHistory.put(edu::ProgramStatusHistoryEntry{
+            .programId = 4_u16, .queueId = 1_u16, .status = edu::ProgramStatus::programRunning});
+
+
+        auto readCnt = edu::programStatusHistory.readCnt;
+        auto writeCnt = edu::programStatusHistory.writeCnt;
+        auto occupiedCnt = edu::programStatusHistory.occupiedCnt;
+
+        PrintBuffer();
+
+        edu::UpdateProgramStatusHistory(2, 1, edu::ProgramStatus::programExecutionSucceeded);
+        edu::UpdateProgramStatusHistory(4, 1, edu::ProgramStatus::programExecutionFailed);
+        edu::programStatusHistory.put(edu::ProgramStatusHistoryEntry{
+            .programId = 5_u16, .queueId = 1_u16, .status = edu::ProgramStatus::programRunning});
+        edu::UpdateProgramStatusHistory(5, 1, edu::ProgramStatus::programExecutionSucceeded);
+
+        // 1, because we did not read anything
+        RODOS::PRINTF("readCnt unchanged     : %d\n",
+                      static_cast<int>(edu::programStatusHistory.readCnt == readCnt));
+        // 0, because we did write
+        RODOS::PRINTF("writeCnt unchanged    : %d\n",
+                      static_cast<int>(edu::programStatusHistory.readCnt == writeCnt));
+        // 0
+        RODOS::PRINTF("OccupiedCnt unchanged : %d\n",
+                      static_cast<int>(edu::programStatusHistory.occupiedCnt == occupiedCnt));
+
+        PrintBuffer();
+
+        RODOS::hwResetAndReboot();
+    }
+} updateRingBufferTest;
+}