Isobuffer refactor - Part 3 (#66)

* Start moving towards cstdint types. Change m_bufferEnd to m_bufferLen (i.e. dont substract 1 when constructing), this fixes a bunch of small bugs and off by one errors

* Fix loop in isoBuffer::getDelayedTriggerPoint not touching the first element

* Comments, formatting

* Reorder some of isobuffer.cpp's functions

* Fix compile error caused by not fully renaming a variable

* Fix some off-by-one errors

* formatting

* Tabs -> Spaces

Desktop_Interface/i2cdecoder.cpp

@@ -45,15 +45,15 @@ void i2cDecoder::run()
 		updateBitValues();
 		runStateMachine();
 		serialPtr_bit ++;
-        if (serialPtr_bit > (sda->m_bufferEnd * 8))
+        if (serialPtr_bit >= (sda->m_bufferLen * 8))
-            serialPtr_bit -= (sda->m_bufferEnd * 8);
+            serialPtr_bit -= (sda->m_bufferLen * 8);
 	}	
 } 
 
 int i2cDecoder::serialDistance(isoBuffer* buffer)
 {
     int back_bit = buffer->m_back * 8;
-    int bufferEnd_bit = buffer->m_bufferEnd * 8;
+    int bufferEnd_bit = buffer->m_bufferLen * 8;
     if (back_bit >= serialPtr_bit)
         return back_bit - serialPtr_bit;
     else

Desktop_Interface/isobuffer.cpp

@@ -7,50 +7,50 @@
 
 namespace
 {
-	constexpr char const* fileHeaderFormat =
+constexpr char const* fileHeaderFormat =
     "EspoTek Labrador DAQ V1.0 Output File\n"
     "Averaging = %d\n"
     "Mode = %d\n";
 
-	constexpr auto kSamplesSeekingCap = 20;
+constexpr auto kSamplesSeekingCap = 20;
 
-	#ifdef INVERT_MM
+#ifdef INVERT_MM
-	constexpr auto fX0Comp = std::greater<int> {};
+constexpr auto fX0Comp = std::greater<int> {};
-	constexpr auto fX1X2Comp = std::less<int> {};
+constexpr auto fX1X2Comp = std::less<int> {};
-	#else
+#else
-	constexpr auto fX0Comp = std::less<int> {};
+constexpr auto fX0Comp = std::less<int> {};
-	constexpr auto fX1X2Comp = std::greater<int> {};
+constexpr auto fX1X2Comp = std::greater<int> {};
-	#endif
+#endif
 
-	constexpr auto kTopMultimeter = 2048;
+constexpr auto kTopMultimeter = 2048;
-    constexpr double kTriggerSensitivityMultiplier = 4;
+constexpr double kTriggerSensitivityMultiplier = 4;
 }
 
 isoBuffer::isoBuffer(QWidget* parent, int bufferLen, isoDriver* caller, unsigned char channel_value)
     : QWidget(parent)
     , m_channel(channel_value)
     , m_buffer(std::make_unique<short[]>(bufferLen*2))
-	, m_bufferEnd(bufferLen-1)
+    , m_bufferLen(bufferLen)
     , m_samplesPerSecond(bufferLen/21.0/375*VALID_DATA_PER_375)
     , m_sampleRate_bit(bufferLen/21.0/375*VALID_DATA_PER_375*8)
     , m_virtualParent(caller)
 {
 }
 
-// NOTE: the length of half of the allocated buffer is m_bufferEnd+1
+
 void isoBuffer::insertIntoBuffer(short item)
 {
     m_buffer[m_back] = item;
-	m_buffer[m_back+m_bufferEnd+1] = item;
+    m_buffer[m_back+m_bufferLen] = item;
     m_back++;
     m_insertedCount++;
 
-	if (m_insertedCount > m_bufferEnd)
+    if (m_insertedCount > m_bufferLen)
     {
-		m_insertedCount = m_bufferEnd+1;
+        m_insertedCount = m_bufferLen;
     }
 
-    if (m_back > m_bufferEnd)
+    if (m_back == m_bufferLen)
     {
         m_back = 0;
     }
@@ -58,60 +58,11 @@ void isoBuffer::insertIntoBuffer(short item)
     checkTriggered();
 }
 
-short isoBuffer::bufferAt(int idx) const
+short isoBuffer::bufferAt(uint32_t idx) const
 {
-	// NOTE: this is only correct if idx < m_insertedCount
+    if (idx > m_insertedCount)
-    return m_buffer[(m_back-1) + (m_bufferEnd+1) - idx];
+        qFatal("isoBuffer::bufferAt: invalid query");
-}
+    return m_buffer[(m_back-1) + m_bufferLen - idx];
-
-void isoBuffer::outputSampleToFile(double averageSample)
-{
-		char numStr[32];
-		sprintf(numStr,"%7.5f, ", averageSample);
-
-		m_currentFile->write(numStr);
-		m_currentColumn++;
-
-		if (m_currentColumn == COLUMN_BREAK)
-		{
-			m_currentFile->write("\n");
-			m_currentColumn = 0;
-		}
-}
-
-void isoBuffer::maybeOutputSampleToFile(double convertedSample)
-{
-	/*
-	 * This function adds a sample to an accumulator and bumps a sample count.
-	 * After the sample count hits some threshold the samples are averaged
-	 * and the average is written to a file.
-	 * If this makes us hit the max. file size, then fileIO is disabled.
-	 */
-
-	m_fileIO_sampleAccumulator += convertedSample;
-	m_fileIO_sampleCount++;
-
-	if (m_fileIO_sampleCount == m_fileIO_sampleCountPerWrite)
-	{
-		double averageSample = m_fileIO_sampleAccumulator / m_fileIO_sampleCount;
-		outputSampleToFile(averageSample);
-
-		// Reset the accumulator and sample count for next data point.
-		m_fileIO_sampleAccumulator = 0;
-		m_fileIO_sampleCount = 0;
-
-		// value of 0 means "no limit", meaning we must skip the check by returning.
-		if (m_fileIO_maxFileSize == 0)
-			return;
-
-		// 7 chars(number) + 1 char(comma) + 1 char(space) = 9 bytes/sample.
-		m_fileIO_numBytesWritten += 9;
-		if (m_fileIO_numBytesWritten >= m_fileIO_maxFileSize)
-		{
-			m_fileIOEnabled = false; // Just in case signalling fails.
-			fileIOinternalDisable();
-		}
-	}
 }
 
 template<typename T, typename Function>
@@ -185,23 +136,82 @@ std::unique_ptr<short[]> isoBuffer::readBuffer(double sampleWindow, int numSampl
 
 void isoBuffer::clearBuffer()
 {
-	for (int i = 0; i < m_bufferEnd; i++)
+    for (uint32_t i = 0; i < m_bufferLen; i++)
     {
         m_buffer[i] = 0;
+        m_buffer[i + m_bufferLen] = 0;
     }
 
     m_back = 0;
+    m_insertedCount = 0;
 }
 
 void isoBuffer::gainBuffer(int gain_log)
 {
     qDebug() << "Buffer shifted by" << gain_log;
-	for (int i = 0; i < m_bufferEnd; i++)
+    for (uint32_t i = 0; i < m_bufferLen; i++)
     {
         if (gain_log < 0)
+        {
             m_buffer[i] <<= -gain_log;
+            m_buffer[i+m_bufferLen] <<= -gain_log;
+        }
         else
+        {
             m_buffer[i] >>= gain_log;
+            m_buffer[i+m_bufferLen] >>= gain_log;
+        }
+    }
+}
+
+
+void isoBuffer::outputSampleToFile(double averageSample)
+{
+    char numStr[32];
+    sprintf(numStr,"%7.5f, ", averageSample);
+
+    m_currentFile->write(numStr);
+    m_currentColumn++;
+
+    if (m_currentColumn == COLUMN_BREAK)
+    {
+        m_currentFile->write("\n");
+        m_currentColumn = 0;
+    }
+}
+
+void isoBuffer::maybeOutputSampleToFile(double convertedSample)
+{
+    /*
+     * This function adds a sample to an accumulator and bumps a sample count.
+     * After the sample count hits some threshold the samples are averaged
+     * and the average is written to a file.
+     * If this makes us hit the max. file size, then fileIO is disabled.
+     */
+
+    m_fileIO_sampleAccumulator += convertedSample;
+    m_fileIO_sampleCount++;
+
+    if (m_fileIO_sampleCount == m_fileIO_sampleCountPerWrite)
+    {
+        double averageSample = m_fileIO_sampleAccumulator / m_fileIO_sampleCount;
+        outputSampleToFile(averageSample);
+
+        // Reset the accumulator and sample count for next data point.
+        m_fileIO_sampleAccumulator = 0;
+        m_fileIO_sampleCount = 0;
+
+        // value of 0 means "no limit", meaning we must skip the check by returning.
+        if (m_fileIO_maxFileSize == 0)
+            return;
+
+        // 7 chars(number) + 1 char(comma) + 1 char(space) = 9 bytes/sample.
+        m_fileIO_numBytesWritten += 9;
+        if (m_fileIO_numBytesWritten >= m_fileIO_maxFileSize)
+        {
+            m_fileIOEnabled = false; // Just in case signalling fails.
+            fileIOinternalDisable();
+        }
     }
 }
 
@@ -239,6 +249,7 @@ void isoBuffer::disableFileIO()
     return;
 }
 
+
 double isoBuffer::sampleConvert(short sample, int TOP, bool AC) const
 {
     double scope_gain = (double)(m_virtualParent->driver->scopeGain);
@@ -279,14 +290,14 @@ short isoBuffer::inverseSampleConvert(double voltageLevel, int TOP, bool AC) con
     return sample;
 }
 
-// For capacitance measurement. x0, x1 and x2 are all various time points
+// For capacitance measurement.
-// used to find the RC coefficient.
+// x0, x1 and x2 are all various time points used to find the RC coefficient.
 template<typename Function>
 int isoBuffer::capSample(int offset, int target, double seconds, double value, Function comp)
 {
     int samples = seconds * m_samplesPerSecond;
 
-	if (m_back < samples + offset) return -1;
+    if (int32_t(m_back) < samples + offset) return -1;
 
     short sample = inverseSampleConvert(value, 2048, 0);
 
@@ -386,10 +397,9 @@ double isoBuffer::getDelayedTriggerPoint(double delay)
     auto isValid = [=](uint32_t index)->bool
     {
         if (m_back > delaySamples)
-            return (index < ((uint32_t)m_back - delaySamples)) || (index >= (uint32_t)m_back);
+            return (index < m_back - delaySamples) || (index >= m_back);
         else
-            // Fixme: There's probably an off by one here.
+            return (index < m_bufferLen + m_back - delaySamples) && (index >= m_back);
-            return (index < (m_bufferEnd + m_back - delaySamples)) && (index >= m_back);
     };
 
     auto getDelay = [=](uint32_t index)->double
@@ -397,19 +407,32 @@ double isoBuffer::getDelayedTriggerPoint(double delay)
         if (m_back > index)
             return (m_back - index) / static_cast<double>(m_samplesPerSecond);
         else
-            return (m_bufferEnd + m_back - index) / static_cast<double>(m_samplesPerSecond);
+            return (m_bufferLen + (m_back-1) - index) / static_cast<double>(m_samplesPerSecond);
     };
 
-    // Fixme: this won't look at the first element in the list.
+    // Like std::find_if but returns the last element matching the predicate instead of the first one
-    for (auto it = std::prev(m_triggerPositionList.end()); it != m_triggerPositionList.begin(); it--)
+    // TODO: Move this elsewhere (maybe a utils / algorithms file??)
+    // requires first and last to be Bidirectional iters, and form a valid range
+    // requires p to be a valid unaryPredicate
+    // Full signature would be:
+    // template<typename It, typename Predicate>
+    // It find_last_if(It begin, It end, Predicate p)
+    auto find_last_if = [](auto begin, auto end, auto p)
     {
-        if (isValid(*it))
+        using It = decltype(begin); // TODO: remove this line once this is a proper function
+        std::reverse_iterator<It> rlast(begin), rfirst(end);
+        auto found = std::find_if(rfirst, rlast, p);
+        return found == rlast
+               ? end
+               : std::prev(found.base());
+    };
+
+    auto it = find_last_if(m_triggerPositionList.begin(), m_triggerPositionList.end(), isValid);
+    if (it != m_triggerPositionList.end())
     {
-            uint32_t index = *it;
+        // NOTE: vector::erase does not remove the element pointed to by the second iterator.
-            if (it != m_triggerPositionList.begin())
+        m_triggerPositionList.erase(m_triggerPositionList.begin(), it);
-                m_triggerPositionList.erase(m_triggerPositionList.begin(), std::prev(it));
+        return getDelay(m_triggerPositionList[0]);
-            return getDelay(index);
-        }
     }
 
     return 0;

