Merge pull request #765 from SignalK/unit_tests

Unit tests and memory safety

platformio.ini

@@ -66,6 +66,7 @@ build_flags =
 ;debug_tool = esp-prog
 ;debug_init_break = tbreak setup
 
+test_build_src = true
 check_tool = clangtidy
 check_flags =
   clangtidy: --fix --format-style=file --config-file=.clang-tidy

src/sensesp/controllers/smart_switch_controller.cpp

@@ -95,7 +95,7 @@ SmartSwitchController::SyncPath::SyncPath() {}
 SmartSwitchController::SyncPath::SyncPath(String sk_sync_path)
     : sk_sync_path_{sk_sync_path} {
   ESP_LOGD(__FILENAME__, "DoubleClick will also sync %s", sk_sync_path.c_str());
-  this->put_request_ = new BoolSKPutRequest(sk_sync_path, "", false);
+  this->put_request_ = std::make_shared<BoolSKPutRequest>(sk_sync_path, "", false);
 }
 
 }  // namespace sensesp

src/sensesp/controllers/smart_switch_controller.h

@@ -1,10 +1,10 @@
 #ifndef _smart_switch_controller_h
 #define _smart_switch_controller_h
 
-#include "sensesp/ui/config_item.h"
 #include "sensesp/signalk/signalk_put_request.h"
 #include "sensesp/system/valueconsumer.h"
 #include "sensesp/transforms/click_type.h"
