Add bi-directional (stream + get/set) support and refactoring of the internal classes #330
Conversation
|
Thanks for working on this!
I wonder if we could simplify further. I don't think we need the extra layer of indirection and we can consolidate Firebase and FirebaseArduino in a single object. (and I'm not even sure we need the FirebaseCall/Stream helper class either).
I think we could deprecate modem (/cc @ed7coyne) and reduce the scope of the project to maintaining the core library. |
| @@ -37,7 +37,7 @@ class FirebaseArduino { | |||
| * \param host Your firebase db host, usually X.firebaseio.com. | |||
| * \param auth Optional credentials for the db, a secret or token. | |||
| */ | |||
| void begin(const String& host, const String& auth = ""); | |||
| virtual void begin(const String& host, const String& auth = ""); | |||
There was a problem hiding this comment.
curious why this need to be virtual now?
There was a problem hiding this comment.
- Before virtual functions were in Firebase.h/FirebaseCall/FirebaseStream -> so they can be mocked in mock-firebase.h. Since that went away and modem library had to use FirebaseArduino instead, interface that needed to be mocked had to be declared virtual.
So if modem library rests in peace -> (or just the unit tests for it) -> it won't be needed.
|
We can definitely compact and reduce it further. Let's call it continuous refactoring, this was just the first sprint :) Btw, can we start tagging changes with version numbers? and maybe add that to release note history? |
|
@kotl let me try to get you added as an admin. Is that OK if I submit a PR against your branch to make further refactoring? Or would you prefer we do that in separate PRs? |
|
just sent you invite to my repo. go ahead. |
|
@kotl Sorry for the late reply. I don't want to block this further. So maybe we should merge and keep iterating? What do you think? |
|
Sorry for the lack of followup: merging this, let's iterate! |
This refactoring is fully compatible with previous FirebaseArduino.h interface and will use same or less amount of RAM if used in one-way mode (stream-only or get/set only).
From now on, you won't need two instances of FirebaseArduino in order to be able to stream and change values at the same time: second https connection will be created dynamically and work while you are using stream mode. Bi-directional operation does require almost twice as much RAM, leaving only about 10k left for vanilla sample in ESP8266. You will also need to use ESP8266 Arduino core 2.4.1 at the very least (that's when bi-directional support became possible due to fixes in https context).
Internal classes have been refactored into 2 to simplify development in future.
contrib/src/modem was adjusted to use FirebaseArduino, but has not been tested enough. please let me know if you still use it and if you encounter any bugs.