Proposal for example unification

[sandeepmistry]

Proposal to resolve #14.

Based on discussion with @cmaglie, we decided to have examples with comments to describe how to run on other boards. The Firmata library takes a similar approach.

These changes also use a NTPClient library, which has a few PR's open that will be released in a new version. A summary of my PR's can be found in the v3 branch of my fork.

I've also introduce a new AzureIoTHubClient client class, which is very minimal for now. However, we can use it as a foundation for providing more Arduino style API's. For example, begin() could accept a connection string parameter, can we could have a connect() API that calls IoTHubClient_LL_CreateFromConnectionString.

We should also remove the use of macros in command_center.ino later.

@stefangordon @obsoleted please review and provide feedback.

[obsoleted]

What do you think about leaving in/combining the #ifdef logic? That way the sample can work as is for all known boards without having to manually edit the includes?

[sandeepmistry]

Using C macros is something we should avoid. It's not "Arduino" style. The Arduino Style Guide for Writing Libraries is a good read, most of the Arduino user base is not familiar with them.

Also, once arduino-libraries/WiFi101#57 is resolved by something like arduino-libraries/WiFi101#58 we can simplify these quite a bit.

[obsoleted]

(@jebrando from Azure iot team has interest in this also)

[obsoleted]

There is nothing to stop folks from creating multiple instances of the class and having the last one "win" in terms of setting the sslclient used by the rest of the library. Is there a pattern other libraries use in this case?

[sandeepmistry]

The RTCZero library has this pattern.

Another option is to pass the sslClient in begin and rename AzureIoTHubClient to AzureIoTHubClientClass and create an instance in the library.

Thoughts?

[obsoleted]

If RTCZero already has this pattern then it is probably OK as is. Since it fits in with the Arduino style.

[obsoleted]

So the best way to test these changes would be to use the ntp library from the following fork right? (https://github.com/sandeepmistry/NTPClient/tree/v3)

[sandeepmistry]

So the best way to test these changes would be to use the ntp library from the following fork right?

@obsoleted yes, this is correct. I'll see if I can get those changes merged in.

[sandeepmistry]

@obsoleted I've enabled write access, so if this PR looks good with your testing please merge and also make minors changes as necessary.

[obsoleted]

Awesome, thanks!

[obsoleted]

Unfortunately it seems the NTPClient library doesn't use the inner sntp methods for the esp board. So while it does execute NTP it doesn't actually set the internal state of the chip so that other parts of the library can benefit (like ssl and the rest of the time methods).

Working on a way to try to minimize the libraries this library depends on, avoids changes to the upstream sdk, and keeps the examples concise.

[sandeepmistry]

@obsoleted that's a shame, would you mind testing the following branch: https://github.com/sandeepmistry/AzureIoT/tree/agenttime

I went ahead and made the change suggested in #17. It's working fine on my MKR1000. I found an Olimex ESP8266 board to test with, but unfortunately after it makes the HTTPS request and receives the response it crashes while free'ing the headers.

One note the simplesample_http.c uses time(NULL) instead of `get_time(NULL)``.

[obsoleted]

I'll look at this today. I still suspect there might be issues with not using the internal ntp stuff on esp. If even that we end up increasing the size of the sketch by including a redundant ntp implementation.

I've got a couple different esp8266 boards that i can try it out with. Will see.

[sandeepmistry]

Thanks, same thing is happening with the Huzzah. I've been using v2.1.0 of the esp8266 core. I also tried adding the configTime call in.

Which version of the core are you testing with?

[obsoleted]

I've been using the latest from git or the staging release: http://arduino.esp8266.com/staging/package_esp8266com_index.json

I think the latest staging is 2.2.0-rc1

[obsoleted]

And yeah, configTIme should kick off the internal ntp stuff on esp. Then at some point time will return the value when it is completed. This is roughly similar to the ntp update loop.

That being said we should still keep this PR scoped to the sample unification (even if it results in more commented out code for other boards).

I think what you have here is close to working I just need to add a little bit for the esp side. I'll send that out when i can.

[obsoleted]

ok, pushed an update here: https://github.com/obsoleted/AzureIoT/tree/example-unification

I've validated the simple sample on esp and samd. Will need some more time to check the others.

[obsoleted]

Other sample verified also.

[sandeepmistry]

Great, I've cherry-picked your commit into my fork/branch and merged!

[sandeepmistry]

FWIW, this is working on my Huzzah board now with 2.1 stable release. The changes from b52ef5a have resolved the crashing behaviour I was seeing last week.