Feature Request: Vorschläge Code Anpassungen app::setup & app::loop

[beegee3]

hier ein paar Hinweise und Vorschläge basierend auf dev03, Version 0.5.47:

• WIFI_TRY_CONNECT_TIME (in config.h definiert) wird nur in main.cpp für myApp.setup(...) benutzt,
  aber in app::setup(uint32_t timeout) gar nicht verwendet, kann also entfernt werden.

• Der ahoyTimer wird in ahoywifi.app nicht benutzt, d.h. #include "../utils/ahoyTimer.h" kann dort entfernt werden.

• In app::setup wird addListener(...) für ntpUpdateTick erstellt. Das sollte nur für #if !defined(AP_ONLY) sein.

• In app:loop wird jede Sekunde geprüft, ob die Sonnenaufgang bzw. -untergang Zeiten neu berechnet werden müssen.
  Auch das sollte nur für #if !defined(AP_ONLY) gemacht werden, aber viel wichtiger:
  der if(...) Ausdruck wird 86400-mal am Tag berechnet, obwohl die Zeiten nur 1-mal aktualisiert werden.
  Und in dem Ausdruck sind zwei Divisionen, was den Prozessor unnötig auslastet (weil es ja jede Sekunde passiert).
  Ohne sonst viel anzupassen, sollte die Aktualisierung der Sonnenzeiten besser in einer eigenen Funktion isoliert werden,
  die z.B. mit addListener(EVERY_HR, ...) in app::setup eingestellt wird und so nur stündlich aufgerufen wird.
  Das ist dann zwar immer noch 24-mal pro Tag und die Aktualisierung findet irgendwann zwischen Mitternacht und ein Uhr morgens statt, aber das sollte akzeptabel sein.
  Der erste Aufruf nach einem Reboot könnte z.B. in der Funktion uptimeTick if (mUpdateNtp) {...} nach mWifi.getNtpTime() passieren (sonst wartet man ja eine Stunde).
  Die oben genannten zwei Divisionen kann man auch vermeiden, indem nicht mLatestSunTimestamp berechnet wird, sondern die
  nächste Mitternacht Zeit in UTC konvertiert:
  mNextSunTimestamp = ((mUtcTimestamp + mCalculatedTimezoneOffset) / 86400 + 1) * 86400 - mCalculatedTimezoneOffset;
  Die if(...) Abfrage wäre dann:
  if (mUtcTimestamp > mNextSunTimestamp && mConfig->sun.lat && mConfig->sun.lon) { // update on reboot or midnight ... }
  Daraus folgen dann weitere Anpassungen:
  in app::resetSystem:
  mNextSunTimestamp = 946684800; // midnight 1/1/2000

  in app.h:
  ...
  inline uint32_t getLatestSunTimestamp(void) { return mNextSunTimestamp; }
  ...
  uint32_t mNextSunTimestamp;

  wenn man die inline Funktion z.B. in getNextSunTimestamp umbenennen will, dann auch in webApi.cpp!

• In der Scheduler loop wird davon ausgegangen, dass von einem zum nächsten Aufruf weniger als 2 Sekunden vergehen.
  Das muss aber nicht sein, z.B. wenn der NTP Server nicht antwortet. Ist nicht dramatisch, aber es können sich Verschiebungen anhäufen.
  Ausgehend davon, dass es keine einzelnen Verschiebungen von 1 Minute oder mehr gibt, genügt es, die Sekunden aus der Differenz von millis() und mPrevMillis zu berechnen. Im Detail ist es dann wegen des millis() overflow alle ~50 Tage aber doch schwieriger,
  hier mein Vorschlag (bei Vermeidung dynamischer Variablen und Integerdivison):

    void loop() { 
        mMillis = millis();
        mDiff = mMillis - mPrevMillis;
        if (mDiff >= 1000) {
            if (mMillis < mPrevMillis) { // milis() overflow correction
                mDiff +=  ((uint32_t)-1); // ((uint32_t)-1) = UINT32_MAX
                if(mDiff < 1000)
                    return;
            }
            mDiffSeconds = 1;
            if (mDiff >= 2000)
                mDiffSeconds = mDiff / 1000;
            mPrevMillis += mDiffSeconds * 1000;
            notify(&mListSecond);
            mSeconds += mDiffSeconds;
            if(mSeconds >= 60) {
                mSeconds = 0;
                notify(&mListMinute);
                if(++mMinutes >= 60) {
                    mMinutes = 0;
                    notify(&mListHour);
                    if(++mHours == 12)
                        notify(&mList12h);
                    if(mHours >= 24) {
                        mHours = 0;
                        notify(&mListDay);
                    }
                }
            }
        }
    }
    ...
    private:
    uint32_t mMillis;
    uint32_t mPrevMillis;
    uint32_t mDiff;
    uint8_t mDiffSeconds, mSeconds, mMinutes, mHours;

[knickohr]

Die Sonnenberechnung reicht wirklich nur einmal um Mitternacht und nachdem der ESP gebootet hat. Sooo viele Sonnen haben wir ja nicht 😅

[lumapu]

vielen Dank für die konstruktiven Vorschläge, sind sehr sinnvoll.
Das Thema scheduling bearbeite ich gerade, dabei ist mir die Berechnung von sunrise auch aufgefallen.
Gerne weiter Vorschläge machen, damit wird Ahoy immer besser!

[beegee3]

@lumapu vielen Dank für die guten Worte. Wenn schon am scheduling gearbeitet wird und Vorschläge willkommen sind, so würde ich die ganze Scheduler Klasse entfernen und durch die Ticker Klasse ersetzen. Nutzt den ESP Timer, ist viel flexibler (beliebige Zeiten) und kann wieder entfernt werden.
Beispiel Sonnenberechnung:
in app.h #include <Ticker.h> und Ticker mSunTicker;
in app.cpp

app::setup 
...
mSunTicker.attach(10.0, calcSun); // start with 10 sec timer, will be adjusted in calcSun
...

und die Funktion 

void calcSun() {
   if ( ... ) {
      ... // sunrise/sunset calculation
    mSunTicker.detach();
    uint32_t mNextSunTimestamp = ((mUtcTimestamp + mCalculatedTimezoneOffset) / 86400 + 1) * 86400 - mCalculatedTimezoneOffset;
    mSunTicker.attach((float)(mNextSunTimestamp-mUtcTimestamp+1), calcSun); // next update: midnight + 1 sec
   }

[lumapu]

Ich habe jetzt absichtlich nicht die Ticker Funktion von ESP genommen, da ich gerne noch andere Plattformen (RP2040, proMini 328p) unterstützen möchte.

[beegee3]

@lumapu 👍👍👍 super Update, insbesondere der neue Scheduler!!! (verstehe auch, warum die Ticker Funktion nicht passt 😒; vielleicht wird's irgendwann ja noch etwas, auch RP2040 und proMini 328p haben Hardware Timer)

Allerdings ist auch die Änderung "system clock was too fast" so nicht korrekt.
Es ist einfach nur mPrevMillis += (mDiffSeconds * 1000); (in der Version [d9290d9] stand da irrtümlich mPrevMillis statt mDiffSeconds). mDiffFraq ist also überflüssig und sollte komplett entfernt werden.

Noch ein paar Anmerkungen zum Scheduler:

• Divison kostet Zeit, daher erscheint es mir besser mit mDiffSeconds = 1; zu starten und das nur für mDiff >= 2000 anzupassen. In der Regel ist das nicht der Fall. Dann kann man auch die overflow Überprüfung da hineinstecken. Und ganz ignorieren würde ich den overflow auch nicht, im schlimmsten Fall verschenkt man dabei zu viel Zeit.
• mTimestamp sollte vor dem Aufruf von checkAt() aktualisiert werden.
• in checkEvery wird if((p->d.timeout--) == 0) expired = true; gerechnet.
  Es sollte aber if((p->d.timeout -= mDiffSeconds) == 0) expired = true; sein.

Vorschlag für die Scheduler loop:

void loop(void) {
    mMillis = millis();
    mDiff = mMillis - mPrevMillis;
    if (mDiff < 1000)
        return;
    
    mDiffSeconds = 1;
    if (mDiff < 2000)
        mPrevMillis += 1000;
    else {
        if (mMillis < mPrevMillis) { // overflow
            mDiff = mMillis;
            if (mDiff < 1000)
                return;
        }
        mDiffSeconds = mDiff / 1000;
        mPrevMillis += (mDiffSeconds * 1000);
    }
    mUptime += mDiffSeconds;
    if(0 != mTimestamp)
        mTimestamp += mDiffSeconds;
    checkEvery();
    checkAt();
    }
}

und ein Vorschlag für die NTP Abfrage:
wenn man tickNtpUpdate analog zu tickCalcSunrise mit once(...) anstelle von every(...) startet, kann man sich die Variablen mUpdateNtp und mInitNtp komlett sparen und tickMinute braucht man auch nicht mehr. Wenn getNtpTime noch den (Miß-)Erfolg zurückgibt, kann man entsprechend darauf reagieren, d.h.

void app::tickNtpUpdate(void) {
    uint32_t nxtTrig = 5;  // default: check again in 5 sec
    if (mWifi.getNtpTime())
        nxtTrig = 43200;    // check again in 12 h
    once(std::bind(&app::tickNtpUpdate, this), nxtTrig);
}
 
bool ahoywifi::getNtpTime(void) {
    //DPRINTLN(DBG_VERBOSE, F("wifi::getNtpTime"));
    IPAddress timeServer;
    uint8_t buf[NTP_PACKET_SIZE];
    uint8_t retry = 0;

    if (!mConnected)
        return(false);

    WiFi.hostByName(mConfig->ntp.addr, timeServer);
    mUdp.begin(mConfig->ntp.port);

    sendNTPpacket(timeServer);

    while(retry++ < 5) {
        int wait = 150;
        while(--wait) {
            if(NTP_PACKET_SIZE <= mUdp.parsePacket()) {
                uint64_t secsSince1900;
                mUdp.read(buf, NTP_PACKET_SIZE);
                secsSince1900  = (buf[40] << 24);
                secsSince1900 |= (buf[41] << 16);
                secsSince1900 |= (buf[42] <<  8);
                secsSince1900 |= (buf[43]      );

                *mUtcTimestamp = secsSince1900 - 2208988800UL; // UTC time
                DPRINTLN(DBG_INFO, "[NTP]: " + ah::getDateTimeStr(*mUtcTimestamp) + " UTC");
                return(true);
            } else
                delay(10);
        }
    }
    DPRINTLN(DBG_INFO, "[NTP]: getNtpTime failed");
    return(false);
}

Übrigens: bisher wurde mUtcTimestamp auf 0 gesetzt, wenn der timeServer mal nicht antwortet.
Außerdem wurde auch bei Erfolg die retry Schleife nicht sofort verlassen.

[beegee3]

@lumapu die Reaktionszeiten sind manchmal etwas träge. Wird deutlich besser, wenn in checkEvery und checkAt hinter
(p->d.c)(); jeweils noch ein yield(); steht.

[beegee3]

@lumapu 👍 vielen Dank für die Umsetzung. Nur die Anpassung in checkEvery fehlt noch (s.o.):
es wird bei else if((p->d.timeout--) == 0) expired = true; nur 1 Sekunde abgezogen. Wenn mDiffSeconds > 1 ist, passt das nicht. Ich würde es daher in else if((p->d.timeout -= mDiffSeconds) == 0) expired = true; ändern.

Und dann hat sich leider bei tickCalcSunrise ein Fehler bei der Mitternachtsberechnung eingeschlichen:
uint32_t nxtTrig = mTimestamp - ((mTimestamp + 1000) % 86400) + 86400; // next midnight
mTimestamp gibt Sekunden an, daher ist es 1 statt 1000 und eigentlich - 1, da vor der Klammer ja ein Minus steht.
Letztlich soll doch der Trigger 1 Sekunde nach nächstes Mitternacht ausgelöst werden, d.h. einfach:
uint32_t nxtTrig = mTimestamp - (mTimestamp % 86400) + 86400 + 1; // next midnight + 1 sec

[lumapu]

bei
else if((p->d.timeout -= mDiffSeconds) == 0) expired = true;
müssten wir dann noch prüfen, ob p->d.timeout größer oder gleich mDiffSeconds ist.

bei
uint32_t nxtTrig = mTimestamp - ((mTimestamp + 1000) % 86400) + 86400; // next midnight

ist es glaube ich ein Verständnisproblem. Der Trigger wurde bei mir je nach ESP (ich habe 3stk. am laufen) nur ab und zu am nächsten Tag aufgeführt. Manchmal hat er wohl schon um 23:59:59 getriggert wodurch die gleichen Ergebnisse rauskommen und nie wieder getriggert wird. Daher die +1000

[beegee3]

@lumapu

bei
else if((p->d.timeout -= mDiffSeconds) == 0) expired = true;
müssten wir dann noch prüfen, ob p->d.timeout größer oder gleich mDiffSeconds ist.

Das wird ja in der Zeile davor gemacht.
😲 merke gerade: da dort if(mDiffSeconds >= p->d.timeout) steht, muss in der else Anweisung
sogar immer mDiffSeconds < p->d.timeout gelten, d.h. man kann das gesamte

if ...
else if ... 
else ...

verkürzen zu

if(mDiffSeconds >= p->d.timeout)
    expired = true;
else {
    p->d.timeout-= mDiffSeconds;
    expired = false;
}

---

bei
uint32_t nxtTrig = mTimestamp - ((mTimestamp + 1000) % 86400) + 86400; // next midnight
...

Das Triggerproblem ist merkwürdig. Selbst das NTP Update sollte das nicht beeinflussen,
denn (mTimestamp % 86400) sind die Sekunden seit letzter Mitternacht, also
mTimestamp - (mTimestamp % 86400) die UTC Zeit von letzter Mitternacht.
Vielleicht lag es an dem fehlenden yield() in checkAt, aber das ist ja jetzt drin!

Nur so, wie jetzt getriggert wird, sind es 1000 Sekunden VOR Mitternacht!
Bsp.: mTimestamp = 1670922000 = 13.12.2020 09:00:00, dann ist
nxtTrig = 1670922000 - (1670923000 % 86400) + 86400 = 1670922000 - 33400 + 86400 = 1670975000 = 13.12.2022 23:43:20
Zu der Zeit (also immer noch am 13.12.) läuft das Update das nächste mal und da ergibt sich
nxtTrig = 1670975000 = 14.12.2022 23:43:20,
d.h. erst kurz vor Mitternacht am 14.12. kommt das Update für den 14.12. Bis dahin hat man noch die Werte vom 13.12.

Sollen es (warum auch immer) 1000 Sekunden NACH Mitternacht sein, muss 1000 abgezogen werden:
uint32_t nxtTrig = mTimestamp - ((mTimestamp - 1000) % 86400) + 86400;
Dann wird das Update täglich um 00:16:40 durchgeführt (mein Vorschlag oben war mit 1 Sekunde nach Mitternacht halt ein bisschen früher 😄).

[beegee3]

Sorry, hab' noch zwei Punkte:

• die ahoywifi::loop ist ohne wesentliche Funktion, da in der letzten Zeile mCnt = 0; steht. So kann bei einem disconnected nie ein reconnect erfolgen. Einzig zum Start, wo noch keine Wifi Verbindung steht, wird bei jedem loop Aufruf ein unnötiges delay(100) ausgeführt, was den Start extrem verzögert und evtl. das Connect ganz verhindert. Statt über mConnect könnte man ein bool mReconnect einführen, das zu Beginn und nach erfolgreichem Connect auf false steht und nur beim ungewollten Disconnect auf true gesetzt wird. Die loop() Funktion wäre dann

void ahoywifi::loop() {
    #if !defined(AP_ONLY)
    if(mReconnect) {
        delay(100);
        mCnt++;
        if((mCnt % 50) == 0)
            WiFi.disconnect();
        else if((mCnt % 60) == 0) {
            WiFi.reconnect();
            mCnt = 0;
        }
    }
    #endif
}

In onConnect und onWiFiEvent ... SYSTEM_EVENT_STA_GOT_IP: ist dann mConnect = true; mReconnect = false; mCnt = 0;
mInitNtp braucht man gar nicht mehr, dafür hat man ja tickNtpUpdate!

• die Sekunden in der Web Ansicht laufen ruhiger ohne else in dertick() Funktion von index.html.
  Und erst die Anzeige, dann die (Zeit brauchende) Ajax Abfrage:

function tick() {
    if(0 != ts)
        document.getElementById("date").innerHTML = (new Date((++ts) * 1000)).toLocaleString('de-DE');
    if(++tickCnt >= 10) {
        tickCnt = 0;
        getAjax('/api/index', parse);
    }
}

[lumapu]

vielen Dank für deine Beiträge, ich habe hoffentlich alles integriert und umgesetzt. Ich freue mich immer wieder hier über gute Verbesserungen zu lesen, weiter so :-)

[beegee3]

👍👍👍 WOW! Klasse Idee und Umsetzung Interface Class und RestApi! Macht es auch einfacher, den Code zu lesen.

Nur bei ahoywifi ist nur noch nicht alles integriert:

• bei onDisconnect fehlt noch der Counter reset: mCnt = 0;
• bei onWiFiEvent
case SYSTEM_EVENT_STA_GOT_IP: fehlt mReconnect = false;
case SYSTEM_EVENT_STA_DISCONNECTED: fehlt mReconnect = true; und mCnt = 0;

---

Zwei Anmerkungen (dann ist auch gut 😄):
Wieso steht im Scheduler checkEvery - 1 bei p->d.timeout = p->d.reload - 1;?
Da soll doch der Timeout zurückgesetzt werden, also auf den in reload gespeicherten Originalwert.
Sonst wird doch z.B. ein 5 Sekunden Ticker bereits nach 4 Sekunden erneut ausgelöst.

In der tick() Funktion von index.html hatte ich bewusst (++ts) statt (ts+tickCnt) vorgeschlagen.
Für den Fall, dass getAjax(...) mal keine Rückmeldung bekommt (🤔 kann das eigentlich passieren?),
kommt die Zeitanzeige so nicht in eine 10 Sekunden Schleife (ewig grüßt das Murmeltier).

[lumapu]

Wieso steht im Scheduler checkEvery - 1 bei p->d.timeout = p->d.reload - 1;?
Da soll doch der Timeout zurückgesetzt werden, also auf den in reload gespeicherten Originalwert.
Sonst wird doch z.B. ein 5 Sekunden Ticker bereits nach 4 Sekunden erneut ausgelöst.

Ich habe die Scheduler Klasse per einzeln simuliert und gemerkt, wenn ich die -1 weglasse die once genau eine Sekunde länger dauern als gewünscht. Das liegt daran, dass beim Reload schon der erste Zyklus gezählt werden muss. Ich bin mir hier aber auch nicht ganz sicher was richtig ist. Ich kann bei Gelegenheit die Simulation nochmal rauskramen.

[beegee3]

Ich habe die Scheduler Klasse per einzeln simuliert und gemerkt, wenn ich die -1 weglasse die once genau eine Sekunde länger dauern als gewünscht. Das liegt daran, dass beim Reload schon der erste Zyklus gezählt werden muss.

Ok, ich bin da Theoretiker:
Bsp.: ein every 5 sec Ticker startet mit timeout = 5. Mit jeder sec wird 1 abgezogen -> (wie gewünscht) nach 5 sec auf 0
-> zug. Funktion wird aufgerufen -> timeout wird auf 5-1=4 gesetzt -> ... usw. ... -> nach 4 sec auf 0 😞 (nicht gewünscht)
Es kann ja passieren, dass mDiff knapp unter 1000 ist, dann wird erst beim nächsten loop ausgelöst, was ggfs. als 1 sec Verzögerung wahrgenommen wird?!

[beegee3]

👏Kommunikation mit einzelnen Invertern an- und abschalten
War allerdings nach dem Update irritiert, dass der Inverter zunächst stumm blieb. Im Nachhinein klar, da in den Settings dieses neue Feature nicht vorhanden war und die Kommunikation als 'disabled' interpretiert wurde.
Das sollte vielleicht irgendwo dokumentiert werden, oder es wird einfach das fehlende Setting als 'enabled' definiert:
settings.h, void jsonIv(...) cfg->enabled = (bool)(obj[F("en")] | true);

[beegee3]

noch etwas:
wenn in app.h, setTimestamp(...) die Zeile mUpdateNtp = true; durch mWifi.getNtpTime(); ersetzt wird, braucht man die Variable mUpdateNtp nicht mehr und ist insbesondere die if (mUpdateNtp) Prüfung in tickSecond(void) los.
Wenn dort die if (mShouldReboot) Prüfung direkt nach setRebootFlag() verlegt werden kann, ist die ganze Funktion tickSecond(void) überflüssig.

[beegee3]

Vorschlag zu setRebootFlag():

        void setRebootFlag() {
            once(std::bind(&app::tickReboot, this), 1);
        }

        void tickReboot(void) {
            DPRINTLN(DBG_INFO, F("Rebooting..."));
            ESP.restart();
        }

Zusammen mit dem setTimestamp(...) Vorschlag (s.o.) können dann mUpdateNtp, mShouldReboot und tickSecond(void) gelöscht werden.

[lumapu]

@beegee3 danke für deine Ideen👍, ich habe es mit tickReboot umgesetzt habe es für mMqtt.sendDiscoveryConfig() auch mit once gebaut, gefällt mir besser

[beegee3]

👌 Ein letzter Hinweis: in app::loop, letzte Anweisung: mMqtt.loop(); bitte nur für !defined(AP_ONLY)

[lumapu]

bitte nicht aufhören, finde die Verbesserungen Klasse!

[beegee3]

@lumapu Danke sehr. Manchmal finde ich meine eigenen Kommentare sehr pedantisch. Denke dann aber, vielleicht kann doch das eine oder andere dazu beitragen, AHOY noch besser zu machen. Der größte Dank gebührt aber dir für deine Arbeit!!!

[knickohr]

Nun ja, ich habe das Gefühl das durch Deine Vorschläge und Anpassungen das Ganze stabiler wird. Es gab Tage da bootete mir der ESP mehrmals täglich. Seit dem läuft er stabiler, keine Selbstständigen Reboots mehr, allerdings sind es selten mehr als 2 Tage bevor wieder ein neuer Update rein kommt 😅

[beegee3]

OK, Danke. Das hier diskutierte ist aber entweder umgesetzt, oder mittlerweile obsolet. Daher schließe ich das hier ab und wünsche vorsorglich schon mal allen, die hier mitgemacht haben, frohe Weihnachten und einen guten Rutsch 🎅 🎄 🎁
...
nicht ohne darauf hinzuweisen, dass ich heute noch einen neuen Vorschlag 'issue' aufmache 😄.