[CM-123] VerneMQ integration #7

AlbyIanna · 2018-08-28T17:02:15Z

No description provided.

sandeepmistry · 2018-08-28T17:34:28Z

@AlbyIanna can you please rebase this?

sandeepmistry · 2018-08-28T17:37:30Z

src/SerialFlashStorage.h

@@ -0,0 +1,43 @@
+/*


Why do we need the flash storage?

sandeepmistry · 2018-08-28T17:39:19Z

src/ArduinoCloud.cpp

@@ -52,35 +52,137 @@ int ArduinoCloudClass::begin(Client& net)
    return 0;
  }

+  Serial.println("Compressed cert = ");


I think we can remove printing the compressed cert to the serial

examples/MKR1000_Cloud_Blink/MKR1000_Cloud_Blink.ino

sandeepmistry · 2018-08-28T17:40:38Z

examples/MKR1000_Cloud_Blink/MKR1000_Cloud_Blink.ino

-  while (!Serial) {
-    ; // wait for serial port to connect. Needed for native USB port only
-  }
+  delay(7000);


Why delay(7000)?

It because of the Serial. I want to make sure the sketch doesn't start if the serial isn't up, but at the same i want the sketch to work even when you are not using the serial. This is mainly because of the Getting Started: we are telling users that, once uploaded the sketch, they can unplug the board and plug it to a socket wall. Do you have a better idea?

Best practice in this case is:

int timeout = millis() + TIMEOUT; while (!Serial && (millis() < timeout)) {}

so it starts immediately when the serial port is connected and waits a bit if unplugged

It makes sense. Thanks!

it should not happen in this case but to avoid wrapping errors better to do the delta between currtime and start time

unsigned long start = millis(); while (!Serial && (millis() - start < TIMEOUT)) {}

sandeepmistry · 2018-08-28T17:41:45Z

examples/MKR1000_Cloud_Blink/MKR1000_Cloud_Blink.ino

 ///////please enter your sensitive data in the Secret tab/arduino_secrets.h
 char ssid[] = SECRET_SSID;        // your network SSID (name)
 char pass[] = SECRET_PASS;    // your network password (use for WPA, or use as key for WEP)
 int status = WL_IDLE_STATUS;     // the WiFi radio's status
+String serialString = "";        // the string used to compose network messages from the received characters


We can have a better name for this, maybe something like inputBuffer or cloudSerialBuffer?

sandeepmistry · 2018-08-28T17:42:50Z

examples/MKR1000_Cloud_Blink/MKR1000_Cloud_Blink.ino

  }

+  // Just to be able to simulate MKR1000's responses through the serial monitor


This sketch can also run on the MKR WiFi 1010?

I think the MKR1010 would need WiFiNINA instead of WiFi101

Right, maybe we can add a comment at the start of the sketch that says:

// change to WiFiNINA.h if you are using the MKR WiFi 1010 or MKR Vidor 4000

sandeepmistry · 2018-08-28T17:43:47Z

examples/MKR1000_Cloud_Blink/MKR1000_Cloud_Blink.ino

+    digitalWrite(6, LOW);
+  }
+
+  // Send back the command you just applied. This way the Angular frontend can stay synchronized with the MKR1000 state.


Let's remove the comment about Angular, most users won't know what it is.

sandeepmistry · 2018-08-28T17:48:41Z

src/ArduinoCloud.cpp

+    // try establish the MQTT broker connection 
+    connect();
+    // delay eventually used for the nex re-connection try
+    delay(timeout);                   


Seems weird to have a delay in the lib.

If there's no problem with the tight connect() loop I'd replace the condition as

timeout += millis(); while(!_mqttClient.connected() && (retries++ < maxRetries) && (millis() < timeout)) {...} if (retries == maxRetries || millis() < timeout) return false;

so it fails in both cases without locking for too long

same here, this code has integer overflow problems. Please use only unsigned long and subtractions when dealing with millis().

sandeepmistry · 2018-08-28T17:49:26Z

src/ArduinoCloudV2.h

+    String _id;
+    ArduinoCloudThing Thing;
+    BearSSLClient* _bearSslClient;
+    HttpClient* _otaClient;


I don't see used OTA anywhere.

There are 2 commits missing from this PR 02a1994 and 02a1994 that completely remove the OTA code while we find a better way to handle it 🙂
@AlbyIanna I'm pushing them to this branch

sandeepmistry · 2018-08-28T17:49:36Z

src/ArduinoCloudV2.h

+    String _stdoutTopic;
+    String _dataTopicIn;
+    String _dataTopicOut;
+    String _otaTopic;


This variable is not used.

sandeepmistry · 2018-08-28T17:51:01Z

src/ArduinoCloudV2.h

+    // Clean up existing Mqtt connection, create a new one and initialize it
+    void reconnect(Client& net);
+
+    #define addProperty( v, ...) addPropertyReal(v, #v, __VA_ARGS__)


If we're adding this, it would be good to use it in an example

Agree, we should add at least an example using the properties @AlbyIanna

Okay, i'll work on the new example then.
Talking with @facchinm we agreed on using a property instead of the CloudSerial in the CloudBlink sketch, and eventually write another example sketch that uses the CloudSerial.

Do we really need cloud serial for OOBE then?

Yes, we have to use it. And sadly we cannot use the properties to make the led blink in the OOBE, because otherwise we'd also have to create a Thing and introduce the user to the Arduino Cloud application, which is totally outside the scope of the OOBE.

sandeepmistry · 2018-08-28T17:52:16Z

src/ArduinoCloudV2.h

+
+    #define addProperty( v, ...) addPropertyReal(v, #v, __VA_ARGS__)
+
+    template<typename T> void addPropertyReal(T& property, String name, permissionType _permission = READWRITE, long seconds = ON_CHANGE, T minDelta = 0, void(*fn)(void) = NULL) {


Why do we have addProperty and addPropertyReal both as public. It will strange from a documentation point.

I was looking for a better way to do the variable name -> string property trick without using a macro but none was satisfying; theoretically addPropertyReal should be protected in Thing class and CloudV2 should be declared as friend, but this clashes with the macro stuff. Any better idea?

sandeepmistry · 2018-08-30T18:18:59Z

@AlbyIanna can you please rebase this on master?

I really dislike that I had to expose the Thing object but otherwise the addProperty() call would become terrible. I added a line on Cloud_blink example to show the composition of functions on properties. It can be safely removed in the next iteration.

sandeepmistry · 2018-08-31T13:38:59Z

src/ArduinoCloud.cpp

+  // MQTT topics definition
+  _stdoutTopic = "/a/d/" + _id + "/s/o";
+  _stdinTopic = "/a/d/" + _id + "/s/i";
+  _dataTopicIn = "/a/d/" + _id + "/e/i";


We need to have a discussion on this topic structure. It makes things a bit complicated if one thing wants to listen to another things property.

sandeepmistry · 2018-08-31T13:41:22Z

Me and @facchinm discussed splitting what is needed for OOBE into a new PR.

Then the cloud thing stuff can be merged into master once we have end to end functioning.

facchinm · 2018-08-31T15:57:47Z

Closing after merging #8

AlbyIanna requested a review from facchinm August 28, 2018 17:02

AlbyIanna force-pushed the vernemq branch from e80c1c4 to 6b78887 Compare August 28, 2018 17:08

sandeepmistry reviewed Aug 28, 2018

View reviewed changes

src/SerialFlashStorage.h Outdated

@@ -0,0 +1,43 @@

/*

Copy link

Contributor

sandeepmistry Aug 28, 2018

Choose a reason for hiding this comment

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

Why do we need the flash storage?

sandeepmistry reviewed Aug 28, 2018

View reviewed changes

sandeepmistry suggested changes Aug 28, 2018

View reviewed changes

facchinm and others added 23 commits August 31, 2018 14:13

Integrate ArduinoCloudThing

c9764bf

I really dislike that I had to expose the Thing object but otherwise the addProperty() call would become terrible. I added a line on Cloud_blink example to show the composition of functions on properties. It can be safely removed in the next iteration.

Fix .readOnly/.writeOnly compisition

4a52413

Add addProperty API with complete signature

e626ea7

Initial testing on OTA

f5f4399

Port to fixed ArduinoCloudThing

ba53001

Add missing libraries

cb76f6f

Fix addProperty macro, again

8bf9cbc

Fix reference as value in addProperty signature

fcffb1b

Call THing.poll with the new API

8f6964d

TEMP: remove OTA stuff

c1c2f43

Add minDelta API and a bunch of overloaded not clashing calls

62d2ffe

work in progress, no secure communication

bf25568

Reconnection ok!

48c3ba8

Added disconnect method

6448bc5

Updated example sketch cloud_blink

270d67b

blink via cloud monitor

3ddf07a

use AWS server

e30b390

connect to vernemq

5ffabab

Adapt to String property type

c87c017

Remove useless dependency on HTTPClient

de50dde

Fix secrets include in example

80bc4dd

Remove nonsense Thing.hasAllReadProperties

ad4788d

TEMP: fully remove OTA support

d570ac2

Alberto Iannaccone added 2 commits August 31, 2018 14:13

wait for 7 seconds only if serial is not found

f5228c8

print compressed cert only in debug mode

2bf5d1b

facchinm force-pushed the vernemq branch from 29da92a to 2bf5d1b Compare August 31, 2018 12:14

sandeepmistry reviewed Aug 31, 2018

View reviewed changes

facchinm mentioned this pull request Aug 31, 2018

[CM-123] Vernemq rebased #8

Merged

facchinm closed this Aug 31, 2018

aentinger deleted the vernemq branch March 19, 2019 10:15

aentinger restored the vernemq branch March 19, 2019 10:15

aentinger deleted the vernemq branch March 19, 2019 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CM-123] VerneMQ integration #7

[CM-123] VerneMQ integration #7

AlbyIanna commented Aug 28, 2018

sandeepmistry commented Aug 28, 2018

sandeepmistry Aug 28, 2018

sandeepmistry Aug 28, 2018

sandeepmistry Aug 28, 2018

AlbyIanna Aug 30, 2018

facchinm Aug 30, 2018

AlbyIanna Aug 30, 2018

cmaglie Aug 30, 2018

sandeepmistry Aug 28, 2018

sandeepmistry Aug 28, 2018

AlbyIanna Aug 30, 2018

sandeepmistry Aug 30, 2018

sandeepmistry Aug 28, 2018

sandeepmistry Aug 28, 2018

facchinm Aug 30, 2018

cmaglie Aug 30, 2018

sandeepmistry Aug 28, 2018

facchinm Aug 29, 2018

sandeepmistry Aug 28, 2018

sandeepmistry Aug 28, 2018

facchinm Aug 30, 2018

AlbyIanna Aug 30, 2018

sandeepmistry Aug 30, 2018

AlbyIanna Aug 31, 2018

sandeepmistry Aug 28, 2018

facchinm Aug 29, 2018

sandeepmistry commented Aug 30, 2018

sandeepmistry Aug 31, 2018

sandeepmistry commented Aug 31, 2018

facchinm commented Aug 31, 2018

		}

		// Just to be able to simulate MKR1000's responses through the serial monitor


		#define addProperty( v, ...) addPropertyReal(v, #v, __VA_ARGS__)

		template<typename T> void addPropertyReal(T& property, String name, permissionType _permission = READWRITE, long seconds = ON_CHANGE, T minDelta = 0, void(*fn)(void) = NULL) {

[CM-123] VerneMQ integration #7

[CM-123] VerneMQ integration #7

Conversation

AlbyIanna commented Aug 28, 2018

sandeepmistry commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeepmistry commented Aug 30, 2018

Choose a reason for hiding this comment

sandeepmistry commented Aug 31, 2018

facchinm commented Aug 31, 2018