Refactor code structure for improved readability and maintainability, imporved matrix read, improved SB button read

dev/digital/Firmware_TEST/.gitignore

@@ -1,4 +1,7 @@
-.pio
+.pio/libdeps
+.pio/build/project*
+.pio/build/esp32-s3-devkitm-1/*
+!.pio/build/esp32-s3-devkitm-1/firmware.bin
 .vscode/.browse.c_cpp.db*
 .vscode/c_cpp_properties.json
 .vscode/launch.json

dev/digital/Firmware_TEST/include/FIRMWARE.h

@@ -3,7 +3,8 @@
     @author:    Erik Tóth
     @contact:   etoth@tsn.at
     @date:      2025-10-26
-    @brief:     Header for FIRMWARE.cpp
+    @updated:   2025-12-06
+    @brief:     Header for FIRMWARE.cpp (FIXED)
 */
 #include <Arduino.h>
 #include <Wire.h>
@@ -139,6 +140,7 @@ class SequencerBlock
         unsigned long _lastStepTime;
         unsigned long _playStartTime;
         unsigned long _stepStartTime;
+        unsigned long _lastAddStepTime;  // NEU: Rate-Limiting
         
         // Status flags
         bool _isRecording;

dev/digital/Firmware_TEST/include/FIRMWARE_DEF.h

@@ -3,6 +3,7 @@
     @author:    Erik Tóth
     @contact:   etoth@tsn.at
     @date:      2025-10-26
+    @updated:   2025-12-06
     @brief:     Header for constant definitions
 */
 
@@ -33,10 +34,10 @@
 #define PIN_K_C3        5 // DEV. not in use
 #define PIN_K_C4        6 // DEV. not in use
 // SEQUENCER BUTTON PINS
-#define PIN_SB_1_REC    37 // for PROD. change to 33 / not available on dev board
+#define PIN_SB_1_REC    42 // for PROD. change to 33 / not available on dev board
-#define PIN_SB_1_PLAY   38 // for PROD. change to 34 / not available on dev board
+#define PIN_SB_1_PLAY   41 // for PROD. change to 34 / not available on dev board
-#define PIN_SB_2_REC    35
+#define PIN_SB_2_REC    40 // 35
-#define PIN_SB_2_PLAY   36
+#define PIN_SB_2_PLAY   39 // 36
 // MISC/INFO PINS
 #define PIN_ACTIVE      -1 // TODO: if any key is played return HIGH
 #define PIN_REC         -1 // TODO: if any sb is recording return HIGH

dev/digital/Firmware_TEST/src/FIRMWARE.cpp

@@ -3,7 +3,8 @@
     @author:    Erik Tóth
     @contact:   etoth@tsn.at
     @date:      2025-10-26
-    @brief:     Firmware for MCU
+    @updated:   2025-12-06
+    @brief:     Firmware für MCU - FIXED VERSION mit Bounds Checks
 */
 
 #include "FIRMWARE.h"
@@ -48,7 +49,7 @@ Keyboard::Keyboard(uint8_t nRows, uint8_t nCols, uint8_t *pinsRow, uint8_t *pins
 void Keyboard::begin()
 {
     for(int i = 0; i < _nRows; i++) pinMode(_pinsRow[i], INPUT_PULLDOWN);
-    for(int i = 0; i < _nCols; i++) pinMode(_pinsCol[i], OUTPUT);
+    for(int i = 0; i < _nCols; i++) pinMode(_pinsCol[i], INPUT);
 }
 
 void Keyboard::update()
@@ -56,6 +57,7 @@ void Keyboard::update()
     unsigned long now = millis();
     for(uint8_t col = 0; col < _nCols; col++)
     {
+        pinMode(_pinsCol[col], OUTPUT);
         digitalWrite(_pinsCol[col], HIGH);
         for(uint8_t row = 0; row < _nRows; ++row) 
         {
@@ -79,6 +81,7 @@ void Keyboard::update()
             }
         }
         digitalWrite(_pinsCol[col], LOW);
+        pinMode(_pinsCol[col], INPUT);
     }
     if((_nActiveKeys == 1) && _inQueue(NOT_A_KEY)) _nActiveKeys = 0;
 }
