Add GSM and Ethernet conn_manager + add fallback UDP based getTime() #31

facchinm · 2019-02-12T11:20:12Z

No description provided.

Some operators return bogus getTime; if reported value is too low (generating it at compile time would be awesome) use fallback mode.

aentinger · 2019-02-18T05:38:47Z

src/GSMConnectionManager.h

+  int connectionTickTimeInterval;
+  GSMUDP Udp;
+  void sendNTPpacket(const char * address, uint8_t* packetBuffer);
+  unsigned long getNTPTime();
 };

 static const unsigned long NETWORK_CONNECTION_INTERVAL = 30000;

 GSMConnectionManager::GSMConnectionManager(const char *pin, const char *apn, const char *login, const char *pass) :


The member variables pin, apn, login and pass are only stored as const char pointers. This implies that those variables must be of global scope within the calling code, otherwise we might read invalid data if we read them at a later point in time. A alternative would be to copy the content of those variables into the class, e.g. via a String data type.

I agree, the structure is the same as WiFi wrapper that "requires" variables with global scope. I'd be ok to switch to strings, but it should be something shared between all the wrappers.

@lxrobotics @facchinm
because we use them in the Web IDE to create fields in the Secret tab, they are indeed globally defined for our cases.
storing them as String might make sense. I also side with @facchinm with having this kind of structure shared amongst the wrappers

src/utility/NTPUtils.cpp

src/EthernetConnectionManager.h

Co-Authored-By: facchinm <[email protected]>

ilcato · 2019-02-20T13:55:22Z

src/EthernetConnectionManager.h

+          return;
+        }
+        break;
+      case CONNECTION_STATE_GETTIME:


Add an exit condition from the CONNECTION_STATE_GETTIME state in a similar way of the WiFiConnectionManager.

ubidefeo · 2019-02-20T15:22:15Z

Fellows, I've been going through the source and although it will work with just a couple rectifications (such as @ilcato 's note related to MAX_GETTIME_RETRY), I've noticed that it needs a few adjustments to align to the structure of WiFiConnectionManager especially in matter of logging output.
I have used a support method to change state in order to add extra code into its enclosed cases rather than polluting the check() method: changing time interval, logging one message, cleanup and so on.
I expect the same amount of work to be required for EthernetConnectionManager.
I'm working on it :)

- minor refactoring on WiFiConnectionManager

- changed library version to 0.5.1 (new feature: GSM)

- aligned INTERVAL definitions between WiFi and GSM

- fixed typo

ubidefeo · 2019-02-21T19:46:09Z

ok folks
I have done my work, there are some conflicts related to implementations I brought back from master to this branch and changes I made after.
I have also bumped up version in library.properties as this should be a tentative 0.5.1 version with a stable GSMConnectionManager and a refined WiFiConnectionManager.
This took a lot of work, and I have tested it as I went with no errors whatsoever.
I am confident it's flawless, but I'd really need some help reviewing it as I've been at it for over 7 hours today only.
Let me know and hopefully we can merge it tomorrow for Monday.
Goes without saying that I have to reserve the same lifting treatment to EthernetConnectionManager which I have not even looked at.
Thanks :)

- fixed typo in example - cleanup GSMConnectionManager - cleanup WiFiConnectionManager - fixed missing WiFiUDP member which broke compilation

- removed unnecessary call to _mqttReconnect(...) in ArduinoIoTCloud - removed unncessary begin() into case IOT_STATUS_IDLE in ArduinoIoTCloud - removed unnecessary calls to update() in CloudSerial - fixed if statement in ConnectionManager

examples/ArduinoIoTCloud_LED_switch/arduino_secrets.h

examples/ArduinoIoTCloud_LED_switch/thingProperties.h

src/GSMConnectionManager.h

examples/ArduinoIoTCloud_LED_switch/arduino_secrets.h

examples/ArduinoIoTCloud_LED_switch/thingProperties.h

src/ConnectionManager.h

aentinger

Looks good to me 👍

aentinger · 2019-02-26T05:51:13Z

examples/ArduinoIoTCloud_LED_switch/thingProperties.h

+ The following include line is used for WiFi enabled boards (MKR1000, MKR WiFi 1010)
+*/
+ConnectionManager *ArduinoIoTPreferredConnection = new WiFiConnectionManager(SECRET_SSID, SECRET_PASS);


A quick side note: Everything that follows is a suggestion for improvement and must not necessarily implemented within this PR. We can (and probably should) do cleanups in further PRs after this is merged.

A misconception, for which I've fallen myself until quite recently, is that if you instantiate derived classes with virtual functions in it (C++ runtime polymorphism) you always need to use the keyword new, effectively allocating the memory on the heap at runtime. Interestingly enough, the following statement also works:

Suggested change

ConnectionManager *ArduinoIoTPreferredConnection = new WiFiConnectionManager(SECRET_SSID, SECRET_PASS);

WiFiConnectionManager connection_mgr(SECRET_SSID, SECRET_PASS);

Doing it this way the memory for the object is allocated on the stack and not on the heap (static memory allocation vs dynamic memory allocation - prefer the former whenever possible/feasible). The reason above statement works is because of a C++ "feature" called object slicing.

Now object slicing has some pitfalls to it but it can be used effectively in this scenario where we have an abstract base class (= one or more pure virtual functions in it) ConnectionManager from which WiFiConnectionManager and GSMConnectionManager are derived. ConnectionManager serves to define the interface for the derived classes and only those functions defined within ConnectionManager are called by users of the derived classes.

When passing the connection around to various classes you then have to use pass-by-reference.

void myFunc(ConnectionManager & connection_mgr) { ...

It´s also possible to store a reference of it as a class member variable.

class MyClass { public: void MyClass(ConnectionManager & connection_mgr) : _connection_mgr(connection_mgr) { ... } void myMemberFunc(); private: ConnectionManager & _connection_mgr; };

The ConnectionManager can than be accessed within member functions via the . operator instead of -> as would be the case if the ConnectionManager would be stored as a pointer (ConnectionManager * _connection_mgr;)

void MyClass:myMemberFunc() { _connection_mgr.check(); }

Furthermore I'd also suggest to keep variable names in lowercase so to not confuse them with class/type names.

interesting...
I'm gonna level up on this stuff for the next version.
Thanks, @lxrobotics

I'll proceed to merge this PR in :)

aentinger · 2019-02-26T05:51:40Z

examples/ArduinoIoTCloud_LED_switch/thingProperties.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)
 // Your THING_ID


Suggested change

// Your THING_ID

// Enter your THING_ID here

aentinger · 2019-02-26T05:57:31Z

src/ArduinoIoTCloud.h

