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.

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

Already on GitHub? Sign in to your account

Remove temporary buffer in ConfigFile.ino #8298

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

bblanchon
Copy link
Contributor

Hi,

Thank you guys for maintaining this critical piece of software that is at the heart of so many projects 馃憤

This PR simplifies the ConfigFile.ino example by removing the temporary buffer and passing the File directly to deserializeJson().

When reading from a Stream, using a temporary buffer is counterproductive because it complexifies the code and increases memory usage. It's also a source of confusion because it can create dangling pointers in the JsonDocument.
The only benefit of using a buffer is the reading speed, but I don't think speed is the focus in this example; if it were, it would use a buffer in saveConfig() too.

By the way, this example has been the origin of many issues in ArduinoJson's tracker:

It seems that I've been doing the customer service for this example, so I wonder if it's wise to keep it considering there is a comparable example in ArduinoJson, with the differences that it uses SD instead of LittleFS and it shows how to apply best practices by copying the data in a struct.

Please let me know what you think.

Best regards,
Benoit

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 3, 2021

so I wonder if it's wise to keep it
Please let me know what you think.

The least we could do is to ask you to add a comment / link to your example from within this example.

@earlephilhower
Copy link
Collaborator

@bblanchon can you please merge this with current head? Your GH settings don't allow us to update this PR, and it;s out of date so can't merge.

@bblanchon bblanchon force-pushed the remove-buffer-in-configfile-example branch from 66ac3b1 to 16f4f4e Compare September 5, 2021 07:57
@d-a-v
Copy link
Collaborator

d-a-v commented Sep 14, 2021

I suggest to add this comment:

  #include <ArduinoJson.h>
  #include "FS.h"
  #include <LittleFS.h>

+ // more and possibly updated information can be found at:
+ // https://arduinojson.org/v6/example/config/

  bool loadConfig() {

When reading from a `Stream`, like `File`, using a temporary buffer is counterproductive because it complexifies the code and increases memory usage.
It's also a source of confusion because it can create dangling pointers in the `JsonDocument`.
The only benefit of using a buffer is the reading speed, but I don't think speed is the focus in this example; if it were, it would use a buffer in `saveConfig()` too.
@bblanchon bblanchon force-pushed the remove-buffer-in-configfile-example branch from 16f4f4e to ada9fe9 Compare September 16, 2021 07:46
@bblanchon
Copy link
Contributor Author

I amended the commit to include the comments suggested by @d-a-v.

@d-a-v d-a-v merged commit 612e7ff into esp8266:master Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants