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

Migrate to ArduinoJson version 6 #622

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Migrate to ArduinoJson version 6 #622

merged 1 commit into from
Oct 1, 2019

Conversation

mkfrey
Copy link
Contributor

@mkfrey mkfrey commented Aug 25, 2019

  • Migrate the code from ArduinoJson v5 to v6
  • Resolve memory allocation issues resulting from the migration
  • Reduce unnecessary copying of some JSON related strings
  • Change to StaticJsonBuffer if possible

@mkfrey mkfrey mentioned this pull request Aug 25, 2019
Closed
@MajorTwip
Copy link

Would it be possible to integrate some sort of #606 in this PR?

@mkfrey
Copy link
Contributor Author

mkfrey commented Aug 26, 2019

@MajorTwip I will make sure to include it in the pull request for the JSON optimization changes.

iluminat23
iluminat23 reviewed Aug 27, 2019
View reviewed changes
src/Homie/Boot/BootConfig.cpp Outdated
JsonObject& parsedJson = parseJsonBuffer.parseObject(body);
if (!parsedJson.success()) {
StaticJsonDocument<JSON_OBJECT_SIZE(2)> parseJsonDoc;
char* body = (char*)(request->_tempObject);
Copy link
Contributor

@iluminat23 iluminat23 Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You are reusing the _tempObject as buffer in the JsonDocument. It might be better to use an AsyncCallbackJsonWebHandler instead of an AsyncWebServerRequest. This is only a suggestion for further optimization.
https://github.com/me-no-dev/ESPAsyncWebServer#json-body-handling-with-arduinojson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion. I didn't know that ESPAsyncWebServer had this feature.

I took a look at the implementation, but ESPAsyncWebServer doesn't seem to allow to use a static buffer and the dynamic allocation uses a fixed size, so more RAM than necessary is consumed.

iluminat23
iluminat23 reviewed Aug 27, 2019
View reviewed changes
Copy link
Contributor

@iluminat23 iluminat23 left a comment

Choose a reason for hiding this comment

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

Looks good.

I only found one problem with the size of the ip address. Other comments are only hints about style. Or similar minor things.

src/Homie/Boot/BootConfig.cpp Outdated
__SendJSONError(request, F("✖ Invalid or too big JSON"));
return;
}

JsonObject parsedJson = parseJsonDoc.as<JsonObject>();

Copy link
Contributor

Choose a reason for hiding this comment

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

superfluous empty line

src/Homie/Boot/BootConfig.cpp Outdated
@@ -143,8 +144,8 @@ void BootConfig::_onWifiConnectRequest(AsyncWebServerRequest *request) {
void BootConfig::_onWifiStatusRequest(AsyncWebServerRequest *request) {
Interface::get().getLogger() << F("Received Wi-Fi status request") << endl;

DynamicJsonBuffer generatedJsonBuffer(JSON_OBJECT_SIZE(2));
JsonObject& json = generatedJsonBuffer.createObject();
StaticJsonDocument<JSON_OBJECT_SIZE(2) + 18 + 16> generatedJsonDoc; // Includes memory to duplicate strings for status and local_ip
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain how you calculated these numbers.

Are only IPv4 addresses allowed? If not we run in trouble with IPv6 addresses.

You might also place your comment above the line. This increases the readability.

Copy link
Contributor Author

@mkfrey mkfrey Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, WiFi.localIP() only returns an IPv4 address. But I could change the value to 40 to fit IPv6 addresses in the future.

18 ist the size of the longest WiFi status string, which is "no_ssid_available". Since this is a flash string it needs to be duplicated by ArduinoJson.

src/Homie/Boot/BootConfig.cpp Outdated
if (!parsedJson.success()) {
StaticJsonDocument<JSON_OBJECT_SIZE(1)> parseJsonDoc;
char* body = (char*)(request->_tempObject);
if (deserializeJson(parseJsonDoc, body) != DeserializationError::Ok || !parseJsonDoc.is<JsonObject>()) { // do not use plain String, else fails
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the comment still make any sense? The comment also doesn't describe why. I would be in favor to remove this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why it was there the first place. I'll remove it.

src/Homie/Boot/BootConfig.cpp Outdated
__SendJSONError(request, F("✖ Invalid or too big JSON"));
return;
}

JsonObject parsedJson = parseJsonDoc.as<JsonObject>();

Copy link
Contributor

Choose a reason for hiding this comment

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

superfluous empty line

src/Homie/Config.cpp Outdated
Interface::get().getLogger() << F("✖ Invalid JSON in the config file") << endl;
return false;
}

JsonObject parsedJson = jsonDoc.as<JsonObject>();

Copy link
Contributor

Choose a reason for hiding this comment

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

superfluous empty line

src/Homie/Config.cpp Outdated
Interface::get().getLogger() << F("✖ Invalid or too big JSON") << endl;
return false;
}

JsonObject patchObject = patchJsonDoc.as<JsonObject>();

Copy link
Contributor

Choose a reason for hiding this comment

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

superfluous empty line

@iluminat23
Copy link
Contributor

I did a test run on my temp-sensor. I can run the configurator and also run it with my preconfigured filesystem.

@mkfrey
Copy link
Contributor Author

mkfrey commented Aug 27, 2019

@iluminat23 I committed your proposed changes to the pull request and also made sure to make the linter happy again.

iluminat23
iluminat23 approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@iluminat23 iluminat23 left a comment

Choose a reason for hiding this comment

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

Looks good.

Are the commit squashed when the branch is merged or do you need to do this? I'm not used to work with github merge requests.

@mkfrey
Copy link
Contributor Author

mkfrey commented Aug 28, 2019

AFAIK both commits will be merged separately into the branch on approval. Since the first commit doesn't contain a serious issue, I don't think squashing them would be necessary, but I could do it if you like.

@iluminat23
Copy link
Contributor

In my projects I would definitely do this. A clean history is a big plus. It makes refactoring or bug hunting mach easier.

@mkfrey
Migrate to ArduinoJson version 6
1491400 
* Migrate the code from ArduinoJson v5 to v6
* Resolve memory allocation issues resulting from the migration
* Reduce unnecessary copying of some JSON related strings
* Change to StaticJsonBuffer if possible
* Use C++ casts instead of C-style casts
* Remove unnecessary comments
* Serialize JsonDocument instead of JsonObject
@mkfrey
Copy link
Contributor Author

mkfrey commented Aug 28, 2019

I squashed the commits.

@homieiot homieiot deleted a comment from Dude77 Sep 3, 2019
@kleini
Copy link
Collaborator

kleini commented Sep 4, 2019

Will test this, when I have some spare time.

kleini
kleini approved these changes Oct 1, 2019
View reviewed changes
Copy link
Collaborator

@kleini kleini left a comment

Choose a reason for hiding this comment

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

The changes work very well for me.

@stritti stritti merged commit 67d6452 into homieiot:develop-v3 Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluminat23 iluminat23 iluminat23 approved these changes

@kleini kleini kleini approved these changes

@timpur timpur Awaiting requested review from timpur

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mkfrey @MajorTwip @iluminat23 @kleini @stritti