Desktop_Interface/isobuffer.h

@@ -51,7 +51,7 @@ public:
 	~isoBuffer() = default;
 
 //	Basic buffer operations
-	short bufferAt(int idx) const;
+	short bufferAt(uint32_t idx) const;
 	void insertIntoBuffer(short item);
 	void clearBuffer();
 	void gainBuffer(int gain_log);
@@ -99,9 +99,9 @@ public:
 
 //	Internal Storage
 	std::unique_ptr<short[]> m_buffer;
-	int m_back = 0;
+	uint32_t m_back = 0;
-	int m_insertedCount = 0;
+	uint32_t m_insertedCount = 0;
-	int m_bufferEnd;
+	uint32_t m_bufferLen;
 
 // Conversion And Sampling
 	double m_voltage_ref = 1.65;

Desktop_Interface/uartstyledecoder.cpp

@@ -7,7 +7,7 @@ uartStyleDecoder::uartStyleDecoder(QObject *parent_in) : QObject(parent_in)
     parent = (isoBuffer *) parent_in;
 
 	// Begin decoding SAMPLE_DELAY seconds in the past.
-	serialPtr_bit = (int)(parent->m_back * 8 - SERIAL_DELAY * parent->m_sampleRate_bit + parent->m_bufferEnd * 8) % (parent->m_bufferEnd*8);
+	serialPtr_bit = (int)(parent->m_back * 8 - SERIAL_DELAY * parent->m_sampleRate_bit + parent->m_bufferLen * 8) % (parent->m_bufferLen*8);
 
     updateTimer = new QTimer();
     updateTimer->setTimerType(Qt::PreciseTimer);
@@ -91,7 +91,7 @@ void uartStyleDecoder::serialDecode(double baudRate)
 int uartStyleDecoder::serialDistance()
 {
     int back_bit = parent->m_back * 8;
-    int bufferEnd_bit = parent->m_bufferEnd * 8;
+    int bufferEnd_bit = (parent->m_bufferLen-1) * 8;
     if(back_bit >= serialPtr_bit){
         return back_bit - serialPtr_bit;
     }else return bufferEnd_bit - serialPtr_bit + back_bit;
@@ -108,8 +108,8 @@ void uartStyleDecoder::updateSerialPtr(double baudRate, unsigned char current_bi
         serialPtr_bit += distance_between_bits;
     } else serialPtr_bit += (distance_between_bits - 1);  //Less than one baud period so that it will always see that start bit.
 
-    if (serialPtr_bit > (parent->m_bufferEnd * 8)){
+    if (serialPtr_bit >= (parent->m_bufferLen * 8)){
-        serialPtr_bit -= (parent->m_bufferEnd * 8);
+        serialPtr_bit -= (parent->m_bufferLen * 8);
     }
 }