+#include "sensesp/ui/config_item.h"
 
 namespace sensesp {
 
@@ -74,8 +74,8 @@ class SmartSwitchController : public BooleanTransform,
   /// Used to store configuration internally.
   class SyncPath {
    public:
-    BoolSKPutRequest* put_request_;
     String sk_sync_path_;
+    std::shared_ptr<BoolSKPutRequest> put_request_;
 
     SyncPath();
     SyncPath(String sk_sync_path);

src/sensesp/net/http_server.cpp

@@ -74,8 +74,8 @@ esp_err_t HTTPServer::dispatch_request(httpd_req_t* req) {
 
   if (auth_required) {
     bool success;
-    success =
-        authenticate_request(authenticator_, call_request_dispatcher, req);
+    success = authenticate_request(authenticator_.get(),
+                                   call_request_dispatcher, req);
     if (!success) {
       // Authentication failed; do not continue but return success.
       // The client already received a 401 response.

src/sensesp/net/http_server.h

@@ -7,6 +7,7 @@
 #include <esp_http_server.h>
 #include <functional>
 #include <list>
+#include <memory>
 
 #include "WiFi.h"
 #include "sensesp/net/http_authenticator.h"
@@ -61,17 +62,16 @@ class HTTPServer : virtual public FileSystemSaveable {
  public:
   HTTPServer(int port = HTTP_DEFAULT_PORT,
              const String& config_path = "/system/httpserver")
-      : FileSystemSaveable(config_path),
-      config_(HTTPD_DEFAULT_CONFIG()) {
+      : FileSystemSaveable(config_path), config_(HTTPD_DEFAULT_CONFIG()) {
     config_.server_port = port;
     config_.stack_size = HTTP_SERVER_STACK_SIZE;
     config_.max_uri_handlers = 20;
     config_.uri_match_fn = httpd_uri_match_wildcard;
     String auth_realm_ = "Login required for " + SensESPBaseApp::get_hostname();
     load();
     if (auth_required_) {
-      authenticator_ =
-          new HTTPDigestAuthenticator(username_, password_, auth_realm_);
+      authenticator_ = std::unique_ptr<HTTPDigestAuthenticator>(
+          new HTTPDigestAuthenticator(username_, password_, auth_realm_));
     }
     if (singleton_ == nullptr) {
       singleton_ = this;
@@ -111,7 +111,11 @@ class HTTPServer : virtual public FileSystemSaveable {
       MDNS.addService("http", "tcp", 80);
     });
   };
-  ~HTTPServer() {}
+  ~HTTPServer() {
+    if (singleton_ == this) {
+      singleton_ = nullptr;
+    }
+  }
 
   void stop() { httpd_stop(server_); }
 
@@ -148,6 +152,10 @@ class HTTPServer : virtual public FileSystemSaveable {
   }
 
   void add_handler(HTTPRequestHandler* handler) {
+    handlers_.push_back(std::make_shared<HTTPRequestHandler>(*handler));
+  }
+
+  void add_handler(std::shared_ptr<HTTPRequestHandler> handler) {
     handlers_.push_back(handler);
   }
 
@@ -160,7 +168,7 @@ class HTTPServer : virtual public FileSystemSaveable {
   String username_;
   String password_;
   bool auth_required_ = false;
-  HTTPAuthenticator* authenticator_ = nullptr;
+  std::unique_ptr<HTTPAuthenticator> authenticator_;
 
   bool authenticate_request(HTTPAuthenticator* auth,
                             std::function<esp_err_t(httpd_req_t*)> handler,
@@ -184,7 +192,7 @@ class HTTPServer : virtual public FileSystemSaveable {
    */
   bool handle_captive_portal(httpd_req_t* req);
 
-  std::list<HTTPRequestHandler*> handlers_;
+  std::list<std::shared_ptr<HTTPRequestHandler>> handlers_;
 
   friend esp_err_t call_request_dispatcher(httpd_req_t* req);
 };

src/sensesp/net/networking.cpp

@@ -92,8 +92,14 @@ Networking::Networking(String config_path, String client_ssid,
     dns_server_->setErrorReplyCode(DNSReplyCode::NoError);
     dns_server_->start(53, "*", WiFi.softAPIP());
 
-    event_loop()->onRepeat(
-        1, [this]() { dns_server_->processNextRequest(); });
+    event_loop()->onRepeat(1, [this]() { dns_server_->processNextRequest(); });
+  }
+}
+
+Networking::~Networking() {
+  if (dns_server_) {
+    dns_server_->stop();
+    delete dns_server_;
   }
 }
 
@@ -280,13 +286,9 @@ void Networking::reset() {
   WiFi.begin("0", "0", 0, nullptr, false);
 }
 
-WiFiStateProducer* WiFiStateProducer::instance_ = nullptr;
-
 WiFiStateProducer* WiFiStateProducer::get_singleton() {
-  if (instance_ == nullptr) {
-    instance_ = new WiFiStateProducer();
-  }
-  return instance_;
+  static WiFiStateProducer instance;
+  return &instance;
 }
 
 void Networking::start_wifi_scan() {

src/sensesp/net/networking.h

@@ -6,9 +6,9 @@
 #include <WiFi.h>
 
 #include "sensesp/net/wifi_state.h"
-#include "sensesp/system/serializable.h"
 #include "sensesp/system/observablevalue.h"
 #include "sensesp/system/resettable.h"
+#include "sensesp/system/serializable.h"
 #include "sensesp/system/valueproducer.h"
 #include "sensesp_base_app.h"
 
@@ -97,8 +97,6 @@ class WiFiStateProducer : public ValueProducer<WiFiState> {
     ESP_LOGI(__FILENAME__, "Disconnected from wifi.");
     this->emit(WiFiState::kWifiDisconnected);
   }
-
-  static WiFiStateProducer* instance_;
 };
 
 /**
@@ -241,6 +239,8 @@ class Networking : virtual public FileSystemSaveable,
  public:
   Networking(String config_path, String client_ssid = "",
              String client_password = "");
+  ~Networking();
+
   virtual void reset() override;
 
   virtual bool to_json(JsonObject& doc) override;

src/sensesp/net/web/app_command_handler.cpp

@@ -1,11 +1,13 @@
 #include "app_command_handler.h"
 
+#include <memory>
+
 #include <esp_http_server.h>
 
 namespace sensesp {
 
 void add_scan_wifi_networks_handlers(HTTPServer* server) {
-  HTTPRequestHandler* scan_wifi_networks_handler = new HTTPRequestHandler(
+  auto scan_wifi_networks_handler = std::make_shared<HTTPRequestHandler>(
       1 << HTTP_POST, "/api/wifi/scan", [](httpd_req_t* req) {
         auto networking = SensESPApp::get()->get_networking();
         networking->start_wifi_scan();
@@ -17,7 +19,7 @@ void add_scan_wifi_networks_handlers(HTTPServer* server) {
 
   server->add_handler(scan_wifi_networks_handler);
 
-  HTTPRequestHandler* scan_results_handler = new HTTPRequestHandler(
+  auto scan_results_handler = std::make_shared<HTTPRequestHandler>(
       1 << HTTP_GET, "/api/wifi/scan-results", [](httpd_req_t* req) {
         auto networking = SensESPApp::get()->get_networking();
         std::vector<WiFiNetworkInfo> ssid_list;

src/sensesp/net/web/config_handler.cpp

@@ -171,6 +171,7 @@ void add_config_put_handler(HTTPServer* server) {
         // parse the content as JSON
         JsonDocument doc;
         DeserializationError error = deserializeJson(doc, payload);
+        delete[] payload;
         if (error) {
           ESP_LOGE("ConfigHandler", "Error parsing JSON payload: %s",
                    error.c_str());

src/sensesp/sensors/analog_input.cpp

@@ -13,7 +13,7 @@ AnalogInput::AnalogInput(uint8_t pin, unsigned int read_delay,
       pin{pin},
       read_delay{read_delay},
       output_scale{output_scale} {
-  analog_reader_ = new AnalogReader(pin);
+  analog_reader_ = std::unique_ptr<AnalogReader>(new AnalogReader(pin));
   load();
 
   if (this->analog_reader_->configure()) {

src/sensesp/sensors/analog_input.h

@@ -1,6 +1,7 @@
 #ifndef SENSESP_SENSORS_ANALOG_INPUT_H_
 #define SENSESP_SENSORS_ANALOG_INPUT_H_
 
+#include <memory>
 #include "analog_reader.h"
 #include "sensesp/ui/config_item.h"
 #include "sensor.h"
@@ -47,11 +48,11 @@ class AnalogInput : public FloatSensor {
   virtual bool to_json(JsonObject& root) override;
   virtual bool from_json(const JsonObject& config) override;
 
- private:
+ protected:
   uint8_t pin{};
   unsigned int read_delay;
   float output_scale;
-  BaseAnalogReader* analog_reader_;
+  std::unique_ptr<BaseAnalogReader> analog_reader_{};
   void update();
 };
 

src/sensesp/sensors/system_info.h

@@ -1,6 +1,7 @@
 #ifndef SENSESP_SENSORS_SYSTEM_INFO_H_
 #define SENSESP_SENSORS_SYSTEM_INFO_H_
 
+#include <memory>
 #include <Arduino.h>
 #include <elapsedMillis.h>
 
@@ -19,7 +20,7 @@ void connect_system_info_sensor(ValueProducer<T>* sensor, String prefix, String
   String hostname = hostname_obs->get();
   String path = prefix + hostname + "." + name;
 
-  auto* sk_output = new SKOutput<T>(path);
+  auto sk_output = std::make_shared<SKOutput<T>>(path);
 
   // connect an observer to hostname to change the output path
   // if the hostname is changed

src/sensesp/signalk/signalk_emitter.h

@@ -42,7 +42,7 @@ class SKEmitter : virtual public Observable {
    * returned.
    * @see add_metadata()
    */
-  virtual SKMetadata* get_metadata() { return NULL; }
+  virtual SKMetadata* get_metadata() { return nullptr; }
 
   /**
    * Adds this emitter's Signal K meta data to the specified

src/sensesp/signalk/signalk_output.h

@@ -1,8 +1,10 @@
 #ifndef SENSESP_SIGNALK_SIGNALK_OUTPUT_H_
 #define SENSESP_SIGNALK_SIGNALK_OUTPUT_H_
 
-#include "sensesp/ui/config_item.h"
+#include <memory>
+
 #include "sensesp/transforms/transform.h"
+#include "sensesp/ui/config_item.h"
 #include "signalk_emitter.h"
 
 namespace sensesp {
@@ -29,7 +31,10 @@ class SKOutput : public SKEmitter, public SymmetricTransform<T> {
    * metadata to report (or if the path is already an official part of the
    * Signal K specification)
    */
-  SKOutput(String sk_path, String config_path = "", SKMetadata* meta = NULL)
+  SKOutput(String sk_path, String config_path = "", SKMetadata* meta = nullptr)
+      : SKOutput(sk_path, config_path, std::make_shared<SKMetadata>(*meta)) {}
+
+  SKOutput(String sk_path, String config_path, std::shared_ptr<SKMetadata> meta)
       : SKEmitter(sk_path), SymmetricTransform<T>(config_path), meta_{meta} {
     this->load();
   }
@@ -64,12 +69,14 @@ class SKOutput : public SKEmitter, public SymmetricTransform<T> {
    * method of setting the metadata (the first being a parameter
    * to the constructor).
    */
-  virtual void set_metadata(SKMetadata* meta) { this->meta_ = meta; }
+  virtual void set_metadata(SKMetadata* meta) {
+    this->meta_ = std::make_shared<SKMetadata>(*meta);
+  }
 
-  virtual SKMetadata* get_metadata() override { return meta_; }
+  virtual SKMetadata* get_metadata() override { return meta_.get(); }
 
  protected:
-  SKMetadata* meta_;
+  std::shared_ptr<SKMetadata> meta_;
 };
 
 template <typename T>

src/sensesp/system/base_button.h

@@ -1,6 +1,7 @@
 #ifndef SENSESP_SRC_SENSESP_SYSTEM_BASE_BUTTON_H_
 #define SENSESP_SRC_SENSESP_SYSTEM_BASE_BUTTON_H_
 
+#include <memory>
 #include "sensesp.h"
 
 #include "AceButton.h"
@@ -20,7 +21,7 @@ using namespace ace_button;
 class BaseButtonHandler : public IEventHandler {
  public:
   BaseButtonHandler(int pin, String config_path = "") {
-    button_ = new AceButton(pin);
+    button_ = std::unique_ptr<AceButton>(new AceButton(pin));
     pinMode(pin, INPUT_PULLUP);
 
     ButtonConfig* button_config = button_->getButtonConfig();
@@ -35,7 +36,7 @@ class BaseButtonHandler : public IEventHandler {
   }
 
  protected:
-  AceButton* button_;
+  std::unique_ptr<AceButton> button_;
 };
 
 }  // namespace sensesp

src/sensesp/system/filesystem.cpp

@@ -13,6 +13,10 @@ Filesystem::Filesystem() : Resettable(-100) {
   }
 }
 
+Filesystem::~Filesystem() {
+  SPIFFS.end();
+}
+
 void Filesystem::reset() {
   ESP_LOGI(__FILENAME__, "Formatting filesystem");
   SPIFFS.format();

src/sensesp/system/filesystem.h

@@ -8,6 +8,7 @@ namespace sensesp {
 class Filesystem : public Resettable {
  public:
   Filesystem();
+  ~Filesystem();
   virtual void reset() override;
 };
 

src/sensesp/system/system_status_led.cpp

@@ -33,7 +33,8 @@ int ws_disconnected_pattern[] = {50, 50, 50, 50, 50, 50, 50, 650, PATTERN_END};
 int ws_authorizing_pattern[] = {200, 200, PATTERN_END};
 
 SystemStatusLed::SystemStatusLed(int pin)
-    : blinker_(new PatternBlinker(pin, no_ap_pattern)) {}
+    : blinker_(std::unique_ptr<PatternBlinker>(
+          new PatternBlinker(pin, no_ap_pattern))) {}
 
 void SystemStatusLed::set_wifi_no_ap() { blinker_->set_pattern(no_ap_pattern); }
 void SystemStatusLed::set_wifi_disconnected() {

src/sensesp/system/system_status_led.h

@@ -1,6 +1,8 @@
 #ifndef SENSESP_SRC_SENSESP_SYSTEM_SYSTEM_STATUS_LED_H_
 #define SENSESP_SRC_SENSESP_SYSTEM_SYSTEM_STATUS_LED_H_
 
+#include <memory>
+
 #include "lambda_consumer.h"
 #include "led_blinker.h"
 #include "sensesp/controllers/system_status_controller.h"
@@ -15,7 +17,7 @@ namespace sensesp {
 class SystemStatusLed : public ValueConsumer<SystemStatus>,
                         public ValueConsumer<int> {
  protected:
-  PatternBlinker* blinker_;
+  std::unique_ptr<PatternBlinker> blinker_;
 
   virtual void set_wifi_no_ap();
   virtual void set_wifi_disconnected();