@@ -231,7 +234,7 @@ uint8_t CV::_getKeyToVoltageIndex(Key k)
     return (k.row*_col + k.col); 
 }
 
-// ==================== SequencerBlock ====================
+// ==================== SequencerBlock (FIXED) ====================
 
 /*!
 *   @param maxDurationMS maximum loop duration of recording in milliseconds
@@ -252,6 +255,7 @@ SequencerBlock::SequencerBlock(uint16_t maxDurationMS, uint16_t maxStepCount)
     _lastStepTime = 0;
     _playStartTime = 0;
     _stepStartTime = 0;
+    _lastAddStepTime = 0;  // NEU: Rate-Limiting
 }
 
 void SequencerBlock::startRecord()
@@ -262,7 +266,8 @@ void SequencerBlock::startRecord()
     _isRecording = true;
     _recordStartTime = millis();
     _lastStepTime = _recordStartTime;
-    _lastVoltageCh1 = 0xFFFF;  // Ungültiger Wert zum Triggern des ersten Steps
+    _lastAddStepTime = _recordStartTime;  // NEU
+    _lastVoltageCh1 = 0xFFFF;
     _lastVoltageCh2 = 0xFFFF;
 }
 
@@ -276,28 +281,54 @@ void SequencerBlock::stopRecord()
 
 void SequencerBlock::addStep(uint16_t voltage_ch1, uint16_t voltage_ch2)
 {
+    // KRITISCHE SICHERHEITSPRÜFUNGEN ZUERST
     if(!_isRecording) return;
+    
+    // Prüfe ob wir überhaupt noch Platz haben (mit Sicherheitsabstand!)
+    if(_stepCount >= _MAX_SEQUENCE_STEPS - 1) 
+    {
+        Serial.println("\n\r[ERROR] Step limit reached! Stopping recording.");
+        stopRecord();
+        return;
+    }
+    
     if(timeLimitReached()) 
     {
+        Serial.println("\n\r[WARNING] Time limit reached! Stopping recording.");
         stopRecord();
         return;
     }
     
     unsigned long now = millis();
     
+    // NEU: Rate-Limiting - ignoriere zu häufige Aufrufe
+    if((now - _lastAddStepTime) < 5)  // Mindestens 5ms zwischen Updates
+    {
+        return;
+    }
+    _lastAddStepTime = now;
+    
     // Hat sich die Spannung geändert?
     bool voltageChanged = (voltage_ch1 != _lastVoltageCh1) || (voltage_ch2 != _lastVoltageCh2);
     
     if(voltageChanged)
     {
+        // WICHTIG: Prüfe nochmal ob wir Platz haben BEVOR wir schreiben!
+        if(_stepCount >= _MAX_SEQUENCE_STEPS - 1)
+        {
+            Serial.println("\n\r[ERROR] Array full! Stopping recording.");
+            stopRecord();
+            return;
+        }
+        
         // Vorherigen Step abschließen (wenn vorhanden)
-        if(_stepCount > 0)
+        if(_stepCount > 0 && _stepCount <= _MAX_SEQUENCE_STEPS)
         {
             _finishCurrentStep();
         }
         
-        // Neuen Step beginnen
+        // Neuen Step beginnen - mit Bounds Check!
-        if(_canAddStep())
+        if(_stepCount < _MAX_SEQUENCE_STEPS)
         {
             _sequence[_stepCount].voltage_ch1 = voltage_ch1;
             _sequence[_stepCount].voltage_ch2 = voltage_ch2;
@@ -312,7 +343,8 @@ void SequencerBlock::addStep(uint16_t voltage_ch1, uint16_t voltage_ch2)
     else
     {
         // Gleiche Spannung - Duration des aktuellen Steps aktualisieren
-        if(_stepCount > 0)
+        // WICHTIG: Bounds Check!
+        if(_stepCount > 0 && _stepCount <= _MAX_SEQUENCE_STEPS)
         {
             _sequence[_stepCount - 1].duration = now - _lastStepTime;
         }
@@ -345,9 +377,37 @@ void SequencerBlock::update()
 {
     if(!_isPlaying || _stepCount == 0) return;
     
+    // WICHTIG: Bounds Check BEVOR wir auf Array zugreifen!
+    if(_currentStep >= _stepCount || _currentStep >= _MAX_SEQUENCE_STEPS)
+    {
+        Serial.println("\n\r[ERROR] Invalid step index in update()!");
+        stopPlay();
+        return;
+    }
+    
     unsigned long now = millis();
     unsigned long elapsed = now - _stepStartTime;
     
+    // Sicherung gegen Division durch Null / Endlosschleife
+    if(_sequence[_currentStep].duration == 0)
+    {
+        _currentStep++;
+        _stepStartTime = now;
+        
+        if(_currentStep >= _stepCount)
+        {
+            if(_loop)
+            {
+                _currentStep = 0;
+            }
+            else
+            {
+                stopPlay();
+            }
+        }
+        return;
+    }
+    
     // Prüfen ob aktueller Schritt abgelaufen ist
     if(elapsed >= _sequence[_currentStep].duration)
     {
@@ -386,7 +446,8 @@ void SequencerBlock::clear()
     _lastVoltageCh1 = 0;
     _lastVoltageCh2 = 0;
     
-    for(uint8_t i = 0; i < _MAX_SEQUENCE_STEPS; i++)
+    // Optional: Array löschen (kann je nach Use-Case weggelassen werden)
+    for(uint16_t i = 0; i < _MAX_SEQUENCE_STEPS; i++)
     {
         _sequence[i].voltage_ch1 = 0;
         _sequence[i].voltage_ch2 = 0;
@@ -411,7 +472,7 @@ bool SequencerBlock::timeLimitReached()
 
 bool SequencerBlock::stepLimitReached()
 {
-    return (_stepCount >= _maxStepCount);
+    return (_stepCount >= _maxStepCount) || (_stepCount >= _MAX_SEQUENCE_STEPS);
 }
 
 uint16_t SequencerBlock::getStepCount()
@@ -422,7 +483,7 @@ uint16_t SequencerBlock::getStepCount()
 uint16_t SequencerBlock::getCurrentVoltageCh1()
 {
     if(!_isPlaying || _stepCount == 0) return 0;
-    if(_currentStep >= _stepCount) return 0;
+    if(_currentStep >= _stepCount || _currentStep >= _MAX_SEQUENCE_STEPS) return 0;
     
     return _sequence[_currentStep].voltage_ch1;
 }
@@ -430,24 +491,25 @@ uint16_t SequencerBlock::getCurrentVoltageCh1()
 uint16_t SequencerBlock::getCurrentVoltageCh2()
 {
     if(!_isPlaying || _stepCount == 0) return 0;
-    if(_currentStep >= _stepCount) return 0;
+    if(_currentStep >= _stepCount || _currentStep >= _MAX_SEQUENCE_STEPS) return 0;
     
     return _sequence[_currentStep].voltage_ch2;
 }
 
 uint16_t SequencerBlock::getTotalDuration()
 {
-    uint16_t total = 0;
+    uint32_t total = 0;  // uint32 um Overflow zu vermeiden
-    for(uint8_t i = 0; i < _stepCount; i++)
+    for(uint16_t i = 0; i < _stepCount && i < _MAX_SEQUENCE_STEPS; i++)
     {
         total += _sequence[i].duration;
     }
-    return total;
+    return (total > 65535) ? 65535 : (uint16_t)total;  // Clamp auf uint16
 }
 
 void SequencerBlock::_finishCurrentStep()
 {
     if(_stepCount == 0) return;
+    if(_stepCount > _MAX_SEQUENCE_STEPS) return;  // Sicherheitsprüfung
     
     unsigned long now = millis();
     uint16_t duration = now - _lastStepTime;

dev/digital/Firmware_TEST/src/main.cpp

@@ -1,8 +1,9 @@
 /*
-*     Example Code Three - Dual Channel Sequencer
+*     Example Code Three - Dual Channel Sequencer (FIXED)
-*       TODO:
+*     - Bounds Checks hinzugefügt
-            - add predefined sequence of voltage (e.g. for usage as startup sound)
+*     - Rate-Limiting implementiert
-            - implement INFO and MISC pins form file FIRMWARE_DEF.h
+*     - Debug-Ausgaben erweitert
+*     - Stack Overflow verhindert
 */
 #include "FIRMWARE_DEF.h"
 #include "FIRMWARE.h"
