astarte-platform / astarte-device-sdk-esp32 Goto Github PK

The chars /, + and # should not be allowed in endpoint parameters.
This is due to MQTT specifications. As + and # are used as wildcards and / is used as a path separator.

Possible memory leak during the creation of the payload for the credentials request in the pairing API.
The data struct created with cJSON_CreateObject does not get deleted using cJSON_Delete.
Only the root parent gets deleted, but as specified in the cJSON documentation calling delete on an object does not delete its content.

Currently we don't have anything well detailed/up to date on how to use the SDK. Ensure we improve this

Wiping Astarte partition and starting again from scratch (using make erase_flash) might be useful, document it.

The struct astarte_interface_t contains all the information the libraries have regarding an existing interface.

An alternative would be to load in non-volatile memory the full json file and implement a deserialization utility that could parse the json file at runtime filling the struct dynamically.
To achieve this when using a FAT based file system the partition generator provided by ESP could be used.

Currently this has to be done manually modifying the HTTP and MQTT client configurations, it would be a good idea to allow configuring this at the Astarte Device level.

Some credentials functions are marked as "might change in the future" with the following message included in the docstring:

When migrating to Astarte Device SDK ESP32 1.0 from 0.11 CONFIG_ASTARTE_HWID_ENABLE_UUID should be disabled, otherwise device id will be generated differently.
This step should be documented.

The UUID module does not include the astarte_ prefix as all other modules do.

The astarte_credentials_has_certificate function only checks if the certificate file exists, but performs no checks over the certificate file to assess its validity.

See function below:

The device knowledge regarding the interfaces is limited to name, version, ownership, and type.
It should be evaluated how to extend the knowledge of the device regarding the interfaces to include the endpoints of each interface.
This would allow checks to be performed before transmission.

Missing support for newer version of the IDF 5.0.

Some functions in the astarte BSON serializer use some of the arguments to provide outputs. However, they do not check the validity of such pointers.

In some use cases, the Credentials Secret might need to be set at runtime. As such, exporting the save_credentials_secret function (probably with a different name) would suffice to enable the user to autonomously set the credentials secret without resorting to build time configurations.

Right now the user can provide an astarte_device_data_event_callback_t which gets called when data is received from Astarte, but it would be useful to expose the connection status of the device via callbacks.

The maximum length of JWT used in HTTP Header for registering a device is 1024, It is defined as constant here:

At this time, if you have generated, for example a pairing_token greater than 1024, it will be truncated causing the device registration to fail.
This can be occurred when a JWT was generated from RSA key.