@@ -43,14 +43,12 @@ typedef struct {
 extern ConnectionManager *ArduinoIoTPreferredConnection;

 enum ArduinoIoTConnectionStatus {


Maybe it would be an idea to convert the enum into a enum class?

Suggested change

enum ArduinoIoTConnectionStatus {

enum class CloudConnectionState {

Idle,

Connecting,

Connected,

...

@lxrobotics
I've been looking up enum class and I think it's a great way to go, but not for this PR.
We're awaiting on this merge to move forward and release MKR GSM support which after this is merged in will be ready.
this will definitely end up in the next update :)

aentinger · 2019-02-26T05:59:34Z

src/ArduinoIoTCloud.h

@@ -130,7 +128,7 @@ class ArduinoIoTCloudClass {
  ArduinoIoTConnectionStatus getIoTStatus() { return iotStatus; }
  void setIoTConnectionState(ArduinoIoTConnectionStatus _newState);
 private:
-  ArduinoIoTConnectionStatus iotStatus = IOT_STATUS_IDLE;
+  ArduinoIoTConnectionStatus iotStatus = IOT_STATUS_CLOUD_IDLE;


In case you want to go ahead with an enum class:

Suggested change

ArduinoIoTConnectionStatus iotStatus = IOT_STATUS_CLOUD_IDLE;

CloudConnectionState _iot_cloud_state = CloudConnectionState::Idle;

same as above

facchinm added 7 commits February 12, 2019 12:17

GSM: implement full state machine

db14c67

GSM: add fallback getTime retrieve

68d2c3c

Some operators return bogus getTime; if reported value is too low (generating it at compile time would be awesome) use fallback mode.

Merge branch 'master' into gsm_proper_connmanager

1324ac6

Create NTP util class to handle getTime() fallback

19c688e

Restructure GSM connection manager

e0abeb8

Automatically handle getTime fallback in IoTCloud class

75ddfcc

Add EthernetConnectionManager

da7179b

facchinm mentioned this pull request Feb 13, 2019

TEST: try introducing Ethernet based connection manager #26

Closed

facchinm changed the title ~~Add proper state machine for GSM connections~~ Add GSM and Ethernet conn_manager + add fallback UDP based getTime() Feb 13, 2019

facchinm requested review from ubidefeo and cmaglie February 13, 2019 13:22

ubidefeo requested a review from aentinger February 15, 2019 15:18

aentinger reviewed Feb 18, 2019

View reviewed changes

src/utility/NTPUtils.cpp Show resolved Hide resolved

aentinger reviewed Feb 18, 2019

View reviewed changes

src/EthernetConnectionManager.h Outdated Show resolved Hide resolved

[EthConnectionManager] Make "now" const

e01a981

Co-Authored-By: facchinm <[email protected]>

ilcato reviewed Feb 20, 2019

View reviewed changes

- added link to NPT on Arduino Tutorial and WikiPedia

4014095

ubidefeo added 6 commits February 21, 2019 17:29

- refactor GSMConnectionManager

a243573

- minor refactoring on WiFiConnectionManager

- cleanup

096022a

- cleanup WiFi and GSM ConnectionManager

98efd08

- changed library version to 0.5.1 (new feature: GSM)

- whitespace cleanup

e905238

- aligned INTERVAL definitions between WiFi and GSM

- more cleanup and minor refactoring

8646505

- reworked sentence

8eefcc0

- fixed typo

ubidefeo added 3 commits February 21, 2019 22:21

- extended example with GSM compatibility options

e4e11e5

- fixed typo in example - cleanup GSMConnectionManager - cleanup WiFiConnectionManager - fixed missing WiFiUDP member which broke compilation

- switch state cleanup in ArduinoIoTCloud

7fc775b

per1234 requested changes Feb 23, 2019

View reviewed changes

Merge branch 'master' into gsm_proper_connmanager

bff994e

per1234 requested changes Feb 23, 2019

View reviewed changes

src/ConnectionManager.h Show resolved Hide resolved

ubidefeo added 3 commits February 24, 2019 09:50

- applied per1234's suggestions on #31

ae7dae9

- one more missing from per1234's list

f86f04f

- implemented some of the changes requested by @ilcato on PR #37

961a71c

ilcato mentioned this pull request Feb 25, 2019

Managed exit cases for IOT_STATUS_CLOUD_CONNECTED, removed unused states #37

Closed

- ArduinoIoTConnectionStatus cleanup as per @ilcato 's suggestion

fb161bd

ubidefeo requested a review from aentinger February 25, 2019 14:48

aentinger approved these changes Feb 26, 2019

View reviewed changes

ubidefeo merged commit 25c5b71 into master Feb 26, 2019

aentinger deleted the gsm_proper_connmanager branch February 27, 2019 06:26

	ConnectionManager *ArduinoIoTPreferredConnection = new WiFiConnectionManager(SECRET_SSID, SECRET_PASS);
	WiFiConnectionManager connection_mgr(SECRET_SSID, SECRET_PASS);

		@@ -43,14 +43,12 @@ typedef struct {
		extern ConnectionManager *ArduinoIoTPreferredConnection;

		enum ArduinoIoTConnectionStatus {

-enum ArduinoIoTConnectionStatus {
+enum class CloudConnectionState {
+  Idle,
+  Connecting,
+  Connected,
+  ...

	ArduinoIoTConnectionStatus iotStatus = IOT_STATUS_CLOUD_IDLE;
	CloudConnectionState _iot_cloud_state = CloudConnectionState::Idle;

Add GSM and Ethernet conn_manager + add fallback UDP based getTime() #31

Add GSM and Ethernet conn_manager + add fallback UDP based getTime() #31

Uh oh!

Conversation

facchinm commented Feb 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ubidefeo commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ubidefeo commented Feb 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aentinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ubidefeo commented Feb 20, 2019 •

edited

Loading