golioth / golioth-firmware-sdk

Firmware SDK enabling any IoT device to connect to Golioth - the Universal Connector for IoT
https://golioth.io
Apache License 2.0
53 stars 11 forks source link

[CP SDK] Observing the same params results in duplicate stored observations #473

Open szczys opened 2 months ago

szczys commented 2 months ago

Tested in Zephyr (but appears the same in libcoap), registering the same observation multiple times will result in duplicate observations occupying slots in the golioth client.

Reproduce

  1. Use the lightdb/observer sample
  2. Call the register API with the same values in the loop
  3. Print out the client observe slots

Test main.c

#include "../../../../src/coap_client.h"
#include "../../../../src/coap_client_zephyr.h"

int main(void)
{
    LOG_DBG("Start LightDB observe sample");

    IF_ENABLED(CONFIG_GOLIOTH_SAMPLE_COMMON, (net_connect();))

    /* Note: In production, you would provision unique credentials onto each
     * device. For simplicity, we provide a utility to hardcode credentials as
     * kconfig options in the samples.
     */
    const struct golioth_client_config *client_config = golioth_sample_credentials_get();

    client = golioth_client_create(client_config);
    golioth_client_register_event_callback(client, on_client_event, NULL);

    k_sem_take(&connected, K_FOREVER);

    while (true)
    {

        /* Observe LightDB State Path */
        golioth_lightdb_observe_async(client, "counter", counter_observe_handler, NULL);
        golioth_lightdb_observe_async(client, "baloney", counter_observe_handler, NULL);

        LOG_ERR("client: %p", client);
        golioth_coap_observe_info_t *obs_info = NULL;

        for (int i = 0; i < CONFIG_GOLIOTH_MAX_NUM_OBSERVATIONS; i++)
        {
            obs_info = &client->observations[i];
            LOG_ERR("slot: %d client: %p addr: %p in_use: %d",
                    i,
                    obs_info->req.client,
                    &obs_info->req.client,
                    obs_info->in_use);
            if (obs_info->in_use)
            {
                LOG_HEXDUMP_ERR(obs_info, sizeof(golioth_coap_request_msg_t), "obs_info");
            }
        }

        k_sleep(K_SECONDS(5));
    }

    return 0;
}
szczys commented 2 months ago

I discussed with @sam-golioth and we don't necessarily need to guard against this behavior, but we should do a couple of things to address it:

  1. Mention the behavior in the doxygen comments for registering observations. Note that observations are automatically reestablished by the client so each should only be called once.
  2. Once https://github.com/golioth/golioth-firmware-sdk/pull/452 merges, add doxygen to the observation registration APIs that indicates the limit on number of observations, indicating that it may be adjusted with CONFIG_GOLIOTH_MAX_NUM_OBSERVATIONS
  3. Add a function that user can call to check if a path/callback combination is already registered. Make the callback param optional so NULL will only search for the given path.