The SDK should return meaningful errors upon specific errors on the Astarte wire. Currently, a generic ASTARTE_ERR is returned, but more details should be given when possible (e.g.: something like ASTARTE_BAD_CREDENTIALS when pairing doesn't succeed with a 401/403)

Astarte MQTT v1 strives to save bandwidth upon reconnections, to make sure even frequent reconnections don't affect bandwidth consumption. As such, upon connecting and if MQTT advertises a session present, both sides assume that data flow is ordered and consistent. However, there might be cases where this guarantee isn't respected by the device for a number of reasons (e.g.: new device, factory reset, cache lost...). In this case, a device might declare that it has no confidence about its status and its known properties, and can request to resynchronise entirely with Astarte.

In Astarte jargon this message is called empty cache and it is performed by publising 1 on the device /control/emptyCache topic.

After an empty cache message properties might be purged and Astarte might publish all the server owned properties again.

The messages in the astarte_device.c publish_data function are managed using a semaphore that inhibits the publication of two messages at the same time.

A mechanism should be introduced whereby messages are held until the Astarte device client is ready to process them, like a message queue.

In #110 a series of checks have been disabled as they would generate warnings and block the CI.
The long-term objective is to re-enable some of those checks while fixing the generated warnings.
The list of checks that should be re-enabled is the following:

• bugprone-macro-parentheses #139
  Ensures macros have parenthesis around their content.
• bugprone-suspicious-string-compare #139
  Warns when using the result of strncmp() in a easy to misinterpret way.
• cert-err33-c #150
  Forces to always use function results.
• clang-analyzer-deadcode.DeadStores #139
  Warns when a value is assigned to a variable but never used.
• clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling #139
  Identifies some obsolete and unsafe functions from the standard library. This check refers to functions not supported by the compiler we use, it should be ignored.
• cppcoreguidelines-init-variables #145
  Warns when using uninitialized variables.
• cppcoreguidelines-narrowing-conversions #148
  Warns on narrowing implicit conversion.
• hicpp-signed-bitwise #146
  Warns on bitwise operations on signed numbers.
• misc-unused-parameters #147
  Warns against dangling parameters.
• readability-else-after-return #139
  Warns against useless else after a return.
• readability-identifier-length #149
  Warns against too short identifiers.
• readability-identifier-naming #151
  Sets some strict rules for naming of functions/variables/params/etc.
• readability-inconsistent-declaration-parameter-name #139
  Warns when functions definition and declaration have different parameters names.
• readability-magic-numbers #176
  Warns when magic numbers are used.
• readability-uppercase-literal-suffix #139
  Warns when using literal suffices with non-capital letters (e.g. 1u is not ok, 1U is ok, 1 is ok).

The following rules are aliases for some of the rules above:

• bugprone-narrowing-conversions (cppcoreguidelines-narrowing-conversions) #148
• cppcoreguidelines-avoid-magic-numbers (readability-magic-numbers) #176
• hicpp-uppercase-literal-suffix (readability-uppercase-literal-suffix) #139
• llvm-else-after-return (readability-else-after-return) #139

To bring the ESP32 up to speed with other SDKs, the properties cache should be handled within the SDK ensuring server and device properties are synced up and down and emptyCache is handled.

The current example for this SDK presents the following problems:

• No detailed how to start is present
• Requires a button and an LED to be present on the board.
• Only shows how to transmit/receive an individual datastream.

None of the functions in the astarte_bson.h file is documented, making them very difficult to use.

N.B. These functions are to be used to parse a received BSON file.

Documentation cannot be found at https://docs.astarte-platform.org/1.0/device-sdks/esp32

REUSE compliance checker should be added to the CI

The hwid generated with IDF v5.0 is different compared to the one generated from previous versions of the IDF.
The issue spawns from the esp_chip_info function called in the astarte_hwid_get_id function.

The field revision for the type esp_chip_info_t has changed content in this latest version of the ESP IDF.
In the newer version of the IDF the revision field contains the chip revision version, in the format MXX. Where M - wafer major version, XX - wafer minor version.
In previous versions of the IDF, only the major version was contained in the revision field.
Check out the related sections in the migration guide for more info.

Hi,

Tried to build astarte-device-sdk-esp32 (master branch) SDK's 'toggle_led' example but the build system throws following error:
............................................................................................................................
astarte-device-sdk-esp32/astarte_device.c:554:5: error: enumeration value 'MQTT_EVENT_BEFORE_CONNECT' not handled in switch [-Werror=switch]
switch (event->event_id) {
^
cc1: some warnings being treated as errors
............................................................................................................................

Later, I tried to build the example from v0.10.0-rc.0 of the SDK but it results in the same error as above (after execution of 'make'). Please note that both the 'master' and 'v0.10.0-rc.0' versions of the SDK were built with the latest 'esp-idf'

Does the SDK example needs to be built with any specific snapshot of esp-idf? Please advice!

Thanks,
Sanju

Currently, the Astarte SDK made HTTPS request without validating the server's CA, but from esp-idf.4.3 this is no longer possible unless you set these option in sdkconfig:

CONFIG_ESP_TLS_INSECURE = Y
CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY= Y

The Astarte ESP32 Device makes use of the free RTOS APIs.
Some of those APIs usage is relevant to developers using the Astarte ESP32 Device in their projects.
One example is the tasks spawned inside the device and the stack size they require to run.
Some information should be included in the README to ease integration.

The sources are currently scattered around the main folder of this repository.
A cleaner configuration would be to move them into a dedicated folder called src.

See how this can be done in the PR related to the examples: #104.

When a Device certificate expires, sometimes renewal through Pairing doesn't happen, leaving the Device stuck forever with the old certificate and unable to connect.

• Add check to astarte_device_add_interface
• Add specific error to astarte.h like ASTARTE_ERR_INVALID_INTERFACE_VERSION

Right now the SDK doesn't allow to unset a property.
There are several approaches to achieve this:

1. Add callback type *astarte_device_unset_event_callback_t.
2. Change the bson_value_type received from server into BSON Type Null before call astarte_device_data_event_callback_t.
3. etc...

A cluster upgrade from 0.11 to 1.0 caused a device disconnection (due to VMQ upgrade), and the device is not reconnecting.

Following output is displayed:

E (6312451) MQTT_CLIENT: Client has not connected
E (6312451) ASTARTE_DEVICE: Publish on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds failed
E (6312551) esp-tls: mbedtls_ssl_handshake returned -0x50
E (6312561) esp-tls: Failed to open new connection
E (6312561) TRANS_SSL: Failed to open a new connection
E (6312561) MQTT_CLIENT: Error transport connect
I (6312571) ASTARTE_DEVICE: MQTT_EVENT_DISCONNECTED
I (6313371) ASTARTE_DEVICE: Publishing on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds with QoS 0
E (6313371) MQTT_CLIENT: Client has not connected
E (6313371) ASTARTE_DEVICE: Publish on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds failed
I (6314291) ASTARTE_DEVICE: Publishing on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds with QoS 0
E (6314291) MQTT_CLIENT: Client has not connected
E (6314291) ASTARTE_DEVICE: Publish on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds failed
I (6315221) ASTARTE_DEVICE: Publishing on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds with QoS 0
E (6315221) MQTT_CLIENT: Client has not connected
E (6315221) ASTARTE_DEVICE: Publish on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds failed
I (6316441) ASTARTE_DEVICE: Publishing on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds with QoS 0
E (6316441) MQTT_CLIENT: Client has not connected
E (6316441) ASTARTE_DEVICE: Publish on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds failed
I (6317371) ASTARTE_DEVICE: Publishing on test/gqaetex9L_V35MSPITlzlg/org.astarteplatform.esp32.DeviceDatastream/uptimeSeconds with QoS 0

Right now the astarte_device_start function returns void, but it can actually fail (either because the reinit semaphore can't be taken or because esp_mqtt_client_start fails).

We should return an error value to notify the caller in case of failure.

looks like unless the status is 201, this line will return a null (since resp is null), and the next two lines might have undefined behavior. Our device reboot everytime it hit this branch.

The static analysis provided by the compiler could be enhanced by running clang-tidy on our sources.
The ESP-IDF includes experimental functionality to run clang-tidy.
https://docs.espressif.com/projects/esp-idf/en/v5.0.1/esp32/api-guides/tools/idf-clang-tidy.html

In the bson parser, the case for BSON_TYPE_ARRAY is missing in this switch:

This translates in the impossibility of parsing arrays received from the server.

Right now the private key, CSR and Certificate are saved in a FAT partition. This is problematic in applications where the flash space is limited and there's no space for the astarte partition.

We should allow saving the private key, CSR and Certificate in the NVS if the users chooses to.

Right now the SDK doesn't allow to send introspection string greater than 512 bytes.

• Increase the introspection buffer.
• Improve string introspection buffer measurement.

On IDF 5, first call ensure_mounted function returns :

E (1198) ASTARTE_CREDENTIALS: Cannot create /astarte/ast_cred directory
E (1293) ASTARTE_CREDENTIALS: Cannot open /astarte/ast_cred/device.key for writing
E (1293) ASTARTE_CREDENTIALS: Cannot store private

mkdir returns FR_DENIED

In functions such as astarte_bson_serializer_write_document and astarte_bson_serializer_get_document the parameters representing a buffer size should be of type size_t instead of int.

The task astarte_device_reinit_task is terminated using a rtos notification from the main task.
This creates overhead and is more complex compared to deleting the task directly using vTaskDelete(task_handle).
The following:

        vTaskDelete(ret->reinit_task_handle);

Same for the other locations.

The function uuid_to_string accepts a pointer to the buffer as a parameter. Such buffer is used to return the stringified UUID.
However, a parameter specifying the allocated memory for such a buffer is missing.
This could lead to writing out of the array's bounds if the allocated memory is insufficient.

Right now the SDK just uses the default NVS partition, while it would be nice to allow choosing a non-standard one for custom applications.

See also #48, which should also follow the same idea.

As soon as espressif/esp-idf#2490 is merged and espmqtt SSL mutual authentication is upstreamed in esp-idf, we can add instructions to build the example and using the SDK.

We have astarte_device_start but we don't have its counterpart (i.e. astarte_device_stop) to stop the Astarte MQTT client

I have been using the astarte-device-sdk-esp32 for about a year now and I just updated to your recent sdk v0.11.4 for my project. There is a reinitialization issue where if the ESP32 loses its wifi signal and then gets it back it will reconnect to the wifi then 'reinitialize' and when it does this it gets stuck in a cycle where it will display 'MQTT_EVENT_CONNECTED' then 'MQTT_EVENT_DISCONNECTED' and will try to reinitialize again after it gets a 'Certificate error, notifying the reinit task' over and over. Once this starts then the device code will do this loop indefinitely until there is a hard reset.

I saw that in a older code version you had fixed this issue. Is there something that I am missing? If the device receives an MQTT EVENT during reinitialization will that cause it to go into an error state again and again?

Thank you,

I believe that I may have found the issue in my code. I will update you about this when I test some things out.

Thank you