Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Various code optimizations #647

Merged
merged 3 commits into from
Jan 3, 2020
Merged

Various code optimizations #647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25f6b8c
Fix the ability to provide `null` for `wifi.password` for open WiFi n…
mkfrey Dec 19, 2019
64ca0ba
Make `ota.enabled` configuration optional
mkfrey Dec 19, 2019
60ca263
Avoid unnecessary copying in Config::getSafeConfigFile()
mkfrey Dec 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Homie/Boot/BootConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,13 @@ void BootConfig::_onWifiConnectRequest(AsyncWebServerRequest *request) {
JsonObject parsedJson = parseJsonDoc.as<JsonObject>();
JsonVariant wifiSsid = parsedJson["ssid"];
JsonVariant wifiPassword = parsedJson["password"];
if (!wifiSsid.as<const char*>() || !wifiPassword.is<const char*>()) {
__SendJSONError(request, F("✖ SSID and password required"));
if (!wifiSsid.as<const char*>()) {
__SendJSONError(request, F("✖ SSID required"));
return;
}

if (!wifiPassword.isNull() && !wifiPassword.is<const char*>()) {
__SendJSONError(request, F("✖ Password is not a string"));
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Homie/Boot/BootNormal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ void BootNormal::_advertise() {
{
char* safeConfigFile = Interface::get().getConfig().getSafeConfigFile();
packetId = Interface::get().getMqttClient().publish(_prefixMqttTopic(PSTR("/$implementation/config")), 1, true, safeConfigFile);
free(safeConfigFile);
delete safeConfigFile;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can anyone else comment on this? My C++ skills are not good enough to review this specific change, sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, understood. The safeConfigFile is created in

homie-esp8266/src/Homie/Config.cpp

Line 167 in 60ca263

char* jsonString = new char[jsonBufferLength];

and 'delete' is the correct method to release memory when objects are created by the "new" expression (https://www.geeksforgeeks.org/delete-in-c/).

LGTM!

if (packetId != 0) _advertisementProgress.globalStep = AdvertisementProgress::GlobalStep::PUB_IMPLEMENTATION_VERSION;
break;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Homie/Config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ bool Config::load() {

const char* reqName = parsedJson["name"];
const char* reqWifiSsid = reqWifi["ssid"];
const char* reqWifiPassword = reqWifi["password"];
const char* reqMqttHost = reqMqtt["host"];

/* Optional config items */
Expand All @@ -78,6 +77,7 @@ bool Config::load() {

uint16_t reqWifiChannel = reqWifi["channel"] | 0;
const char* reqWifiBssid = reqWifi["bssid"] | "";
const char* reqWifiPassword = reqWifi["password"]; // implicit | nullptr;
const char* reqWifiIp = reqWifi["ip"] | "";
const char* reqWifiMask = reqWifi["mask"] | "";
const char* reqWifiGw = reqWifi["gw"] | "";
Expand Down Expand Up @@ -164,9 +164,9 @@ char* Config::getSafeConfigFile() const {
parsedJson["mqtt"].as<JsonObject>().remove("password");

size_t jsonBufferLength = measureJson(jsonDoc) + 1;
std::unique_ptr<char[]> jsonString(new char[jsonBufferLength]);
serializeJson(jsonDoc, jsonString.get(), jsonBufferLength);
return strdup(jsonString.get());
char* jsonString = new char[jsonBufferLength];
serializeJson(jsonDoc, jsonString, jsonBufferLength);
return jsonString;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better than before. But you should never return an allocated piece of memory as raw pointer in modern C++.

}

void Config::erase() {
Expand Down
36 changes: 20 additions & 16 deletions src/Homie/Utils/Validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,15 @@ ConfigValidationResult Validation::_validateConfigWifi(const JsonObject object)
{
JsonVariant wifiPassword = wifi["password"];

if (!wifiPassword.is<const char*>()) {
result.reason = F("wifi.password is not a string");
return result;
}
if (wifiPassword.as<const char*>() && strlen(wifiPassword.as<const char*>()) + 1 > MAX_WIFI_PASSWORD_LENGTH) {
result.reason = F("wifi.password is too long");
return result;
if (!wifiPassword.isNull()) {
if (!wifiPassword.as<const char*>()) {
result.reason = F("wifi.password is not a string");
return result;
}
if (strlen(wifiPassword.as<const char*>()) + 1 > MAX_WIFI_PASSWORD_LENGTH) {
result.reason = F("wifi.password is too long");
return result;
}
}
}

Expand Down Expand Up @@ -343,17 +345,19 @@ ConfigValidationResult Validation::_validateConfigOta(const JsonObject object) {

JsonVariant ota = object["ota"];

if (!ota.is<JsonObject>()) {
result.reason = F("ota is not an object");
return result;
}
if (!ota.isNull()) {
if (!ota.is<JsonObject>()) {
result.reason = F("ota is not an object");
return result;
}

{
JsonVariant otaEnabled = ota["enabled"];
{
JsonVariant otaEnabled = ota["enabled"];

if (!otaEnabled.is<bool>()) {
result.reason = F("ota.enabled is not a boolean");
return result;
if (!otaEnabled.isNull() && !otaEnabled.is<bool>()) {
result.reason = F("ota.enabled is not a boolean");
return result;
}
}
}

Expand Down