@@ -41,9 +42,17 @@ ButtonState btn_sb2_play;
 
 const unsigned long DEBOUNCE_DELAY = 50;
 
+// Loop-Status für State Machine
+static bool seq1_loop_active = false;
+static bool seq2_loop_active = false;
+
+// NEU: Tracking für Voltage Changes
+static uint16_t last_voltage_ch1 = 0xFFFF;
+static uint16_t last_voltage_ch2 = 0xFFFF;
+
 bool readButton(byte pin, ButtonState &state)
 {
-    bool reading = digitalRead(pin) == HIGH;
+    bool reading = digitalRead(pin) == LOW;
     bool buttonPressed = false;
     
     if(reading != state.last)
@@ -69,10 +78,10 @@ bool readButton(byte pin, ButtonState &state)
 
 void initButtons()
 {
-    pinMode(PIN_SB_1_REC, INPUT_PULLDOWN);
+    pinMode(PIN_SB_1_REC, INPUT_PULLUP);
-    pinMode(PIN_SB_1_PLAY, INPUT_PULLDOWN);
+    pinMode(PIN_SB_1_PLAY, INPUT_PULLUP);
-    pinMode(PIN_SB_2_REC, INPUT_PULLDOWN);
+    pinMode(PIN_SB_2_REC, INPUT_PULLUP);
-    pinMode(PIN_SB_2_PLAY, INPUT_PULLDOWN);
+    pinMode(PIN_SB_2_PLAY, INPUT_PULLUP);
     
     btn_sb1_rec.current = false;
     btn_sb1_rec.last = false;
@@ -93,7 +102,7 @@ void initButtons()
 
 void handleSequencerButtons()
 {
-    // Sequencer 1 Record Button
+    // ===== Sequencer 1 Record Button =====
     if(readButton(PIN_SB_1_REC, btn_sb1_rec))
     {
         if(sb1.isRecording())
@@ -106,58 +115,39 @@ void handleSequencerButtons()
         {
             if(sb1.isPlaying()) sb1.stopPlay();
             sb1.startRecord();
+            last_voltage_ch1 = 0xFFFF;  // Reset voltage tracking
+            last_voltage_ch2 = 0xFFFF;
             Serial.printf("\n\r[SEQ1] Recording started (2 channels)...");
         }
     }
     
-    // Sequencer 1 Play Button - 3 Modi: Play / Loop / Stop
+    // ===== Sequencer 1 Play Button (3 Stati: Play / Loop / Stop) =====
     if(readButton(PIN_SB_1_PLAY, btn_sb1_play))
     {
         if(!sb1.isPlaying())
         {
-            // Nicht am Spielen -> Starte Playback (ohne Loop)
             if(sb1.isRecording()) sb1.stopRecord();
             sb1.setLoop(false);
+            seq1_loop_active = false;
             sb1.startPlay();
-            Serial.printf("\n\r[SEQ1] Playback started (single)\n\r\tSteps: %i, Duartion: %ims", sb1.getStepCount(), sb1.getTotalDuration());
+            Serial.printf("\n\r[SEQ1] Playback started (single)\n\r\tSteps: %i, Duration: %ims", 
+                         sb1.getStepCount(), sb1.getTotalDuration());
+        }
+        else if(!seq1_loop_active)
+        {
+            sb1.setLoop(true);
+            seq1_loop_active = true;
+            Serial.printf("\n\r[SEQ1] Loop activated");
         }
         else
         {
-            // Am Spielen -> Prüfe Loop-Status
+            sb1.stopPlay();
-            if(!sb1.isPlaying())  // Falls schon gestoppt
+            seq1_loop_active = false;
-            {
+            Serial.printf("\n\r[SEQ1] Playback stopped");
-                // Starte neu
-                sb1.setLoop(false);
-                sb1.startPlay();
-                Serial.printf("\n\r[SEQ1] Playback started (single)");
-            }
-            else
-            {
-                // Ist am Spielen - ermittle ob Loop aktiv ist
-                // Wir testen das indirekt: Wenn ein Sequencer am Ende angekommen ist
-                // und noch spielt, dann muss Loop aktiv sein
-                // Alternative: Wir tracken den Loop-Status selbst
-                static bool seq1_loop_active = false;
-                
-                if(!seq1_loop_active)
-                {
-                    // 2. Klick: Loop aktivieren
-                    sb1.setLoop(true);
-                    seq1_loop_active = true;
-                    Serial.printf("\n\r[SEQ1] Loop activated");
-                }
-                else
-                {
-                    // 3. Klick: Stop
-                    sb1.stopPlay();
-                    seq1_loop_active = false;
-                    Serial.printf("\n\r[SEQ1] Playback stopped");
-                }
-            }
         }
     }
     
-    // Sequencer 2 Record Button
+    // ===== Sequencer 2 Record Button =====
     if(readButton(PIN_SB_2_REC, btn_sb2_rec))
     {
         if(sb2.isRecording())
@@ -170,40 +160,35 @@ void handleSequencerButtons()
         {
             if(sb2.isPlaying()) sb2.stopPlay();
             sb2.startRecord();
+            last_voltage_ch1 = 0xFFFF;  // Reset voltage tracking
+            last_voltage_ch2 = 0xFFFF;
             Serial.printf("\n\r[SEQ2] Recording started (2 channels)...");
         }
     }
     
-    // Sequencer 2 Play Button - 3 Modi: Play / Loop / Stop
+    // ===== Sequencer 2 Play Button (3 Stati: Play / Loop / Stop) =====
     if(readButton(PIN_SB_2_PLAY, btn_sb2_play))
     {
-        static bool seq2_loop_active = false;
-        
         if(!sb2.isPlaying())
         {
-            // Nicht am Spielen -> Starte Playback (ohne Loop)
             if(sb2.isRecording()) sb2.stopRecord();
             sb2.setLoop(false);
             seq2_loop_active = false;
             sb2.startPlay();
-            Serial.printf("\n\r[SEQ2] Playback started (single)");
+            Serial.printf("\n\r[SEQ2] Playback started (single)\n\r\tSteps: %i, Duration: %ims", 
+                         sb2.getStepCount(), sb2.getTotalDuration());
+        }
+        else if(!seq2_loop_active)
+        {
+            sb2.setLoop(true);
+            seq2_loop_active = true;
+            Serial.printf("\n\r[SEQ2] Loop activated");
         }
         else
         {
-            if(!seq2_loop_active)
+            sb2.stopPlay();
-            {
+            seq2_loop_active = false;
-                // 2. Klick: Loop aktivieren
+            Serial.printf("\n\r[SEQ2] Playback stopped");
-                sb2.setLoop(true);
-                seq2_loop_active = true;
-                Serial.printf("\n\r[SEQ2] Loop activated");
-            }
-            else
-            {
-                // 3. Klick: Stop
-                sb2.stopPlay();
-                seq2_loop_active = false;
-                Serial.printf("\n\r[SEQ2] Playback stopped");
-            }
         }
     }
 }
@@ -211,8 +196,28 @@ void handleSequencerButtons()
 void setup()
 {
     Serial.begin(BAUDRATE);
+    delay(2000);
+    Serial.printf("\n\r=== FIXED VERSION v2 ===");
+    Serial.printf("\n\rSerial OK!");
+
     keyboard.begin();
-    cv.begin(PIN_SDA, PIN_SCL);
+    
+    // Fehlerbehandlung für CV-Initialisierung
+    unsigned long timeout = millis() + 5000;
+    while(!cv.begin(PIN_SDA, PIN_SCL))
+    {
+        Serial.printf("\n\r[ERROR] CV initialization failed. Retrying...");
+        delay(500);
+        
+        if(millis() > timeout)
+        {
+            Serial.printf("\n\r[FATAL] CV initialization timeout! Check I2C connection.");
+            break;
+        }
+    }
+    
+    Serial.printf("\n\r[OK] CV initialized");
+    
     initButtons();
     
     sb1.setLoop(false);
@@ -227,11 +232,47 @@ void setup()
     Serial.printf("\n\r                 3rd click: Stop");
     Serial.printf("\n\r  PIN_SB_2_REC:  SEQ2 Record Start/Stop (CH1+CH2)");
     Serial.printf("\n\r  PIN_SB_2_PLAY: SEQ2 Play Mode (same as SEQ1)");
+    Serial.printf("\n\r");
+    Serial.printf("\n\rFIXES:");
+    Serial.printf("\n\r  - Bounds checks in all array accesses");
+    Serial.printf("\n\r  - Rate limiting (5ms) for addStep()");
+    Serial.printf("\n\r  - Only call addStep() on voltage change");
+    Serial.printf("\n\r  - Stack overflow prevention");
     Serial.printf("\n\r==============================================\n\r");
 }
 
 void loop() 
 {
+    // ===== DEBUG HEARTBEAT =====
+    static unsigned long lastDebugPrint = 0;
+    static unsigned long loopCounter = 0;
+    
+    loopCounter++;
+    
+    // Debug-Ausgabe alle 5 Sekunden
+    if(millis() - lastDebugPrint > 5000)
+    {
+        Serial.printf("\n\r[HEARTBEAT] Loop count: %lu", loopCounter);
+        Serial.printf("\n\r[DEBUG] SB1: Rec=%d, Play=%d, Steps=%d", 
+                     sb1.isRecording(), sb1.isPlaying(), sb1.getStepCount());
+        Serial.printf("\n\r[DEBUG] SB2: Rec=%d, Play=%d, Steps=%d", 
+                     sb2.isRecording(), sb2.isPlaying(), sb2.getStepCount());
+        Serial.printf("\n\r[DEBUG] Free heap: %lu bytes", ESP.getFreeHeap());
+        lastDebugPrint = millis();
+    }
+    
+    // ===== NON-BLOCKING TIMING SYSTEM =====
+    static unsigned long lastLoopTime = 0;
+    unsigned long now = millis();
+    const unsigned long LOOP_INTERVAL = 10;  // 10ms
+    
+    if((now - lastLoopTime) < LOOP_INTERVAL)
+    {
+        return;  // Nicht blockierend
+    }
+    lastLoopTime = now;
+    
+    // ===== NORMALE UPDATE-FUNKTIONEN =====
     keyboard.update();
     handleSequencerButtons();
     
@@ -263,14 +304,27 @@ void loop()
         }
     }
     
-    // Bei Recording: Beide Kanäle aufnehmen
+    // Bei Recording: Beide Kanäle aufnehmen - NUR bei Änderung!
+    bool voltageChanged = (voltage_ch1 != last_voltage_ch1) || (voltage_ch2 != last_voltage_ch2);
+    
     if(sb1.isRecording())
     {
-        sb1.addStep(voltage_ch1, voltage_ch2);
+        if(voltageChanged)
+        {
+            sb1.addStep(voltage_ch1, voltage_ch2);
+            last_voltage_ch1 = voltage_ch1;
+            last_voltage_ch2 = voltage_ch2;
+        }
     }
+    
     if(sb2.isRecording())
     {
-        sb2.addStep(voltage_ch1, voltage_ch2);
+        if(voltageChanged)
+        {
+            sb2.addStep(voltage_ch1, voltage_ch2);
+            last_voltage_ch1 = voltage_ch1;
+            last_voltage_ch2 = voltage_ch2;
+        }
     }
     
     // CV-Ausgabe: Priorität hat Sequencer-Wiedergabe
@@ -306,6 +360,4 @@ void loop()
         Serial.printf("\n\r[SEQ2] Final: Steps: %i, Duration: %ims", 
                      sb2.getStepCount(), sb2.getTotalDuration());
     }
-    
-    delay(10);
 }