From 80a61a6cbdcd6b5292922425487cfc317bb430f5 Mon Sep 17 00:00:00 2001 From: Quique Tortosa Date: Tue, 1 Sep 2026 14:00:00 +0200 Subject: [PATCH 1/3] Fix stepper drift on modules that are held rather than moved start() re-energizes the coil pattern the rotor is resting on before a move, but it also rewound stepNumber and left it rewound. Two consequences: - a module that is energized and then released without stepping (every module that is not changing character) gets the *previous* pattern written on the next start(), which physically drags the drum one full step backwards. In clock mode moveTo() runs once a minute, so an hour digit that sits still for 59 of 60 updates loses ~59 steps/hour - about 1.4 flaps on a 48-flap drum - and nothing corrects it, because a module that never turns never passes its hall sensor. - a module that does step wastes its first commanded step rewriting the pattern it is already on, so position runs one step ahead of the drum on every move. start() now writes the holding pattern without touching stepNumber, and step() keeps position and stepNumber in lockstep. Also in moveTo(): - track the steps each module still owes instead of comparing positions for equality. A mid-move magnet correction could set position past the target, which the equality test then never matched, sending the module round for another full revolution. - only energize the modules that actually have to move. Holding all eight costs ~200mA each for nothing, and that sag is what makes the modules that *are* moving lose steps on a shared supply. - re-test the finish condition every iteration rather than only inside the 20ms sensor-check window. homeToChar() passed moveTo() its arguments in the wrong order, so homing to a character always ran at the minimum speed. Drop checkAllFalse()/startMotors(), now unused, along with the undefined motorPins[] declaration and the dead file-scope hasErrored. An i2c error now clears itself once the module answers again, instead of silently disabling that module's magnet correction forever. Separately, in the sketch: {mm} maps to %m (month), so the default timeFormat "{HH}:{mm}" rendered hour:month, and the renderTime fallback "HH:mm" had no braces at all so strftime passed it through literally. Both now use {HH}:{MM}. {SS} is documented in the settings help modal but was never translated; added. Co-Authored-By: Claude Opus 5 --- src/SplitFlapDisplay.cpp | 73 ++++++++++++++++++---------------- src/SplitFlapDisplay.h | 2 - src/SplitFlapDisplay.ino | 8 ++-- src/SplitFlapModule.cpp | 86 +++++++++++++++++++++------------------- src/SplitFlapModule.h | 35 +++++++++------- 5 files changed, 109 insertions(+), 95 deletions(-) diff --git a/src/SplitFlapDisplay.cpp b/src/SplitFlapDisplay.cpp index ebe0fe23..8df084f9 100644 --- a/src/SplitFlapDisplay.cpp +++ b/src/SplitFlapDisplay.cpp @@ -115,7 +115,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 +130,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 +140,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 +215,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 +229,44 @@ 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; } } - startMotors(); // not sure if this helps or not, likely that it does not based - // on testing + if (! anyMoving) { + return; + } + 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,16 +283,27 @@ 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 @@ -292,22 +311,6 @@ void SplitFlapDisplay::moveTo(int targetPositions[], float speed, bool releaseMo } } -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++) { diff --git a/src/SplitFlapDisplay.h b/src/SplitFlapDisplay.h index 201e87c4..412417e0 100644 --- a/src/SplitFlapDisplay.h +++ b/src/SplitFlapDisplay.h @@ -39,9 +39,7 @@ class SplitFlapDisplay { private: JsonSettings &settings; - bool checkAllFalse(bool array[], int size); void stopMotors(); - void startMotors(); int numModules; uint8_t moduleAddresses[MAX_MODULES]; diff --git a/src/SplitFlapDisplay.ino b/src/SplitFlapDisplay.ino index c8cb9ae2..c9994957 100644 --- a/src/SplitFlapDisplay.ino +++ b/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("")}, @@ -155,8 +155,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 +276,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 }; diff --git a/src/SplitFlapModule.cpp b/src/SplitFlapModule.cpp index bd7fc25e..4deee17d 100644 --- a/src/SplitFlapModule.cpp +++ b/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,25 @@ int SplitFlapModule::getCharPosition(char inputChar) { } void SplitFlapModule::stop() { - uint16_t stepState = 0b1111111111100001; - writeIO(stepState); + writeIO(IdleState); } +// 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() { diff --git a/src/SplitFlapModule.h b/src/SplitFlapModule.h index 3eed10fe..2388e10d 100644 --- a/src/SplitFlapModule.h +++ b/src/SplitFlapModule.h @@ -12,9 +12,9 @@ class SplitFlapModule { void init(); - void step(bool updatePosition = true); // step motor + void step(); // advance the motor one full step void stop(); // write all motor input pins to low - void start(); // re-energize coils to last position, not stepping motor + void start(); // re-energize coils in place, without moving int getMagnetPosition() const { return magnetPosition; } // position where magnet is detected int getCharPosition(char inputChar); // get integer position given single character @@ -30,21 +30,28 @@ class SplitFlapModule { bool getHasErrored() const { return hasErrored; } private: - uint8_t address; // i2c address of module - int position; // character drum position - int stepNumber; // current position in the stepping order, to make motor move - int stepsPerRot; // number of steps per rotation - bool hasErrored = false; // flag to indicate if an error has occurred + uint8_t address; // i2c address of module + int position; // character drum position + int stepNumber; // index of the NEXT coil pattern to write + int stepsPerRot; // number of steps per rotation + bool hasErrored = false; // set when the last i2c transaction failed, cleared when one succeeds - void writeIO(uint16_t data); // write to motor in pins + void writeIO(uint16_t data); // write to motor in pins - int magnetPosition; // altered by offsets - static const int motorPins[]; // Array of motor pins - static const int HallEffectPIN; // Hall Effect Sensor Pin (On PCF8575) + int magnetPosition; // altered by offsets - const char *chars; // pointer to active character set - int charPositions[48]; // support up to 48 characters - int numChars; // current number of characters + // Coil patterns for one electrical revolution of the 28BYJ-48 (2-phase-on + // full stepping). Bits 1-4 drive the motor, bit 0 and bit 15 are left high: + // bit 15 is the hall effect sensor input on the PCF8575. + // pattern 0 -> P01,P02 1 -> P01,P04 2 -> P03,P04 3 -> P02,P03 + // Writing them in increasing order turns the drum forwards; writing them in + // decreasing order turns it backwards. + static const uint16_t CoilStates[4]; + static const uint16_t IdleState; // all four coils low, hall input high + + const char *chars; // pointer to active character set + int charPositions[48]; // support up to 48 characters + int numChars; // current number of characters int charSetSize; static const char StandardChars[37]; From 75710c041d7099d4a451452f652a8101f26b8db0 Mon Sep 17 00:00:00 2001 From: Quique Tortosa Date: Tue, 1 Sep 2026 14:00:42 +0200 Subject: [PATCH 2/3] Honour releaseMotors when moveTo() has nothing to move The early return added with the only-energize-what-moves change skipped stopMotors(), so a moveTo() that found every module already on target would leave coils energized from a previous moveTo(.., releaseMotors=false). Co-Authored-By: Claude Opus 5 --- src/SplitFlapDisplay.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/SplitFlapDisplay.cpp b/src/SplitFlapDisplay.cpp index 8df084f9..2bcfbe10 100644 --- a/src/SplitFlapDisplay.cpp +++ b/src/SplitFlapDisplay.cpp @@ -247,6 +247,9 @@ void SplitFlapDisplay::moveTo(int targetPositions[], float speed, bool releaseMo } if (! anyMoving) { + if (releaseMotors) { + stopMotors(); + } return; } From 2dd1daca6cfdc0e0bbd917a08178d21b5013b573 Mon Sep 17 00:00:00 2001 From: Quique Tortosa Date: Tue, 1 Sep 2026 14:37:17 +0200 Subject: [PATCH 3/3] Release the coils before anything else at startup The PCF8575 powers up with every output high, and on this hardware high means energized, so at power-on all four coils of all eight modules are driven at once - roughly double the current a module draws while turning, on the whole display simultaneously. Nothing wrote them low until SplitFlapDisplay::init(), which setup() only reaches after mounting LittleFS, starting the web server and running connectToWifi() - and that blocks for up to 20 seconds before giving up. So every boot held the display at full stall current for seconds, longer still when the network was down. setup() now calls releaseAll() as its first act, before STARTUP_DELAY. It needs nothing but the pin and address settings out of NVS and costs one i2c write per module. Same idea as upstream PR #45, which diagnosed this against an 11 module display whose 5V rail collapsed below 3V at power-on; it was closed without merging. Co-Authored-By: Claude Opus 5 --- src/SplitFlapDisplay.cpp | 23 +++++++++++++++++++++++ src/SplitFlapDisplay.h | 1 + src/SplitFlapDisplay.ino | 3 +++ src/SplitFlapModule.cpp | 7 +++++++ src/SplitFlapModule.h | 5 +++++ 5 files changed, 39 insertions(+) diff --git a/src/SplitFlapDisplay.cpp b/src/SplitFlapDisplay.cpp index 2bcfbe10..afd981d9 100644 --- a/src/SplitFlapDisplay.cpp +++ b/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 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"); diff --git a/src/SplitFlapDisplay.h b/src/SplitFlapDisplay.h index 412417e0..78ea0d7f 100644 --- a/src/SplitFlapDisplay.h +++ b/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, diff --git a/src/SplitFlapDisplay.ino b/src/SplitFlapDisplay.ino index c9994957..f032e82c 100644 --- a/src/SplitFlapDisplay.ino +++ b/src/SplitFlapDisplay.ino @@ -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 diff --git a/src/SplitFlapModule.cpp b/src/SplitFlapModule.cpp index 4deee17d..db883402 100644 --- a/src/SplitFlapModule.cpp +++ b/src/SplitFlapModule.cpp @@ -113,6 +113,13 @@ void SplitFlapModule::stop() { 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. diff --git a/src/SplitFlapModule.h b/src/SplitFlapModule.h index 2388e10d..17ae8221 100644 --- a/src/SplitFlapModule.h +++ b/src/SplitFlapModule.h @@ -12,6 +12,11 @@ class SplitFlapModule { void init(); + // Writes the idle pattern straight to an address, without needing a + // constructed module. Used to kill the power-on coil current as early as + // possible, before the settings are even loaded into modules. + static void releaseCoils(uint8_t address); + void step(); // advance the motor one full step void stop(); // write all motor input pins to low void start(); // re-energize coils in place, without moving