From 28cd3cf132a67e7facd87c546079bd1c1b277c73 Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Thu, 10 Oct 2024 15:16:31 +0300 Subject: [PATCH] More pointer safety --- src/sensesp/net/http_server.h | 6 +- src/sensesp/net/networking.h | 4 +- src/sensesp/net/web/base_command_handler.cpp | 12 ++-- src/sensesp/net/web/config_handler.cpp | 15 +++-- src/sensesp/net/web/static_file_handler.cpp | 2 +- src/sensesp/sensors/system_info.h | 20 +++--- src/sensesp/signalk/signalk_ws_client.cpp | 2 +- src/sensesp/signalk/signalk_ws_client.h | 4 +- src/sensesp_app.h | 66 ++++++++++---------- src/sensesp_app_builder.h | 42 ++++++++++--- src/sensesp_minimal_app.h | 2 +- test/system/test_event_loop/minimal_app.cpp | 4 +- test/system/test_offline/basics.cpp | 4 +- 13 files changed, 103 insertions(+), 80 deletions(-) diff --git a/src/sensesp/net/http_server.h b/src/sensesp/net/http_server.h index 23e93e45b..99c22ea76 100644 --- a/src/sensesp/net/http_server.h +++ b/src/sensesp/net/http_server.h @@ -139,11 +139,7 @@ class HTTPServer : public FileSystemSaveable { return true; } - void add_handler(HTTPRequestHandler* handler) { - handlers_.push_back(std::make_shared(*handler)); - } - - void add_handler(std::shared_ptr handler) { + void add_handler(std::shared_ptr& handler) { handlers_.push_back(handler); } diff --git a/src/sensesp/net/networking.h b/src/sensesp/net/networking.h index 0c619e956..8c92a0d7a 100644 --- a/src/sensesp/net/networking.h +++ b/src/sensesp/net/networking.h @@ -277,12 +277,12 @@ class Networking : public FileSystemSaveable, std::unique_ptr dns_server_; - std::shared_ptr wifi_state_producer_{new WiFiStateProducer()}; + std::shared_ptr wifi_state_producer_ = + std::make_shared(); std::shared_ptr> wifi_state_emitter_; }; - inline bool ConfigRequiresRestart(const Networking& obj) { return true; } } // namespace sensesp diff --git a/src/sensesp/net/web/base_command_handler.cpp b/src/sensesp/net/web/base_command_handler.cpp index 1c784fe26..327f5b321 100644 --- a/src/sensesp/net/web/base_command_handler.cpp +++ b/src/sensesp/net/web/base_command_handler.cpp @@ -6,7 +6,7 @@ namespace sensesp { void add_http_reset_handler(std::shared_ptr& server) { - HTTPRequestHandler* reset_handler = new HTTPRequestHandler( + auto reset_handler = std::make_shared( 1 << HTTP_POST, "/api/device/reset", [](httpd_req_t* req) { httpd_resp_send(req, "Resetting device back to factory defaults. " @@ -19,7 +19,7 @@ void add_http_reset_handler(std::shared_ptr& server) { } void add_http_restart_handler(std::shared_ptr& server) { - HTTPRequestHandler* restart_handler = new HTTPRequestHandler( + auto restart_handler = std::make_shared( 1 << HTTP_POST, "/api/device/restart", [](httpd_req_t* req) { httpd_resp_send(req, "Restarting device", 0); event_loop()->onDelay(500, []() { ESP.restart(); }); @@ -29,8 +29,8 @@ void add_http_restart_handler(std::shared_ptr& server) { } void add_http_info_handler(std::shared_ptr& server) { - HTTPRequestHandler* info_handler = - new HTTPRequestHandler(1 << HTTP_GET, "/api/info", [](httpd_req_t* req) { + auto info_handler = std::make_shared( + 1 << HTTP_GET, "/api/info", [](httpd_req_t* req) { auto status_page_items = StatusPageItemBase::get_status_page_items(); JsonDocument json_doc; @@ -74,7 +74,7 @@ void add_routes_handlers(std::shared_ptr& server) { serializeJson(routes_json, response); - HTTPRequestHandler* routes_handler = new HTTPRequestHandler( + auto routes_handler = std::make_shared( 1 << HTTP_GET, "/api/routes", [response](httpd_req_t* req) { httpd_resp_set_type(req, "application/json"); httpd_resp_sendstr(req, response.c_str()); @@ -100,7 +100,7 @@ void add_routes_handlers(std::shared_ptr& server) { for (auto it = routes.begin(); it != routes.end(); ++it) { String path = it->get_path(); - HTTPRequestHandler* route_handler = new HTTPRequestHandler( + auto route_handler = std::make_shared( 1 << HTTP_GET, path.c_str(), [root_page](httpd_req_t* req) { httpd_resp_set_type(req, root_page->content_type); if (root_page->content_encoding != nullptr) { diff --git a/src/sensesp/net/web/config_handler.cpp b/src/sensesp/net/web/config_handler.cpp index ae92d0091..42231d794 100644 --- a/src/sensesp/net/web/config_handler.cpp +++ b/src/sensesp/net/web/config_handler.cpp @@ -80,12 +80,13 @@ esp_err_t handle_config_item_list(httpd_req_t* req) { } void add_config_list_handler(std::shared_ptr& server) { - server->add_handler(new HTTPRequestHandler(1 << HTTP_GET, "/api/config", - handle_config_item_list)); + auto handler = std::make_shared( + 1 << HTTP_GET, "/api/config", handle_config_item_list); + server->add_handler(handler); } void add_config_get_handler(std::shared_ptr& server) { - server->add_handler(new HTTPRequestHandler( + auto handler = std::make_shared( 1 << HTTP_GET, "/api/config/*", [](httpd_req_t* req) { ESP_LOGD("ConfigHandler", "GET request to URL %s", req->uri); String url_tail = String(req->uri).substring(11); @@ -123,11 +124,12 @@ void add_config_get_handler(std::shared_ptr& server) { httpd_resp_set_type(req, "application/json"); httpd_resp_sendstr(req, response.c_str()); return ESP_OK; - })); + }); + server->add_handler(handler); } void add_config_put_handler(std::shared_ptr& server) { - server->add_handler(new HTTPRequestHandler( + auto handler = std::make_shared( 1 << HTTP_PUT, "/api/config/*", [](httpd_req_t* req) { // check that the content type is JSON ESP_LOGI(__FILENAME__, "PUT request to URL %s", req->uri); @@ -198,7 +200,8 @@ void add_config_put_handler(std::shared_ptr& server) { httpd_resp_sendstr(req, response.c_str()); return ESP_OK; - })); + }); + server->add_handler(handler); } void add_config_handlers(std::shared_ptr& server) { diff --git a/src/sensesp/net/web/static_file_handler.cpp b/src/sensesp/net/web/static_file_handler.cpp index 06c09b749..2cf8f432a 100644 --- a/src/sensesp/net/web/static_file_handler.cpp +++ b/src/sensesp/net/web/static_file_handler.cpp @@ -7,7 +7,7 @@ namespace sensesp { void add_static_file_handlers(std::shared_ptr server) { for (int i = 0; kFrontendFiles[i].url != nullptr; i++) { const StaticFileData& data = kFrontendFiles[i]; - HTTPRequestHandler* handler = new HTTPRequestHandler( + auto handler = std::make_shared( 1 << HTTP_GET, kFrontendFiles[i].url, [data](httpd_req_t* req) { httpd_resp_set_type(req, data.content_type); if (data.content_encoding != nullptr) { diff --git a/src/sensesp/sensors/system_info.h b/src/sensesp/sensors/system_info.h index 0f11528e1..7ead39148 100644 --- a/src/sensesp/sensors/system_info.h +++ b/src/sensesp/sensors/system_info.h @@ -1,9 +1,9 @@ #ifndef SENSESP_SENSORS_SYSTEM_INFO_H_ #define SENSESP_SENSORS_SYSTEM_INFO_H_ -#include #include #include +#include #include "sensesp/signalk/signalk_output.h" #include "sensesp_base_app.h" @@ -15,7 +15,8 @@ namespace sensesp { * @brief Connect a system information sensor to SKOutput **/ template -void connect_system_info_sensor(ValueProducer* sensor, String prefix, String name) { +void connect_system_info_sensor(std::shared_ptr>& sensor, + const String& prefix, const String& name) { auto hostname_obs = SensESPBaseApp::get()->get_hostname_observable(); String hostname = hostname_obs->get(); String path = prefix + hostname + "." + name; @@ -49,8 +50,7 @@ class SystemHz : public ValueProducer { elapsed_millis_ = 0; event_loop()->onTick([this]() { this->tick(); }); - event_loop()->onRepeat(1000, - [this]() { this->update(); }); + event_loop()->onRepeat(1000, [this]() { this->update(); }); } String get_value_name() { return "systemhz"; } @@ -71,8 +71,7 @@ class SystemHz : public ValueProducer { class FreeMem : public ValueProducer { public: FreeMem() { - event_loop()->onRepeat(1000, - [this]() { this->update(); }); + event_loop()->onRepeat(1000, [this]() { this->update(); }); } String get_value_name() { return "freemem"; } @@ -91,8 +90,7 @@ class FreeMem : public ValueProducer { class Uptime : public ValueProducer { public: Uptime() { - event_loop()->onRepeat(1000, - [this]() { this->update(); }); + event_loop()->onRepeat(1000, [this]() { this->update(); }); } String get_value_name() { return "uptime"; } @@ -111,8 +109,7 @@ class Uptime : public ValueProducer { class IPAddrDev : public ValueProducer { public: IPAddrDev() { - event_loop()->onRepeat(10000, - [this]() { this->update(); }); + event_loop()->onRepeat(10000, [this]() { this->update(); }); } String get_value_name() { return "ipaddr"; } @@ -131,8 +128,7 @@ class IPAddrDev : public ValueProducer { class WiFiSignal : public ValueProducer { public: WiFiSignal() { - event_loop()->onRepeat(3000, - [this]() { this->update(); }); + event_loop()->onRepeat(3000, [this]() { this->update(); }); } String get_value_name() { return "wifisignal"; } diff --git a/src/sensesp/signalk/signalk_ws_client.cpp b/src/sensesp/signalk/signalk_ws_client.cpp index 6d106105c..cc59a808a 100644 --- a/src/sensesp/signalk/signalk_ws_client.cpp +++ b/src/sensesp/signalk/signalk_ws_client.cpp @@ -74,7 +74,7 @@ static void websocket_event_handler(void* handler_args, esp_event_base_t base, } } -SKWSClient::SKWSClient(const String& config_path, SKDeltaQueue* sk_delta_queue, +SKWSClient::SKWSClient(const String& config_path, std::shared_ptr sk_delta_queue, const String& server_address, uint16_t server_port, bool use_mdns) : FileSystemSaveable{config_path}, diff --git a/src/sensesp/signalk/signalk_ws_client.h b/src/sensesp/signalk/signalk_ws_client.h index 202e5170a..c357ea194 100644 --- a/src/sensesp/signalk/signalk_ws_client.h +++ b/src/sensesp/signalk/signalk_ws_client.h @@ -37,7 +37,7 @@ class SKWSClient : public FileSystemSaveable, ///////////////////////////////////////////////////////// // main task methods - SKWSClient(const String& config_path, SKDeltaQueue* sk_delta_queue, + SKWSClient(const String& config_path, std::shared_ptr sk_delta_queue, const String& server_address, uint16_t server_port, bool use_mdns = true); @@ -113,7 +113,7 @@ class SKWSClient : public FileSystemSaveable, WiFiClient wifi_client_{}; esp_websocket_client_handle_t client_{}; - SKDeltaQueue* sk_delta_queue_; + std::shared_ptr sk_delta_queue_; /// @brief Emits the number of deltas sent since last report TaskQueueProducer delta_tx_tick_producer_{0, event_loop(), 990}; Integrator delta_tx_count_producer_{1, 0, ""}; diff --git a/src/sensesp_app.h b/src/sensesp_app.h index 970a54cb3..77d2b3e7d 100644 --- a/src/sensesp_app.h +++ b/src/sensesp_app.h @@ -46,15 +46,6 @@ class SensESPApp : public SensESPBaseApp { */ SensESPApp(SensESPApp& other) = delete; - ~SensESPApp() { - delete ota_; - delete ws_client_; - delete mdns_discovery_; - delete sk_delta_queue_; - delete system_status_led_; - delete button_handler_; - } - /** * Singletons should not be assignable */ @@ -85,12 +76,12 @@ class SensESPApp : public SensESPBaseApp { } // getters for internal members - SKDeltaQueue* get_sk_delta() { return this->sk_delta_queue_; } - SystemStatusController* get_system_status_controller() { - return &(this->system_status_controller_); + std::shared_ptr get_sk_delta() { return this->sk_delta_queue_; } + std::shared_ptr get_system_status_controller() { + return this->system_status_controller_; } std::shared_ptr& get_networking() { return this->networking_; } - SKWSClient* get_ws_client() { return this->ws_client_; } + std::shared_ptr get_ws_client() { return this->ws_client_; } protected: /** @@ -132,7 +123,8 @@ class SensESPApp : public SensESPBaseApp { this->sk_server_port_ = sk_server_port; return this; } - const SensESPApp* set_system_status_led(SystemStatusLed* system_status_led) { + const SensESPApp* set_system_status_led( + std::shared_ptr& system_status_led) { this->system_status_led_ = system_status_led; return this; } @@ -171,7 +163,7 @@ class SensESPApp : public SensESPBaseApp { if (ota_password_ != nullptr) { // create the OTA object - ota_ = new OTA(ota_password_); + ota_ = std::make_shared(ota_password_); } bool captive_portal_enabled = networking_->is_captive_portal_enabled(); @@ -189,38 +181,38 @@ class SensESPApp : public SensESPBaseApp { ConfigItem(this->http_server_); // create the SK delta object - sk_delta_queue_ = new SKDeltaQueue(); + sk_delta_queue_ = std::make_shared(); // create the websocket client bool const use_mdns = sk_server_address_ == ""; - this->ws_client_ = - new SKWSClient("/System/Signal K Settings", sk_delta_queue_, - sk_server_address_, sk_server_port_, use_mdns); + this->ws_client_ = std::make_shared( + "/System/Signal K Settings", sk_delta_queue_, sk_server_address_, + sk_server_port_, use_mdns); ConfigItem(this->ws_client_); // connect the system status controller this->networking_->get_wifi_state_producer()->connect_to( - &system_status_controller_.get_wifi_state_consumer()); + &system_status_controller_->get_wifi_state_consumer()); this->ws_client_->connect_to( - &system_status_controller_.get_ws_connection_state_consumer()); + &system_status_controller_->get_ws_connection_state_consumer()); // create the MDNS discovery object - mdns_discovery_ = new MDNSDiscovery(); + mdns_discovery_ = std::make_shared(); // create a system status led and connect it - if (system_status_led_ == NULL) { - system_status_led_ = new SystemStatusLed(LED_PIN); + if (system_status_led_ == nullptr) { + system_status_led_ = std::make_shared(LED_PIN); } - this->system_status_controller_.connect_to( + this->system_status_controller_->connect_to( system_status_led_->get_system_status_consumer()); this->ws_client_->get_delta_tx_count_producer().connect_to( system_status_led_->get_delta_tx_count_consumer()); // create the button handler if (button_gpio_pin_ != -1) { - button_handler_ = new ButtonHandler(button_gpio_pin_); + button_handler_ = std::make_shared(button_gpio_pin_); } // connect status page items @@ -258,19 +250,20 @@ class SensESPApp : public SensESPBaseApp { String ap_password_ = "thisisfine"; const char* ota_password_ = nullptr; - MDNSDiscovery* mdns_discovery_; + std::shared_ptr mdns_discovery_; std::shared_ptr http_server_; - SystemStatusLed* system_status_led_ = NULL; - SystemStatusController system_status_controller_; + std::shared_ptr system_status_led_; + std::shared_ptr system_status_controller_ = + std::make_shared(); int button_gpio_pin_ = SENSESP_BUTTON_PIN; - ButtonHandler* button_handler_ = nullptr; + std::shared_ptr button_handler_; std::shared_ptr networking_; - OTA* ota_; - SKDeltaQueue* sk_delta_queue_; - SKWSClient* ws_client_; + std::shared_ptr ota_; + std::shared_ptr sk_delta_queue_; + std::shared_ptr ws_client_; StatusPageItem sensesp_version_ui_output_{ "SenseESP version", kSensESPVersion, "Software", 1900}; @@ -295,6 +288,13 @@ class SensESPApp : public SensESPBaseApp { StatusPageItem delta_rx_count_ui_output_{"SK Delta RX count", 0, "Signal K", 1800}; +// Placeholders for system status sensors in case they are created +std::shared_ptr> system_hz_sensor_; +std::shared_ptr> free_mem_sensor_; +std::shared_ptr> uptime_sensor_; +std::shared_ptr> ip_address_sensor_; +std::shared_ptr> wifi_signal_sensor_; + friend class WebServer; friend class SensESPAppBuilder; }; diff --git a/src/sensesp_app_builder.h b/src/sensesp_app_builder.h index f4b3739e4..4f0dd5607 100644 --- a/src/sensesp_app_builder.h +++ b/src/sensesp_app_builder.h @@ -3,7 +3,9 @@ #include +#include "Arduino.h" #include "sensesp/sensors/system_info.h" +#include "sensesp/system/valueproducer.h" #include "sensesp/transforms/debounce.h" #include "sensesp_app.h" #include "sensesp_base_app_builder.h" @@ -119,7 +121,8 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { * @param system_status_led * @return SensESPAppBuilder* */ - SensESPAppBuilder* set_system_status_led(SystemStatusLed* system_status_led) { + SensESPAppBuilder* set_system_status_led( + std::shared_ptr system_status_led) { app_->set_system_status_led(system_status_led); return this; } @@ -134,7 +137,12 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { */ SensESPAppBuilder* enable_system_hz_sensor( String prefix = kDefaultSystemInfoSensorPrefix) { - connect_system_info_sensor(new SystemHz(), prefix, "systemHz"); + auto sensor = std::make_shared(); + auto vproducer = std::static_pointer_cast>(sensor); + // We need to store the sensor in the app so it doesn't get garbage + // collected. + app_->system_hz_sensor_ = vproducer; + connect_system_info_sensor(vproducer, prefix, "systemHz"); return this; } /** @@ -145,7 +153,12 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { */ SensESPAppBuilder* enable_free_mem_sensor( String prefix = kDefaultSystemInfoSensorPrefix) { - connect_system_info_sensor(new FreeMem(), prefix, "freeMemory"); + auto sensor = std::make_shared(); + auto vproducer = std::static_pointer_cast>(sensor); + // We need to store the sensor in the app so it doesn't get garbage + // collected. + app_->free_mem_sensor_ = vproducer; + connect_system_info_sensor(vproducer, prefix, "freeMemory"); return this; } /** @@ -156,7 +169,12 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { */ SensESPAppBuilder* enable_uptime_sensor( String prefix = kDefaultSystemInfoSensorPrefix) { - connect_system_info_sensor(new Uptime(), prefix, "uptime"); + auto sensor = std::make_shared(); + auto vproducer = std::static_pointer_cast>(sensor); + // We need to store the sensor in the app so it doesn't get garbage + // collected. + app_->uptime_sensor_ = vproducer; + connect_system_info_sensor(vproducer, prefix, "uptime"); return this; } /** @@ -167,7 +185,12 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { */ SensESPAppBuilder* enable_ip_address_sensor( String prefix = kDefaultSystemInfoSensorPrefix) { - connect_system_info_sensor(new IPAddrDev(), prefix, "ipAddress"); + auto sensor = std::make_shared(); + auto vproducer = std::static_pointer_cast>(sensor); + // We need to store the sensor in the app so it doesn't get garbage + // collected. + app_->ip_address_sensor_ = vproducer; + connect_system_info_sensor(vproducer, prefix, "ipAddress"); return this; } /** @@ -178,7 +201,12 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { */ SensESPAppBuilder* enable_wifi_signal_sensor( String prefix = kDefaultSystemInfoSensorPrefix) { - connect_system_info_sensor(new WiFiSignal(), prefix, "wifiSignalLevel"); + auto sensor = std::make_shared(); + auto vproducer = std::static_pointer_cast>(sensor); + // We need to store the sensor in the app so it doesn't get garbage + // collected. + app_->wifi_signal_sensor_ = vproducer; + connect_system_info_sensor(vproducer, prefix, "wifiSignalLevel"); return this; } @@ -238,7 +266,7 @@ class SensESPAppBuilder : public SensESPBaseAppBuilder { const SensESPAppBuilder* enable_wifi_watchdog() { // create the wifi disconnect watchdog app_->system_status_controller_ - .connect_to(new Debounce( + ->connect_to(new Debounce( 3 * 60 * 1000 // 180 s = 180000 ms = 3 minutes )) ->connect_to(new LambdaConsumer([](SystemStatus input) { diff --git a/src/sensesp_minimal_app.h b/src/sensesp_minimal_app.h index 7e0aef6a2..3e983b116 100644 --- a/src/sensesp_minimal_app.h +++ b/src/sensesp_minimal_app.h @@ -15,7 +15,7 @@ class SensESPMinimalApp : public SensESPBaseApp { /** * @brief Get the singleton instance of the SensESPMinimalApp */ - static const std::shared_ptr& get() { + static const std::shared_ptr get() { if (instance_ == nullptr) { instance_ = std::shared_ptr(new SensESPMinimalApp()); } diff --git a/test/system/test_event_loop/minimal_app.cpp b/test/system/test_event_loop/minimal_app.cpp index 863886d6a..764abd732 100644 --- a/test/system/test_event_loop/minimal_app.cpp +++ b/test/system/test_event_loop/minimal_app.cpp @@ -2,8 +2,8 @@ #include "elapsedMillis.h" #include "sensesp/sensors/sensor.h" -#include "sensesp/system/observablevalue.h" #include "sensesp/system/lambda_consumer.h" +#include "sensesp/system/observablevalue.h" #include "sensesp_minimal_app_builder.h" #include "unity.h" @@ -11,7 +11,7 @@ using namespace sensesp; void test_sensesp() { SensESPMinimalAppBuilder builder; - SensESPMinimalApp* sensesp_app = builder.get_app(); + auto sensesp_app = builder.get_app(); TEST_ASSERT_NOT_NULL(sensesp_app); auto sensor = RepeatSensor(10, []() { diff --git a/test/system/test_offline/basics.cpp b/test/system/test_offline/basics.cpp index 196aa6410..893a8ceed 100644 --- a/test/system/test_offline/basics.cpp +++ b/test/system/test_offline/basics.cpp @@ -138,7 +138,7 @@ void test_angle_correction_from_json() { // PersistingObservableValue saves itself on change void test_persisting_observable_value_schema() { SensESPMinimalAppBuilder builder; - SensESPMinimalApp* sensesp_app = builder.get_app(); + auto sensesp_app = builder.get_app(); TEST_ASSERT_NOT_NULL(sensesp_app); ESP_LOGD("test_persisting_observable_value_schema", "about to create object"); @@ -366,7 +366,7 @@ void test_connect_to() { // safely delete both the producer and the consumer. void test_weak_ptr_connect_to() { SensESPMinimalAppBuilder builder; - SensESPMinimalApp* sensesp_app = builder.get_app(); + auto sensesp_app = builder.get_app(); TEST_ASSERT_NOT_NULL(sensesp_app); auto value = std::make_shared>(2);