Thing id discovery protocol #297

pennam · 2022-01-12T08:51:56Z

The Goal of this PR is to remove the hardcoded thing_id from the sketch and improve OTA flow.

Properties spit:

Propetries are splitted in device_properties and thing_properties with 2 separate containers.
Thing configuration and OTA will use device_properties.
User data will use thing_properties.

Thing_id:

No hardcoded thing_id, the board will receive the thing_id from the cloud after subscribing to device_id topic.
After receiving the thing_id the general flow is unchanged and the board will start listen on thing_shadow_topic for LastValues and on thing_input_topic for InputValues.

OTA flow:

Send OTA_CAP, OTA_ERROR, and OTA_SHA256 as soon as connected to the broker to close any pending OTA process.
Use a dedicated topic for OTA and thing configuration.

f2d0bdf and 9305761 are here only for testing purpose and will be dropped before merging.

Thing ID configuration ✔️
Attach/Detach action
OTA ✔️
Deferred OTA ✔️

codecov-commenter · 2022-01-12T08:53:55Z

Codecov Report

Merging #297 (d436d33) into master (b0b75cd) will not change coverage.
The diff coverage is n/a.

❗ Current head d436d33 differs from pull request most recent head 1fdd6ad. Consider uploading reports for the commit 1fdd6ad to get more accurate results

@@           Coverage Diff           @@
##           master     #297   +/-   ##
=======================================
  Coverage   94.87%   94.87%           
=======================================
  Files          27       27           
  Lines        1113     1113           
=======================================
  Hits         1056     1056           
  Misses         57       57

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0b75cd...1fdd6ad. Read the comment docs.

aentinger

This PR is quite hard for me to review due to lack of knowledge of the background info re thing id update, consequently the value of my advice is probably limited. Nonetheless, here are a couple of thoughts 🙇.

src/ArduinoIoTCloudTCP.cpp

aentinger · 2022-02-03T08:26:17Z

src/ArduinoIoTCloudTCP.cpp

-  std::for_each(ota_property_list.cbegin(),
-                ota_property_list.cend(),
+  std::list<String> ota_property_list {"OTA_CAP", "OTA_ERROR", "OTA_SHA256"};
+  if (include_ota_req)


I'd suggest to either enclose the expression below within braces, i.e.

if (include_ota_req) { ota_property_list.push_back("OTA_REQ"); }

or to leave one free line below the statement before and the statement after. Otherwise one get's all expressions clustered up and its not easy to see what belongs to the if statement and what not.

fully agree, this was part of the initial changes i've done to support OTA with this new flow, then i've changed this function in this commit e6b252c and again here 98ed735 so it is already fixed

aentinger · 2022-02-03T08:32:15Z

src/ArduinoIoTCloudTCP.cpp

@@ -69,6 +69,11 @@ extern "C" void updateTimezoneInfo()
  ArduinoCloud.updateInternalTimezoneInfo();
 }

+extern "C" void setThingIdOutdated()


