fix: modules held rather than moved drift one step backwards per move

src/SplitFlapDisplay.cpp

@@ -6,6 +6,29 @@
 
 SplitFlapDisplay::SplitFlapDisplay(JsonSettings &settings) : settings(settings) {}
 
+// The PCF8575 powers up with every output high, which on this hardware means
+// all four coils of every module are energized - roughly double the current a
+// module draws while actually turning, on all of them at once. Nothing writes
+// them low until init(), which runs after the web server is up and after a WiFi
+// connect that blocks for up to 20 seconds, so the whole display sits at that
+// current for the entire startup. Call this first instead: it only needs the
+// pin and address settings, and costs a handful of i2c writes.
+void SplitFlapDisplay::releaseAll() {
+    int count = constrain(settings.getInt("moduleCount"), 1, MAX_MODULES);
+    std::vector<int> addresses = settings.getIntVector("moduleAddresses");
+
+    Wire.begin(settings.getInt("sdaPin"), settings.getInt("sclPin"));
+    Wire.setClock(400000);
+
+    for (int i = 0; i < count; i++) {
+        SplitFlapModule::releaseCoils((uint8_t) (i < (int) addresses.size() ? addresses[i] : 0x20 + i));
+    }
+
+    Serial.print("Released coils on ");
+    Serial.print(count);
+    Serial.println(" modules");
+}
+
 void SplitFlapDisplay::init() {
     numModules = settings.getInt("moduleCount");
     stepsPerRot = settings.getInt("stepsPerRot");
@@ -115,7 +138,6 @@ void SplitFlapDisplay::home(float speed) {
     for (int i = 0; i < numModules; i++) {
         targetPositions[i] = (modules[i].getPosition() - 1 + stepsPerRot) % stepsPerRot;
     }
-    startMotors();
     moveTo(targetPositions, speed, false);
     char homeChar = ' ';
     int charPosition;
@@ -131,7 +153,6 @@ void SplitFlapDisplay::homeToString(String homeString, float speed, bool centeri
     for (int i = 0; i < numModules; i++) {
         targetPositions[i] = (modules[i].getPosition() - 1 + stepsPerRot) % stepsPerRot;
     }
-    startMotors();
     moveTo(targetPositions, speed, false);
     writeString(homeString, speed, centering);
 }
@@ -142,13 +163,12 @@ void SplitFlapDisplay::homeToChar(char homeChar, float speed) {
     for (int i = 0; i < numModules; i++) {
         targetPositions[i] = (modules[i].getPosition() - 1 + stepsPerRot) % stepsPerRot;
     }
-    startMotors();
     moveTo(targetPositions, speed, false);
 
     for (int i = 0; i < numModules; i++) {
         targetPositions[i] = modules[i].getCharPosition(homeChar);
     }
-    moveTo(targetPositions, true, speed);
+    moveTo(targetPositions, speed);
 }
 
 void SplitFlapDisplay::writeChar(char inputChar, float speed) {
@@ -218,10 +238,12 @@ void SplitFlapDisplay::moveTo(int targetPositions[], float speed, bool releaseMo
 
     bool resetLatches[numModules] = {}; // Initialize to false //start with latch on to prevent case where the
     // motion starts with the magnet over the sensor
-    bool needsStepping[numModules] = {};             // Initialize to false; //modules that still require moving
+    int stepsRemaining[numModules] = {};             // steps each module still has to turn
     unsigned long lastStepTimes[numModules] = {};    // Initialize to false; //track when each module was last stepped
     unsigned long lastSensorCheckTime = currentTime; // track when we last read all the hall effect sensors
 
+    bool anyMoving = false;
+
     for (int i = 0; i < numModules; i++) {
         targetPositions[i] = constrain(
             targetPositions[i],
@@ -230,35 +252,47 @@ void SplitFlapDisplay::moveTo(int targetPositions[], float speed, bool releaseMo
         ); // Constrain to avoid errors with incorrect inputs
         resetLatches[i] = true;
         lastStepTimes[i] = currentTime;
-        if (modules[i].getPosition() != targetPositions[i]) {
-            needsStepping[i] = true;
-        } else {
-            needsStepping[i] = false;
+
+        // The drum only ever turns forwards, so the work left to do is the
+        // forward distance to the target. Tracking a step count rather than
+        // comparing positions for equality means a mid-move magnet correction
+        // can never step straight over the target and send the module round for
+        // another full revolution.
+        stepsRemaining[i] = (targetPositions[i] - modules[i].getPosition() + stepsPerRot) % stepsPerRot;
+
+        if (stepsRemaining[i] > 0) {
+            // Only energize the modules that actually have to move. Holding all
+            // of them costs ~200mA each for nothing, and on a shared supply that
+            // sag is exactly what makes the moving modules lose steps.
+            modules[i].start();
+            anyMoving = true;
+        }
+    }
+
+    if (! anyMoving) {
+        if (releaseMotors) {
+            stopMotors();
         }
+        return;
     }
 
-    startMotors(); // not sure if this helps or not, likely that it does not based
-    // on testing
     delay(startStopDelay); // give the motor time to align to magnetic field
 
-    bool isFinished = checkAllFalse(needsStepping, numModules);
+    bool isFinished = false;
     while (! isFinished) {
         currentTime = micros();
         for (int i = 0; i < numModules; i++) {
-            if (((currentTime - lastStepTimes[i]) > timePerStep) && needsStepping[i]) {
+            if (((currentTime - lastStepTimes[i]) > timePerStep) && stepsRemaining[i] > 0) {
                 modules[i].step();
+                stepsRemaining[i]--;
                 lastStepTimes[i] = micros();
-                if (modules[i].getPosition() == targetPositions[i]) { // this module is not in the correct position,
-                    // requires stepping
-                    needsStepping[i] = false;
-                }
             }
         }
 
         if ((currentTime - lastSensorCheckTime) > checkIntervalUs) { // check hall effect sensor every checkIntervalMs
             // check every modules sensor
             for (int i = 0; i < numModules; i++) {
-                if (needsStepping[i] &&
+                if (stepsRemaining[i] > 0 &&
                     (modules[i].readHallEffectSensor() == true
                     )) { // only check sensors where the module is still moving
                     if (! resetLatches[i]) {
@@ -275,39 +309,34 @@ void SplitFlapDisplay::moveTo(int targetPositions[], float speed, bool releaseMo
                         //  modules[i].getPosition()));
                         modules[i].magnetDetected(); // update position to the modules
                         // magnet position
+
+                        // re-derive what is left to turn from the corrected position
+                        stepsRemaining[i] =
+                            (targetPositions[i] - modules[i].getPosition() + stepsPerRot) % stepsPerRot;
                         resetLatches[i] = true;
                     }
                 } else if (resetLatches[i] == true) {
                     resetLatches[i] = false;
                 }
             }
-            isFinished = checkAllFalse(needsStepping, numModules);
             lastSensorCheckTime = currentTime; // recall micros because for loop may
             // take a moment to execute
         }
+
+        isFinished = true;
+        for (int i = 0; i < numModules; i++) {
+            if (stepsRemaining[i] > 0) {
+                isFinished = false;
+                break;
+            }
+        }
     }
     if (releaseMotors) {
         delay(startStopDelay); // allow all motors time to settle
         stopMotors();
     }
 }
 
-bool SplitFlapDisplay::checkAllFalse(bool array[], int size) {
-    for (int i = 0; i < size; i++) {
-        if (array[i] == true) {
-            return false;              // As soon as a true value is found, return false
-        }
-    }
-    return true;                       // All values were false
-}
-
-void SplitFlapDisplay::startMotors() { // Probably broken somewhere, not sure
-    // why, haven't looked
-    for (int i = 0; i < numModules; i++) {
-        modules[i].start();
-    }
-}
-
 void SplitFlapDisplay::stopMotors() {
     // Serial.println("Stopping Motors");
     for (int i = 0; i < numModules; i++) {

src/SplitFlapDisplay.h

@@ -14,6 +14,7 @@ class SplitFlapDisplay {
   public:
     SplitFlapDisplay(JsonSettings &settings);
 
+    void releaseAll(); // de-energize every module's coils as early as possible
     void init();
     void writeString(
         String inputString, float speed = MAX_RPM,
@@ -39,9 +40,7 @@ class SplitFlapDisplay {
   private:
     JsonSettings &settings;
 
-    bool checkAllFalse(bool array[], int size);
     void stopMotors();
-    void startMotors();
 
     int numModules;
     uint8_t moduleAddresses[MAX_MODULES];

src/SplitFlapDisplay.ino

@@ -20,7 +20,7 @@ JsonSettings settings = JsonSettings("config", {
     {"otaPass", JsonSetting("")},
     {"timezone", JsonSetting("UTC0")},
     {"dateFormat", JsonSetting("{dd}-{mm}-{yy}")},
-    {"timeFormat", JsonSetting("{HH}:{mm}")},
+    {"timeFormat", JsonSetting("{HH}:{MM}")},
     // Wifi Settings
     {"ssid", JsonSetting("")},
     {"password", JsonSetting("")},
@@ -54,6 +54,9 @@ void setup() {
     // put your setup code here, to run once:
     Serial.begin(SERIAL_SPEED);
 
+    // Before anything else: the modules power up with every coil energized.
+    display.releaseAll();
+
 #ifdef STARTUP_DELAY
     delay(STARTUP_DELAY);
 #endif
@@ -155,8 +158,9 @@ void timeMode() {
     if (millis() - webServer.getLastCheckDateTime() > webServer.getDateCheckInterval()) {
         webServer.setLastCheckDateTime(millis());
 
-        // Get user-friendly format from settings (fallback to "HH:mm")
-        String userFormat = settings.getString("timeFormat").length() > 0 ? settings.getString("timeFormat") : "HH:mm";
+        // Get user-friendly format from settings (fallback to "{HH}:{MM}")
+        String userFormat =
+            settings.getString("timeFormat").length() > 0 ? settings.getString("timeFormat") : "{HH}:{MM}";
 
         // Convert to strftime-compatible format
         String strftimeFormat = convertToStrftime(userFormat);
@@ -275,6 +279,7 @@ String convertToStrftime(String userFormat) {
         {"{HH}", "%H"},   // Hours (24-hour clock, 00–23)
         {"{hh}", "%I"},   // Hours (12-hour clock, 01–12)
         {"{MM}", "%M"},   // Minutes (00–59)
+        {"{SS}", "%S"},   // Seconds (00-59)
         {"{AMPM}", "%p"}, // AM or PM
     };
 

src/SplitFlapModule.cpp

@@ -12,7 +12,16 @@ const char SplitFlapModule::ExtendedChars[48] = {
     '5', '6', '7', '8', '9', '\'', ':', '?', '!', '.', '-', '/', '$', '@', '#', '%',
 };
 
-bool hasErrored = false;
+// Full-step (2-phase-on) sequence, see the header for the pin mapping.
+const uint16_t SplitFlapModule::CoilStates[4] = {
+    0b1111111111100111, // P01 + P02
+    0b1111111111110011, // P01 + P04
+    0b1111111111111001, // P03 + P04
+    0b1111111111101101, // P02 + P03
+};
+
+// All four coil pins low: the motor is released and draws no current.
+const uint16_t SplitFlapModule::IdleState = 0b1111111111100001;
 
 // Default Constructor
 SplitFlapModule::SplitFlapModule()
@@ -38,17 +47,26 @@ void SplitFlapModule::writeIO(uint16_t data) {
 
     byte error = Wire.endTransmission();
 
-    if (error > 0 && ! hasErrored) {
-        hasErrored = true; // Set the error flag
-        Serial.print("Error writing data to module ");
+    if (error > 0) {
+        if (! hasErrored) {
+            hasErrored = true; // Set the error flag
+            Serial.print("Error writing data to module ");
+            Serial.print(address);
+            Serial.print(", error code: ");
+            Serial.println(error); // Error codes:
+            // 0 = success
+            // 1 = data too long to fit in transmit buffer
+            // 2 = received NACK on transmit of address
+            // 3 = received NACK on transmit of data
+            // 4 = other error
+        }
+    } else if (hasErrored) {
+        // A single glitch on the bus must not disable this module forever:
+        // clear the flag as soon as it answers again.
+        hasErrored = false;
+        Serial.print("Module ");
         Serial.print(address);
-        Serial.print(", error code: ");
-        Serial.println(error); // Error codes:
-        // 0 = success
-        // 1 = data too long to fit in transmit buffer
-        // 2 = received NACK on transmit of address
-        // 3 = received NACK on transmit of data
-        // 4 = other error
+        Serial.println(" recovered");
     }
 }
 
@@ -61,10 +79,10 @@ void SplitFlapModule::init() {
         currentPosition += stepSize;
     }
 
-    uint16_t initState = 0b1111111111100001; // Pin 15 (17) as INPUT, Pins 1-4 as OUTPUT
+    uint16_t initState = IdleState; // Pin 15 (17) as INPUT, Pins 1-4 as OUTPUT
     writeIO(initState);
 
-    stop();                                  // Write all motor coil inputs LOW
+    stop();                         // Write all motor coil inputs LOW
 
     int initDelay = 100;
 
@@ -92,39 +110,32 @@ int SplitFlapModule::getCharPosition(char inputChar) {
 }
 
 void SplitFlapModule::stop() {
-    uint16_t stepState = 0b1111111111100001;
-    writeIO(stepState);
+    writeIO(IdleState);
+}
+
+void SplitFlapModule::releaseCoils(uint8_t address) {
+    Wire.beginTransmission(address);
+    Wire.write(IdleState & 0xFF);
+    Wire.write((IdleState >> 8) & 0xFF);
+    Wire.endTransmission();
 }
 
+// Re-energize the coil pattern the rotor is already resting on so the drum is
+// held before a move begins. step() leaves stepNumber pointing at the *next*
+// pattern to write, so the pattern currently under the rotor is stepNumber - 1.
+//
+// This must NOT advance or rewind stepNumber: doing so writes a pattern the
+// rotor is not sitting on and drags the drum a full step backwards every time
+// start() is called, which silently de-calibrates any module that is being held
+// rather than moved.
 void SplitFlapModule::start() {
-    stepNumber = (stepNumber + 3) % 4; // effectively take one off stepNumber
-    step(false);                       // write the "previous" step high again, in case turned off
+    writeIO(CoilStates[(stepNumber + 3) % 4]);
 }
 
-void SplitFlapModule::step(bool updatePosition) {
-    uint16_t stepState;
-    switch (stepNumber) {
-        case 0:
-            stepState = 0b1111111111100111;
-            writeIO(stepState);
-            break;
-        case 1:
-            stepState = 0b1111111111110011;
-            writeIO(stepState);
-            break;
-        case 2:
-            stepState = 0b1111111111111001;
-            writeIO(stepState);
-            break;
-        case 3:
-            stepState = 0b1111111111101101;
-            writeIO(stepState);
-            break;
-    }
-    if (updatePosition) {
-        position = (position + 1) % stepsPerRot;
-        stepNumber = (stepNumber + 1) % 4;
-    }
+void SplitFlapModule::step() {
+    writeIO(CoilStates[stepNumber]);
+    stepNumber = (stepNumber + 1) % 4;
+    position = (position + 1) % stepsPerRot;
 }
 
 bool SplitFlapModule::readHallEffectSensor() {