-
Notifications
You must be signed in to change notification settings - Fork 494
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve stability of the repo #296
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Can you add the new version of ArduinoJson to the third-party directory? |
…y shipped with repo.
Finally fixed both CI builds. |
.travis.yml
Outdated
@@ -9,7 +9,6 @@ addons: | |||
- g++-4.8 | |||
env: | |||
- ARDUINO_VERSION=1.6.9 ARDUINO_ESP8266_VERSION=2.3.0 LIB_NEOPIXEL_VERSION=v1.0.5 LIB_GFX_VERSION=v1.1.5 LIB_SSD1306_VERSION=1.1.0 LIB_JSON_VERSION=v5.11.2 ARDUINO_ROOT=${HOME}/arduino-${ARDUINO_VERSION} ARDUINO_ESP8266_ROOT=${ARDUINO_ROOT}/hardware/esp8266com/esp8266 ARDUINO_HOME=${HOME}/Arduino | |||
- ARDUINO_VERSION=nightly ARDUINO_ESP8266_VERSION=master LIB_NEOPIXEL_VERSION=master LIB_GFX_VERSION=master LIB_SSD1306_VERSION=master LIB_JSON_VERSION=master ARDUINO_ROOT=${HOME}/arduino-${ARDUINO_VERSION} ARDUINO_ESP8266_ROOT=${ARDUINO_ROOT}/hardware/esp8266com/esp8266 ARDUINO_HOME=${HOME}/Arduino |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you removing nightly on purpose?
- ${ARDUINO_ROOT}/arduino-builder -verbose -hardware ${ARDUINO_ROOT}/hardware/ -tools ${ARDUINO_ESP8266_ROOT}/tools/ -tools ${ARDUINO_ROOT}/tools-builder/ -fqbn esp8266com:esp8266:nodemcuv2 -libraries ${ARDUINO_HOME}/libraries/ -prefs build.flash_ld=${ARDUINO_ESP8266_ROOT}/tools/sdk/ld/eagle.flash.4m.ld -prefs build.flash_freq=40 -prefs build.flash_size=4M examples/FirebaseNeoPixel_ESP8266/FirebaseNeoPixel_ESP8266.ino | ||
- ${ARDUINO_ROOT}/arduino-builder -verbose -hardware ${ARDUINO_ROOT}/hardware/ -tools ${ARDUINO_ESP8266_ROOT}/tools/ -tools ${ARDUINO_ROOT}/tools-builder/ -fqbn esp8266com:esp8266:nodemcuv2 -libraries ${ARDUINO_HOME}/libraries/ -prefs build.flash_ld=${ARDUINO_ESP8266_ROOT}/tools/sdk/ld/eagle.flash.4m.ld -prefs build.flash_freq=40 -prefs build.flash_size=4M examples/FirebaseStream_ESP8266/FirebaseStream_ESP8266.ino | ||
- ${ARDUINO_ROOT}/arduino-builder -verbose -hardware ${ARDUINO_ROOT}/hardware/ -tools ${ARDUINO_ESP8266_ROOT}/tools/ -tools ${ARDUINO_ROOT}/tools-builder/ -fqbn esp8266com:esp8266:nodemcuv2 -libraries ${ARDUINO_HOME}/libraries/ -prefs build.flash_ld=${ARDUINO_ESP8266_ROOT}/tools/sdk/ld/eagle.flash.4m.ld -prefs build.flash_freq=40 -prefs build.flash_size=4M examples/FirebaseRoom_ESP8266/FirebaseRoom_ESP8266.ino | ||
- ${ARDUINO_ROOT}/arduino-builder -verbose -hardware ${ARDUINO_ROOT}/hardware/ -tools ${ARDUINO_ESP8266_ROOT}/tools/ -tools ${ARDUINO_ROOT}/tools-builder/ -fqbn esp8266com:esp8266:nodemcuv2 -libraries ${ARDUINO_HOME}/libraries/ -prefs build.flash_ld=${ARDUINO_ESP8266_ROOT}/tools/sdk/ld/eagle.flash.4m.ld -prefs build.flash_freq=40 -prefs build.flash_size=4M -prefs build.f_cpu=80000000 examples/FirebaseDemo_ESP8266/FirebaseDemo_ESP8266.ino |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this is not already added by the board package:
https://github.com/esp8266/Arduino/blob/master/boards.txt#L1353
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did not work without it, I had to create my own travis docker container to figure out why the heck it was failing.
types. | ||
friendly way. | ||
|
||
ArduinoJson is no longer part of this library and you will have to install latest version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a new # DEPENDENCIES
sections in the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
FirebaseError error_; | ||
std::string response_; | ||
DynamicJsonBuffer buffer_; | ||
std::shared_ptr<StaticJsonBuffer<FIREBASE_JSONBUFFER_SIZE>> buffer_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could simply be statically allocated as
StaticJsonBuffer<FIREBASE_JSONBUFFER_SIZE> buffer_
without shared_ptr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember the details, but I am pretty sure latest version of JSONArduino is very strict on memory allocations so passing this buffer around as-is results in compilation errors.
ping? |
Any update appreciated. - It would be great to dissolve the current dependency to the contributed JSON library and use Arduino JSON form head or a stable version. |
I am really sorry, I did not follow up on these changes. This change was intended to be complemented by other changes in the main esp8266 repo for handling https contexts to make a rock solid firmware that never crashes and also allowing firebase-arduino for 2 simultaneous connections to stream and write at the same time. And the moment I had it working well, esp8266 repo got changed so much, that fixes I already tested and debugged for many days, stopped working, so regrettably I decided to abandon this route. However, I believe there is still a benefit to get this change in for firebase-arduino repo, so if @proppy let's me try again, I would be happy to submit this. |
@kotl can you try again with a signed CLA? |
CLAs look good, thanks! |
Done! |
Thanks for working on this! |
Tested successfully with the following sketch
|
Thank you for your update, Stream works fine. But if I test changing value by Update, not Set. |
@lenguyenvu007 can you file a separate issue about this? Thanks! |
This update contains:
https://github.com/bblanchon/ArduinoJson