Where is this function called? Why is there an extern "C" declaration necessary? (Looks like it's passed to C code as a callback function).

This function is used as callback function fot the update event of the _thing_id property. Each time a new thing_id value is received this function is called to restart a new configuration process. It should not be necessary, i think i've copy pasted from

extern "C" void updateTimezoneInfo()

that i have again copy pasted from

extern "C" unsigned long getTime()

but i think it is safe to remove all three extern "C" declarations

Allright, but where is this function registered as a callback?

Here db3e943 i think i've messed up a bit rebasing this firsts commits. I'll fix it keeping

addPropertyReal(_thing_id, _device_property_container, "thing_id", Permission::ReadWrite) in the first and adding

onUpdate(setThingIdOutdated); in the second one

i've fixed the history and force pushed so now also the first commit builds correctly and the other are consistent.

I've also added a new commit to remove the extern "C" declaration see 0541f7b

aentinger · 2022-02-03T08:34:53Z

src/ArduinoIoTCloudTCP.cpp

-      /* Unsubscribe from old things topics and go oi with a new subsctiption */
-      _mqttClient.unsubscribe(_shadowTopicIn);
-      _mqttClient.unsubscribe(_dataTopicIn);
+    /* Unsubscribe from old things topics and go oi with a new subsctiption */


Typo in comment, oi -> on (I guess).

yes, it was already fixed here defe4dd i know it has nothig to do with delay macros... i will create a separate commit and squash it to the original one

Removed the fix from here defe4dd and squashed directly here a0916cd

aentinger · 2022-02-03T08:39:10Z

src/ArduinoIoTCloudTCP.cpp

    _time_service.setTimeZoneData(_tz_offset, _tz_dst_until);
    execCloudEventCallback(ArduinoIoTCloudEvent::SYNC);
    _last_sync_request_cnt = 0;
    _last_sync_request_tick = 0;
-    _state = State::Connected;
+    _next_state = State::Connected;


Is it really necessary to hold _next_state as a private member variable and therefore make it part of the overall state of an ArduinoIoTCloudTCP object? I'm sceptical of any additional member variable since each member variable equals at least two (bool) or many more possible states. Therefore many member variables equal exploding state space which makes it more easy to introduce new bugs and more hard to test all possible state transition paths.

need to doublecheck, but i'm quite confident this commit can be dropped, because State::CheckDeviceConfig logic as changed after this commit.

i've dropped this commit since at the end of the whole work it wasn't necessary.

src/ArduinoIoTCloudTCP.cpp

pennam · 2022-02-03T13:42:56Z

i've found a stray debug print, in the first commit so i will force push again in a while

…operty container

…protocol

pennam · 2022-02-04T13:18:04Z

dropping 912b068 and force-pushing again since setThingId() it is still needed for LORA device

… to append OTA_REQ property

…other messages on device topic

…dle_Disconnect() function

…is not attached to a thing

…vice_topic for OTA properties

…DevicePropertiesToCloud(). Provide functions to clear OTA_REQ flag and update OTA_ERROR and OTA_URL.

…IBE_RETRY_DELAY

…"Disconnected from Arduino IoT Cloud" on serial monitor

…cloud

github-actions · 2022-02-08T11:43:04Z

Memory usage change @ 93d6fa8

Board	flash	%	RAM for global variables	%
arduino:mbed:envie_m4	🔺 +1640 - +2504	+0.16 - +0.24	🔺 +72 - +72	+0.02 - +0.02
arduino:mbed:envie_m7	🔺 +1352 - +1568	+0.17 - +0.2	🔺 +64 - +64	+0.01 - +0.01
arduino:mbed_nano:nanorp2040connect	🔺 0 - +1478	0.0 - +0.01	🔺 0 - +64	0.0 - +0.02
arduino:samd:mkr1000	🔺 +1696 - +2360	+0.65 - +0.9	🔺 +72 - +72	+0.22 - +0.22
arduino:samd:mkrgsm1400	🔺 +1456 - +1648	+0.56 - +0.63	🔺 +64 - +64	+0.2 - +0.2
arduino:samd:mkrnb1500	🔺 +1696 - +2360	+0.65 - +0.9	🔺 +72 - +72	+0.22 - +0.22
arduino:samd:mkrwan1300	🔺 +72 - +264	+0.03 - +0.1	🔺 +24 - +32	+0.07 - +0.1
arduino:samd:mkrwifi1010	🔺 0 - +1624	0.0 - +0.62	🔺 0 - +64	0.0 - +0.2
arduino:samd:nano_33_iot	🔺 0 - +1616	0.0 - +0.62	🔺 0 - +64	0.0 - +0.2
esp32:esp32:esp32	🔺 +2560 - +3924	+0.2 - +0.3	🔺 +80 - +80	+0.02 - +0.02
esp8266:esp8266:huzzah	🔺 +1800 - +2680	+0.17 - +0.26	🔺 +212 - +212	+0.26 - +0.26

Click for full report table

Board	examples/ArduinoIoTCloud-Advanced flash	%	examples/ArduinoIoTCloud-Advanced RAM for global variables	%	examples/ArduinoIoTCloud-Basic flash	%	examples/ArduinoIoTCloud-Basic RAM for global variables	%	examples/utility/ArduinoIoTCloud_Travis_CI flash	%	examples/utility/ArduinoIoTCloud_Travis_CI RAM for global variables	%	examples/utility/Provisioning flash	%	examples/utility/Provisioning RAM for global variables	%	examples/utility/SelfProvisioning flash	%	examples/utility/SelfProvisioning RAM for global variables	%
arduino:mbed:envie_m4	2440	0.23	72	0.02	2504	0.24	72	0.02	1640	0.16	72	0.02	2160	0.21	72	0.02
arduino:mbed:envie_m7	1352	0.17	64	0.01	1488	0.19	64	0.01	1544	0.2	64	0.01	1568	0.2	64	0.01
arduino:mbed_nano:nanorp2040connect	1314	0.01	64	0.02	1422	0.01	64	0.02	1478	0.01	64	0.02	1458	0.01	64	0.02	0	0.0	0	0.0
arduino:samd:mkr1000	2296	0.88	72	0.22	2352	0.9	72	0.22	1696	0.65	72	0.22	2360	0.9	72	0.22
arduino:samd:mkrgsm1400	1456	0.56	64	0.2	1560	0.6	64	0.2	1616	0.62	64	0.2	1648	0.63	64	0.2
arduino:samd:mkrnb1500	2288	0.87	72	0.22	2352	0.9	72	0.22	1696	0.65	72	0.22	2360	0.9	72	0.22
arduino:samd:mkrwan1300	80	0.03	32	0.1	72	0.03	24	0.07	264	0.1	24	0.07
arduino:samd:mkrwifi1010	1432	0.55	64	0.2	1528	0.58	64	0.2	1592	0.61	64	0.2	1624	0.62	64	0.2	0	0.0	0	0.0
arduino:samd:nano_33_iot	1432	0.55	64	0.2	1528	0.58	64	0.2	1592	0.61	64	0.2	1616	0.62	64	0.2	0	0.0	0	0.0
esp32:esp32:esp32	3740	0.29	80	0.02	3924	0.3	80	0.02	2560	0.2	80	0.02
esp8266:esp8266:huzzah	2616	0.25	212	0.26	2680	0.26	212	0.26	1800	0.17	212	0.26

Click for full report CSV

Board,examples/ArduinoIoTCloud-Advanced<br>flash,%,examples/ArduinoIoTCloud-Advanced<br>RAM for global variables,%,examples/ArduinoIoTCloud-Basic<br>flash,%,examples/ArduinoIoTCloud-Basic<br>RAM for global variables,%,examples/utility/ArduinoIoTCloud_Travis_CI<br>flash,%,examples/utility/ArduinoIoTCloud_Travis_CI<br>RAM for global variables,%,examples/utility/Provisioning<br>flash,%,examples/utility/Provisioning<br>RAM for global variables,%,examples/utility/SelfProvisioning<br>flash,%,examples/utility/SelfProvisioning<br>RAM for global variables,%
arduino:mbed:envie_m4,2440,0.23,72,0.02,2504,0.24,72,0.02,1640,0.16,72,0.02,2160,0.21,72,0.02
arduino:mbed:envie_m7,1352,0.17,64,0.01,1488,0.19,64,0.01,1544,0.2,64,0.01,1568,0.2,64,0.01
arduino:mbed_nano:nanorp2040connect,1314,0.01,64,0.02,1422,0.01,64,0.02,1478,0.01,64,0.02,1458,0.01,64,0.02,0,0.0,0,0.0
arduino:samd:mkr1000,2296,0.88,72,0.22,2352,0.9,72,0.22,1696,0.65,72,0.22,2360,0.9,72,0.22,,,,
arduino:samd:mkrgsm1400,1456,0.56,64,0.2,1560,0.6,64,0.2,1616,0.62,64,0.2,1648,0.63,64,0.2,,,,
arduino:samd:mkrnb1500,2288,0.87,72,0.22,2352,0.9,72,0.22,1696,0.65,72,0.22,2360,0.9,72,0.22,,,,
arduino:samd:mkrwan1300,80,0.03,32,0.1,72,0.03,24,0.07,264,0.1,24,0.07,,,,,,,,
arduino:samd:mkrwifi1010,1432,0.55,64,0.2,1528,0.58,64,0.2,1592,0.61,64,0.2,1624,0.62,64,0.2,0,0.0,0,0.0
arduino:samd:nano_33_iot,1432,0.55,64,0.2,1528,0.58,64,0.2,1592,0.61,64,0.2,1616,0.62,64,0.2,0,0.0,0,0.0
esp32:esp32:esp32,3740,0.29,80,0.02,3924,0.3,80,0.02,2560,0.2,80,0.02,,,,,,,,
esp8266:esp8266:huzzah,2616,0.25,212,0.26,2680,0.26,212,0.26,1800,0.17,212,0.26,,,,,,,,

pennam force-pushed the thing_id_discovery branch 2 times, most recently from 2efd398 to ff1d6c6 Compare January 12, 2022 10:07

arduino-libraries deleted a comment from github-actions bot Jan 12, 2022

pennam force-pushed the thing_id_discovery branch from ff1d6c6 to 58b2603 Compare January 12, 2022 10:19

arduino-libraries deleted a comment from github-actions bot Jan 12, 2022

pennam mentioned this pull request Jan 18, 2022

Add retry delay on topic subscription failures #298

Merged

zmoog mentioned this pull request Jan 20, 2022

THING ID becomes optional, as DEVICE ID is solely used for topic identifcation #259

Closed

pennam force-pushed the thing_id_discovery branch from 684b5b0 to 9305761 Compare January 24, 2022 15:33

arduino-libraries deleted a comment from github-actions bot Jan 24, 2022

pennam requested review from polldo and aentinger January 24, 2022 16:19

pennam force-pushed the thing_id_discovery branch from 9305761 to de65b3f Compare January 24, 2022 16:34

aentinger reviewed Feb 3, 2022

View reviewed changes

pennam force-pushed the thing_id_discovery branch from de65b3f to 068ae20 Compare February 3, 2022 13:27

pennam added 2 commits February 3, 2022 14:43

Split _property_container in _device_property_container and _thing_pr…

fc0ddb9

…operty container

ArduinoIoTCloudTCP state machine changes to handle thig_id discovery …

1af4701

…protocol

pennam force-pushed the thing_id_discovery branch from 068ae20 to d436d33 Compare February 3, 2022 13:59

arduino-libraries deleted a comment from github-actions bot Feb 3, 2022

pennam added 8 commits February 4, 2022 14:18

If board has a valid _dataTopicIn also _shadowTopicIn is valid

f4af0d2

Add configuration flag to sendOTAPropertiesToCloud() fuction in order…

79656c4

… to append OTA_REQ property

Execute handle_CheckDeviceConfig() only on thing_id changes ignoring …

48494fa

…other messages on device topic

Do not try to reconfigure device if connection is dropped

65b82e6

There is no more distinction between not configured and not attached

850095d

Move thing_id outdated flag check outside update function and add han…

08964b3

…dle_Disconnect() function

Use specific macros to define device topic subscription delays

95eeec8

Use incremental long delay before retry subscribtion when the device …

ae45d0c

…is not attached to a thing

pennam added 9 commits February 4, 2022 14:18

Add LIB_VERSION property to let cloud decide to use thing_topic or de…

221b176

…vice_topic for OTA properties

Spit sendPropertiesToCloud() in sendThingPropertiesToCloud() and send…

4aa628a

…DevicePropertiesToCloud(). Provide functions to clear OTA_REQ flag and update OTA_ERROR and OTA_URL.

Remove the onSync callback end event from device properties

0ebefe8

Use a single function to keep OTA properties in sync with cloud

366bde1

Use better define name to distinguish between topic and device SUBSCR…

e0a1d5a

…IBE_RETRY_DELAY

Remove extern C declaration from local module functions

12d3b26

Do not print Thing ID at startup because its value will be ignored

c35be6f

Print Thing ID on serial monitor when board connects to the cloud

c9cd3f5

Call DISCONNECT event when device is detached from a thing and print …

d373405

…"Disconnected from Arduino IoT Cloud" on serial monitor

pennam force-pushed the thing_id_discovery branch from 1fdd6ad to d373405 Compare February 4, 2022 13:18

arduino-libraries deleted a comment from github-actions bot Feb 7, 2022

Add DEBUG_ERROR if device fails to subscribe to _deviceTopicIn

4a121ae

pennam changed the title ~~[POC] Thing id discovery protocol~~ Thing id discovery protocol Feb 8, 2022

Add DEBUG_ERROR if device is not receiving a valid thing_id from the …

93d6fa8

…cloud

pennam force-pushed the thing_id_discovery branch from 583ec5b to 93d6fa8 Compare February 8, 2022 10:38

arduino-libraries deleted a comment from github-actions bot Feb 8, 2022

pennam merged commit 9cc43f7 into arduino-libraries:master Feb 8, 2022

pennam mentioned this pull request Nov 14, 2022

Reorder and simplify addPropertyReal methods #343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thing id discovery protocol #297

Thing id discovery protocol #297

pennam commented Jan 12, 2022 •

edited

Loading

codecov-commenter commented Jan 12, 2022 •

edited

Loading

aentinger left a comment

aentinger Feb 3, 2022

pennam Feb 3, 2022 •

edited

Loading

aentinger Feb 3, 2022

pennam Feb 3, 2022

aentinger Feb 3, 2022

pennam Feb 3, 2022

pennam Feb 3, 2022

pennam Feb 3, 2022

aentinger Feb 3, 2022

pennam Feb 3, 2022

pennam Feb 3, 2022

aentinger Feb 3, 2022

pennam Feb 3, 2022

pennam Feb 3, 2022

pennam commented Feb 3, 2022

pennam commented Feb 4, 2022

github-actions bot commented Feb 8, 2022

Thing id discovery protocol #297

Thing id discovery protocol #297

Conversation

pennam commented Jan 12, 2022 • edited Loading

codecov-commenter commented Jan 12, 2022 • edited Loading

Codecov Report

aentinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pennam Feb 3, 2022 • edited Loading

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

pennam commented Feb 3, 2022

pennam commented Feb 4, 2022

github-actions bot commented Feb 8, 2022

pennam commented Jan 12, 2022 •

edited

Loading

codecov-commenter commented Jan 12, 2022 •

edited

Loading

pennam Feb 3, 2022 •

edited

Loading