☠️ Finally addresses the heap corruption reboots with mutexes

- adds lock_ mutex in the Publishable class - Publishable now gets the log buffer and ‘logme’ - adds static log() function
opensolarproject · Jun 25, 2020 · d7e88d5 · d7e88d5
1 parent 48239dd
commit d7e88d5
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 46 deletions.
diff --git a/lib/MPPTLib/publishable.cpp b/lib/MPPTLib/publishable.cpp
@@ -37,7 +37,7 @@ template<> void Pub<String*>::load(Preferences&p) {
   (*value) = String(buf);
 }
 
-Publishable::Publishable() {
+Publishable::Publishable() : lock_(xSemaphoreCreateCounting( 10, 0 )) {
   add("save", [this](String s){
     return str("saved %d prefs", this->savePrefs());
   }).hide();
@@ -48,6 +48,38 @@ Publishable::Publishable() {
   add("list", [this](String s){ printHelp(); return ""; }).hide();
 }
 
+void Publishable::log(const String &s) {
+  Serial.println(s);
+  if (xSemaphoreTake(lock_, (TickType_t) 10) == pdTRUE) {
+    logPub_.push_back(s);
+    xSemaphoreGive(lock_);
+  }
+}
+bool Publishable::popLog(String *s) {
+  if (xSemaphoreTake(lock_, (TickType_t) 10) == pdTRUE) {
+    bool got = logPub_.size() > 0;
+    if (got) (*s) = logPub_.pop_front();
+    xSemaphoreGive(lock_);
+    return got;
+  }
+  return false;
+}
+void Publishable::logNote(const String &s) {
+  if (xSemaphoreTake(lock_, (TickType_t) 10) == pdTRUE) {
+    logNote_ += s;
+    xSemaphoreGive(lock_);
+  }
+}
+String Publishable::popNotes() {
+  if (xSemaphoreTake(lock_, (TickType_t) 10) == pdTRUE) {
+    String ret = logNote_;
+    logNote_ = "";
+    xSemaphoreGive(lock_);
+    return ret;
+  }
+  return "";
+}
+
 PubItem& Publishable::add(PubItem* p) { items_[p->key] = p; return *p; }
 PubItem& Publishable::add(String k, double &v, int p) { return add(new Pub<double*>(k,&v,p)); }
 PubItem& Publishable::add(String k, float  &v, int p) { return add(new Pub<float*> (k,&v,p)); }

diff --git a/lib/MPPTLib/publishable.h b/lib/MPPTLib/publishable.h
@@ -3,6 +3,7 @@
 #include <functional>
 #include <map>
 #include <list>
+#include "utils.h"
 
 class PubSubClient;
 class WebServer;
@@ -56,10 +57,18 @@ class Publishable {
   void clearDirty();
   void printHelp() const;
 
+  void log(const String &);
+  bool popLog(String*);
+  void logNote(const String &); //adds note to next status
+  String popNotes();
+  // void log(const char *fmtStr, ...);
 
 private:
   PubItem& add(PubItem*);
   std::map<String, PubItem*> items_;
   int defaultPeriod_ = 12000;
+  String logNote_;
+  CircularArray<String, 16> logPub_;
+  SemaphoreHandle_t lock_;
 };
 
diff --git a/lib/MPPTLib/solar.cpp b/lib/MPPTLib/solar.cpp
@@ -23,16 +23,16 @@ uint32_t nextPSUpdate_ = 0, nextSolarAdjust_ = 1000;
 uint32_t lastPSUpdate_ = 0, lastPSUSuccess_ = 0;
 uint32_t nextAutoSweep_ = 0, lastAutoSweep_ = 0;
 double newDesiredCurr_ = 0;
-String logme;
 extern const String updateIndex;
 
 void Solar::setup() {
   Serial.begin(115200);
   Serial.setTimeout(10); //very fast, need to keep the ctrl loop running
+  addLogger(&pub_); //sets global context
   delay(100);
   uint64_t chipid = ESP.getEfuseMac();
-  Serial.printf("startup, ID %08llX %04X\n", chipid, (uint16_t)(chipid >> 32));
-  Serial2.begin(4800, SERIAL_8N1, 16, 17, false, 1000);
+  log(str("startup, ID %08llX %04X\n", chipid, (uint16_t)(chipid >> 32)));
+  Serial2.begin(4800, SERIAL_8N1, -1, -1, false, 1000);
   analogSetCycles(32);
 
   WiFi.onEvent([](WiFiEvent_t event, WiFiEventInfo_t info){
@@ -121,10 +121,10 @@ void Solar::setup() {
 
   if (!psu_.begin())
     log("PSU begin failed");
-  newDesiredCurr_ = currFilt_ = psu_.outCurr_;
+  newDesiredCurr_ = currFilt_ = psu_.limitCurr_ = psu_.outCurr_;
   log(str("startup current is %0.3fAdes/%0.3fAfilt/%0.3fAout", newDesiredCurr_, currFilt_, psu_.outCurr_));
   nextAutoSweep_ = millis() + 10000;
-  Serial.println("finished setup");
+  log("finished setup");
   log("OSPController Version " GIT_VERSION);
 }
 
@@ -136,20 +136,20 @@ void Solar::doConnect() {
       String hostname = str("mpptESP-%02X", chipid & 0xff);
       WiFi.setHostname(hostname.c_str());
       if (WiFi.waitForConnectResult() == WL_CONNECTED) {
-        Serial.println("Wifi connected! hostname: " + hostname);
-        Serial.println("IP: " + WiFi.localIP().toString());
+        log("Wifi connected! hostname: " + hostname);
+        log("IP: " + WiFi.localIP().toString());
         MDNS.begin("mppt");
         MDNS.addService("http", "tcp", 80);
         server_.begin();
       }
-    } else Serial.println("no wifiap or wifipass set!");
+    } else log("no wifiap or wifipass set!");
   }
   if (WiFi.isConnected() && !db_.client.connected()) {
     if (db_.serv.length() && db_.feed.length()) {
-      Serial.println("Connecting MQTT to " + db_.user + "@" + db_.serv);
+      log("Connecting MQTT to " + db_.user + "@" + db_.serv);
       db_.client.setServer(db_.getEndpoint().c_str(), db_.getPort());
       if (db_.client.connect("MPPT", db_.user.c_str(), db_.pass.c_str())) {
-        Serial.println("PubSub connect success! " + db_.client.state());
+        log("PubSub connect success! " + db_.client.state());
         auto pubs = pub_.items(true);
         for (auto i : pubs)
           if (i->pref_)
@@ -167,7 +167,7 @@ String SPoint::toString() const {
 void Solar::applyAdjustment() {
   if (newDesiredCurr_ != psu_.limitCurr_) {
     if (psu_.setCurrent(newDesiredCurr_))
-      logme += str("[adjusting %0.2fA (from %0.2fA)] ", newDesiredCurr_ - psu_.limitCurr_, psu_.limitCurr_);
+      pub_.logNote(str("[adjusting %0.2fA (from %0.2fA)] ", newDesiredCurr_ - psu_.limitCurr_, psu_.limitCurr_));
     else log("error setting current");
     delay(50);
     psu_.readCurrent();
@@ -211,7 +211,7 @@ void Solar::doSweepStep() {
   int collapsedPoints = 0;
   for (int i = 0; i < sweepPoints_.size(); i++)
     if (sweepPoints_[i].collapsed) collapsedPoints++;
-  if (isCollapsed) logme += str("COLLAPSED[%d] ", collapsedPoints);
+  if (isCollapsed) pub_.logNote(str("COLLAPSED[%d] ", collapsedPoints));
 
   if (isCollapsed && collapsedPoints >= 2) { //great, sweep finished
     int maxIndex = 0;
@@ -255,8 +255,8 @@ bool Solar::hasCollapsed() const {
     return true;
   float vunder = psu_.outVolt_ / psu_.limitVolt_;
   float cunder = psu_.outCurr_ / psu_.limitCurr_;
-  if (psu_.debug_) Serial.printf("hasCollapsed under[v%0.3f c%0.3f] PSU[%0.3fV %0.3fVlim %0.3fA %0.3fClim]",
-      vunder, cunder, psu_.outVolt_, psu_.limitVolt_,psu_.outCurr_, psu_.limitCurr_);
+  if (psu_.debug_) log(str("hasCollapsed under[v%0.3f c%0.3f] PSU[%0.3fV %0.3fVlim %0.3fA %0.3fClim]",
+      vunder, cunder, psu_.outVolt_, psu_.limitVolt_,psu_.outCurr_, psu_.limitCurr_));
   if (psu_.limitCurr_ > 1.5 && (vunder < 0.9) && (cunder < 0.7) && (inVolt_ < (psu_.outVolt_ * 1.25)))
     return true;
   return false;
@@ -278,7 +278,7 @@ void Solar::loop() {
       if (error > 0.3 || (-error > 0.2)) { //adjustment deadband, more sensitive when needing to ramp down
         newDesiredCurr_ = min(psu_.limitCurr_ + dcurr, currentCap_);
         if (error < 0.6) { //ramp down, quick!
-          logme += "[QUICK] ";
+          pub_.logNote("[QUICK] ");
           nextSolarAdjust_ = now;
         }
       }
@@ -338,7 +338,7 @@ void Solar::loop() {
           applyAdjustment();
     }
     if (collapses_.size() && (millis() - collapses_.front()) > (5 * 60000)) { //5m age
-      logme += str("[clear collapse (%ds ago)]", (now - collapses_.pop_front())/1000);
+      pub_.logNote(str("[clear collapse (%ds ago)]", (now - collapses_.pop_front())/1000));
       pub_.setDirty("collapses");
     }
     heap_caps_check_integrity_all(true);
@@ -412,19 +412,18 @@ void Solar::publishTask() {
           wins += db_.client.publish((db_.feed + "/" + (i->pref_? "prefs/":"") + i->key).c_str(), i->toString().c_str(), true)? 1 : 0;
           if (i->pref_) ignoreSubsUntil_ = now + 3000;
         }
-        logme += str("[published %d] ", wins);
+        pub_.logNote(str("[published %d] ", wins));
         pub_.clearDirty();
       } else {
-        logme += "[pub disconnected] ";
+        pub_.logNote("[pub disconnected] ");
         doConnect();
       }
       heap_caps_check_integrity_all(true);
       lastpub = now;
     }
-    if (db_.client.connected() && logPub_.size()) {
-      String s = logPub_.pop_front();
+    String s;
+    while (db_.client.connected() && pub_.popLog(&s))
       db_.client.publish((db_.feed + "/log").c_str(), s.c_str(), false);
-    }
     db_.client.loop();
     pub_.poll(&Serial);
     server_.handleClient();
@@ -436,15 +435,9 @@ void Solar::printStatus() {
   String s = state_;
   s.toUpperCase();
   s += str(" %0.1fVin -> %0.2fWh <%0.2fV out %0.2fA %den> ", inVolt_, wh_, psu_.outVolt_, psu_.outCurr_, psu_.outEn_);
-  s += logme;
-  logme = ""; //clear
-  Serial.println(s);
-  if (psu_.debug_) logPub_.push_back(s);
-}
-
-void Solar::log(String s) {
-  Serial.println(s);
-  logPub_.push_back(s);
+  s += pub_.popNotes();
+  if (psu_.debug_) log(s);
+  else Serial.println(s);
 }
 
 void Solar::backoff(String reason) {

diff --git a/lib/MPPTLib/solar.h b/lib/MPPTLib/solar.h
@@ -33,8 +33,6 @@ class Solar {
   bool hasCollapsed() const;
   int getCollapses() const;
 
-  void log(String s);
-  // void log(const char *fmtStr, ...);
   void backoff(String reason);
   void setState(const String state, String reason="");
 
@@ -49,7 +47,6 @@ class Solar {
   int autoSweep_ = 10 * 60; //every 10m
   float vadjust_ = 116.50;
   CircularArray<SPoint, 10> sweepPoints_; //size here is important, larger == more stable setpoint
-  CircularArray<String, 16> logPub_;
   String wifiap, wifipass;
   uint32_t ignoreSubsUntil_ = 0;
   int8_t backoffLevel_ = 0;

diff --git a/lib/MPPTLib/utils.cpp b/lib/MPPTLib/utils.cpp
@@ -1,4 +1,5 @@
-#include "Arduino.h"
+#include <Arduino.h>
+#include "publishable.h"
 #include "utils.h"
 
 
@@ -15,7 +16,7 @@ String str(const char *fmtStr, ...) {
   va_end(arg_ptr);
   return String(buf);
 }
-String str(std::string s) {
+String str(const std::string &s) {
   return String(s.c_str());
 }
 String str(bool v) {
@@ -24,6 +25,9 @@ String str(bool v) {
 
 // float lifepo4_soc[] = {13.4, 13.3, 13.28, 13.};
 
+Publishable* pub_; //static
+void log(const String &s) { pub_->log(s); }
+void addLogger(Publishable* p) { pub_ = p; }
 
 
 PowerSupply::PowerSupply(Stream &port) : _port(&port), debug_(false) { }
@@ -40,6 +44,7 @@ bool PowerSupply::doUpdate() {
   if (res && !limitVolt_) {
     handleReply(cmdReply("arc")); //read current limit
     handleReply(cmdReply("arv")); //read voltage limit
+    log(str("finished PSU begin, got %0.3fV %0.3fA limits\n", limitVolt_, limitCurr_));
   }
   return res;
 }
@@ -50,7 +55,7 @@ bool PowerSupply::getOutputEnabled() { return handleReply(cmdReply("aro")); }
 
 template<typename T>void setCheck(T &save, float in, float max) { if (in < max) save = in; }
 
-bool PowerSupply::handleReply(String msg) {
+bool PowerSupply::handleReply(const String &msg) {
   if (!msg.length()) return false;
   String hdr = msg.substring(0, 3);
   String body = msg.substring(3);
@@ -60,7 +65,7 @@ bool PowerSupply::handleReply(String msg) {
   else if (hdr == "#rv") setCheck(limitVolt_, body.toFloat() / 100.0, 80);
   else if (hdr == "#ra") setCheck(limitCurr_, body.toFloat() / 100.0, 15);
   else {
-    Serial.println("got unknown msg > '" + hdr + "' / '" + body + "'");
+    log("PSU got unknown msg > '" + hdr + "' / '" + body + "'");
     return false;
   }
   return true;
@@ -70,9 +75,9 @@ void PowerSupply::flush() {
   _port->flush();
 }
 
-String PowerSupply::cmdReply(String cmd) {
+String PowerSupply::cmdReply(const String &cmd) {
   _port->print(cmd + "\r\n");
-  if (debug_) Serial.println("> " + cmd + "CRLF");
+  if (debug_) log("PSU > " + cmd + "CRLF");
   String reply;
   uint32_t start = millis();
   char c;
@@ -83,10 +88,10 @@ String PowerSupply::cmdReply(String cmd) {
     String debug = reply;
     debug.replace("\r", "CR");
     debug.replace("\n", "NL");
-    Serial.println("< " + debug);
+    log("PSU < " + debug);
   }
   if (!reply.length() && debug_ && _port->available())
-    Serial.printf("nothing read.. stuff available!? %d", _port->available());
+    log("PSU nothing read.. stuff available!? " + String(_port->available()));
   reply.trim();
   return reply;
 }

diff --git a/lib/MPPTLib/utils.h b/lib/MPPTLib/utils.h
@@ -1,5 +1,7 @@
 #pragma once
-#include "Arduino.h"
+#include <Arduino.h>
+
+class Publishable;
 
 class PowerSupply {
     Stream *_port;
@@ -12,7 +14,7 @@ class PowerSupply {
     PowerSupply(Stream &port);
     bool begin();
 
-    String cmdReply(String cmd);
+    String cmdReply(const String &cmd);
     bool setVoltage(float);
     bool setCurrent(float);
     bool enableOutput(bool);
@@ -24,16 +26,19 @@ class PowerSupply {
     void flush();
 
   private:
-    bool handleReply(String);
+    bool handleReply(const String &);
     String fourCharStr(uint16_t input);
 };
 
+void log(const String &);
+void addLogger(Publishable*);
+
 float mapfloat(long x, long in_min, long in_max, long out_min, long out_max);
 
 extern const char* adafruitRootCert;
 
 String str(const char *fmtStr, ...);
-String str(std::string s);
+String str(const std::string &s);
 String str(bool v);