Azure-iot-sdk-c: OPTION_DO_WORK_FREQUENCY_IN_MS data type mismatch

[kiranpradeep]

Development Machine, OS, Compiler (and Other Relevant Toolchain Info)

Ubuntu 18.04, Intel Core i5, clang 6.0.0

SDK Version (Please Give Commit SHA if Manually Compiling)

apt-get install azure-iot-sdk-c-dev (latest - 0.2.0.0-11bionic)

Protocol

MQTT Websockets (bug is irrespective of protocol)

Describe the Bug

1. IoTHub C SDK Options document [1] doesn't document data type of OPTION_DO_WORK_FREQUENCY_IN_MS.
2. iothub_client_core.c[2] defines the type as uint16_t. But there are occasional casts to tickcounter_ms_t [3] which appears to be buggy.
3. Assertion fails when used as in below sample source.

#include <assert.h>
#include "azureiot/iothub.h"
#include "azureiot/iothub_device_client.h"
#include "azureiot/iothub_client_options.h"
#include "azureiot/iothubtransportmqtt_websockets.h"
#include "azure_c_shared_utility/tickcounter.h"

int main(void)
{
        char connectionString[] = "HostName=xxxxxxxxxxxxxxxxx.azure-devices.net;DeviceId=xxxxxxxx;SharedAccessKey=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
        IoTHub_Init();

        IOTHUB_DEVICE_CLIENT_HANDLE iotHubClientHandle = IoTHubDeviceClient_CreateFromConnectionString(connectionString, MQTT_WebSocket_Protocol);
        assert(iotHubClientHandle != NULL) ;

        tickcounter_ms_t timeout = 10000;
        int ret = IoTHubDeviceClient_SetOption(iotHubClientHandle, OPTION_MESSAGE_TIMEOUT, &timeout);
        assert(ret == IOTHUB_CLIENT_OK);

        uint16_t sleepTime = 100; //assuming data type from declarations - is this correct ?
        ret = IoTHubDeviceClient_SetOption(iotHubClientHandle, OPTION_DO_WORK_FREQUENCY_IN_MS, &sleepTime);
        assert(ret == IOTHUB_CLIENT_OK);

        IoTHub_Deinit();
        return 0;
}

Console Logs

Error: Time:Fri May 17 23:19:22 2019 File:/build/azure-iot-sdk-c-u7bEy1/azure-iot-sdk-c-0.2.0.0/iothub_client/src/iothub_client_core.c Func:IoTHubClientCore_SetOption Line:1725 Invalid value: OPTION_DO_WORK_FREQUENCY_IN_MS cannot exceed that of OPTION_MESSAGE_TIMEOUT.
a.out: main.c:22: int main(void): Assertion `ret == IOTHUB_CLIENT_OK' failed.
Aborted (core dumped)

[massand]

Good catch @kiranpradeep! Your repro should be a unit test here (combining message timeout and dowork frequency). Marked as bug.

[massand]

Since this is a 'good first issue' class of bug, do you mind if you work on a small PR with us? Please switch the dowork frequency data type to tickcounter_ms_t type and add your test. Assigning to you for now.

[kiranpradeep]

@massand To confirm, shall I also change as below. Kindly correct as appropriate.

1. SRS_IOTHUBCLIENT_41_003 [ If the value of OPTION_DO_WORK_FREQUENCY_IN_MS is greater than 100, IotHubClientCore_SetOption shall return IOTHUB_CLIENT_INVALID_ARG ]
2. SRS_IOTHUBCLIENT_41_004 [ If currentMessageTimeout is not greater than do_work_freq_ms, IotHubClientCore_SetOption shall return IOTHUB_CLIENT_INVALID_ARG ]

[massand]

SRS_IOTHUBCLIENT_41_003 [ If the value of OPTION_DO_WORK_FREQUENCY_IN_MS is greater than 100, IotHubClientCore_SetOption shall return IOTHUB_CLIENT_INVALID_ARG ]
SRS_IOTHUBCLIENT_41_004 [ If currentMessageTimeout is not greater than do_work_freq_ms, IotHubClientCore_SetOption shall return IOTHUB_CLIENT_INVALID_ARG ]

That sounds good. There is also a case where the currentMessageTimeout is set on the iotHubClientInstance, even if the value is less than do_work_freq_ms. I think we should not set it on the handle in that case.

[YoDaMa]

Just adding here. The uint16_t vs. tickcounter_ms_t is intentional. The documentation should be updated to indicate that tickcounter_ms_t is the necessary unit from the API side for setting this variable. It can also be added to the convenience sample to indicate the proper usage of this variable.

Your analysis on the code is good though (really, a great issue file with good code snippet and everything! Thanks!). Here's a deeper explanation:

Why use tickcounter_ms_t for OPTION_DO_WORK_FREQUENCY_IN_MS?

• In iothub_client_core.c the OPTION_DO_WORK_FREQUENCY_IN_MS has a value that is checked against iotHubClientInstance->currentMessageTimeout, which is of type tickcounter_ms_t (which is defined in the azure_c_shared_utilities as uint_fast64_t or uint_fast32_t depending on the platform).
• Because of this check against currentMessageTimeout, OPTION_DO_WORK_FREQUENCY_IN_MS needs to be of a type tickcounter_ms_t *.

But then in iothub_client_core.c, do_work_freq_ms is set as a uint16_t. Why?

• do_work_freq_ms is used to specify a sleep interval via ThreadAPI_Sleep, which takes a type unsigned int.
• If the do_work_freq_ms were a tickcounter_ms_t, we end up with a bug. When it is cast to a unsigned int to input into ThreadAPI_Sleep, the number will be undefined (just random garbage).
• So to avoid this we cast the tickcounter_ms_t pointer to a uint16_t pointer, and because we've set the range to be between 0-100, there's no chance of this pointer cast losing data. The do_work_freq_ms variable for iotHubClientInstance will be set as the dereferenced uint16_t pointer.

[massand]

@kiranpradeep - @YoDaMa brings up valid points - the API is correct infact.
The data type for SetOption cannot truly be validated in our C SDK, but it should really be tickcounter_ms_t (in line with the OPTION_MESSAGE_TIMEOUT). The internal version is uint16_t to ensure we don't recast the option value for the ThreadAPI_Sleep(unsigned int) call.
So, your sample should contain this -

tickcounter_ms_t timeout = 10000;
int ret = IoTHubDeviceClient_SetOption(iotHubClientHandle, OPTION_MESSAGE_TIMEOUT, &timeout);
assert(ret == IOTHUB_CLIENT_OK);

tickcounter_ms_t sleepTime = 100; //assuming data type from declarations - is this correct ?
ret = IoTHubDeviceClient_SetOption(iotHubClientHandle, OPTION_DO_WORK_FREQUENCY_IN_MS, &sleepTime);
assert(ret == IOTHUB_CLIENT_OK);

Improvements that @YoDaMa is working on his PR are:
1) Switching our internal data type for do_work from uint16_t to unsigned int - because ThreadAPI_Sleep(unsigned int) uses that type and we want to be consistent
2) Adding unit tests to validate some additional scenarios - comparison with messageTimeout, range checks.
3) Adding convenience sample usage (he started this already in PR above)

Please help review the work when he is done, so we are sure it works for you. Thanks!

[az-iot-builder-01]

@massand, @kiranpradeep, @YoDaMa, thank you for your contribution to our open-sourced project! Please help us improve by filling out this 2-minute customer satisfaction survey