diff --git a/Sts1CobcSw/Periphery/FramRingArray.hpp b/Sts1CobcSw/Periphery/FramRingArray.hpp index c4adc549..fb0977fe 100644 --- a/Sts1CobcSw/Periphery/FramRingArray.hpp +++ b/Sts1CobcSw/Periphery/FramRingArray.hpp @@ -16,22 +16,31 @@ namespace sts1cobcsw::fram { +// TODO: Rename size to capacity +// TODO: Maybe rename RingArray to RingVector template +// TODO: require serialSize > 0, rename size to nElements class RingArray { public: + // TODO: Use serialSize instead of sizeof + // TODO: buffersize_ and offset_ can be a static constexpr member + // TODO: Why is bufferSize_ size + 1U? RingArray() : bufferSize_(size + 1U), offset_(sizeof(std::size_t) * 2) { + // TODO: We can't initialize it here because the RingArray will be a global variable Initialize(); }; auto Push(T const & newData) -> void; + // TODO: All functions returning something should be marked as [[nodiscard]] auto operator[](std::size_t index) -> T; auto Front() -> T; auto Back() -> T; + // TODO: We don't need those useless comments //! @brief Returns the current size of the ring array auto Size() -> std::size_t; diff --git a/Sts1CobcSw/Periphery/FramRingArray.ipp b/Sts1CobcSw/Periphery/FramRingArray.ipp index a76c4386..ea54819f 100644 --- a/Sts1CobcSw/Periphery/FramRingArray.ipp +++ b/Sts1CobcSw/Periphery/FramRingArray.ipp @@ -6,6 +6,11 @@ namespace sts1cobcsw::fram { +// TODO: Use an SPI timeout > 0 everywhere. (with 6 MHz SPI clock, 750 bytes take 1 ms, so that +// should be enough) +// TODO: Drop the fram:: everywhere. We are already in the fram namespace +// TODO: Rename this to PushBack(). It's not consistent with the ETL but with the STL, and a bit +// more descriptive. template void RingArray::Push(T const & newData) { @@ -13,6 +18,9 @@ void RingArray::Push(T const & newData) fram::WriteTo(fram::Address(rawAddress), Span(Serialize(newData)), 0); ++iEnd_; + // TODO: Move this check to the beginning of the function because as it is right now iEnd_ is + // never equal to iBegin_ and we loose one element. If this is at the beginning, we can use the + // full capacity of the ring. if(iEnd_ == iBegin_) { iBegin_++; @@ -25,6 +33,9 @@ void RingArray::Push(T const & newData) template auto RingArray::Front() -> T { + // TODO: This function should just be `return *this[0]`; + // TODO: Put this into a function IndexToAddress(std::size_t index) or, ToAddress(), or + // AddressOfIndex(), or AddressOf(), ... auto const rawAddress = offset_ + value_of(startAddress) + (iBegin_ * serialSize); auto readData = fram::ReadFrom>(fram::Address(rawAddress), 0); auto fromRing = Deserialize(std::span(readData)); @@ -36,6 +47,8 @@ auto RingArray::Front() -> T template auto RingArray::Back() -> T { + // TODO: Just make readIndex a cyclic_value, copy constructed from iEnd_ and decremented it + // instead of reinventing its logic here std::uint32_t readIndex = 0; if(iEnd_ == 0) { @@ -57,6 +70,9 @@ auto RingArray::Back() -> T template auto RingArray::operator[](std::size_t index) -> T { + // TODO: Again, copy iBegin_ and use .advance(index) on it + // TODO: Check if index is in bounds, i.e., index < Size(). If the index is out of bounds, we + // should DEBUG_PRINT() and do something predictable like returning the last element. auto const rawAddress = offset_ + value_of(startAddress) + ((iBegin_ + index) % bufferSize_) * serialSize; auto readData = fram::ReadFrom>(fram::Address(rawAddress), 0); @@ -73,25 +89,31 @@ auto RingArray::Size() -> std::size_t { return (iEnd_ - iBegin_); } + // TODO: If iEnd_ < iBegin_, the ring is full and (iBegin_ - iEnd_) is always 0 return (bufferSize_ - (iBegin_ - iEnd_)); } template auto RingArray::Capacity() -> std::size_t { + // TODO: This function should be a static constexpr member and return size (= capacity after + // renaming) return (bufferSize_ - 1U); } template auto RingArray::Initialize() -> void { + // TODO: We should always read the indexes from the FRAM, not just when calling Initialize() ReadIndices(); } template auto RingArray::WriteIndices() -> void { + // TODO: beginAddress and endAddress can be static constexpr members auto beginAddress = value_of(startAddress); + // TODO: Use serialSize instead of sizeof auto endAddress = beginAddress + sizeof(std::size_t); fram::WriteTo(fram::Address(beginAddress), Span(Serialize(iBegin_.get())), 0); @@ -101,7 +123,9 @@ auto RingArray::WriteIndices() -> void template auto RingArray::ReadIndices() -> void { + // TODO: beginAddress and endAddress can be static constexpr members auto beginAddress = value_of(startAddress); + // TODO: Use serialSize instead of sizeof auto endAddress = beginAddress + sizeof(std::size_t); auto beginData = fram::ReadFrom(fram::Address(beginAddress), 0);