From 84c373634b4d5a1067347afb2aded608d63cc130 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Wed, 3 Apr 2024 14:58:08 +0200 Subject: [PATCH] fixup! cloud: set/get function for timeouts of the default retry action --- api/cloud/oc_cloud.c | 33 ++++++---- api/cloud/oc_cloud_context.c | 3 + api/cloud/oc_cloud_context_internal.h | 4 +- api/cloud/oc_cloud_internal.h | 6 ++ api/cloud/oc_cloud_schedule.c | 10 +-- api/cloud/oc_cloud_schedule_internal.h | 2 + api/cloud/unittest/cloud_manager_test.cpp | 6 +- api/cloud/unittest/cloud_test.cpp | 78 ++++++++++++++++++----- 8 files changed, 104 insertions(+), 38 deletions(-) diff --git a/api/cloud/oc_cloud.c b/api/cloud/oc_cloud.c index e349f19ed..4be5996e0 100644 --- a/api/cloud/oc_cloud.c +++ b/api/cloud/oc_cloud.c @@ -494,21 +494,31 @@ oc_cloud_manager_stop(oc_cloud_context_t *ctx) return 0; } +void +cloud_deinit_devices(size_t devices) +{ + for (size_t device = 0; device < devices; ++device) { + cloud_context_deinit(oc_cloud_get_context(device)); + } +} + static bool -cloud_init_for_devices(size_t devices) +cloud_init_device(size_t device) +{ + return cloud_context_init(device) != NULL && + oc_cloud_add_resource(oc_core_get_resource_by_index(OCF_P, 0)) == 0 && + oc_cloud_add_resource(oc_core_get_resource_by_index(OCF_D, device)) == + 0; +} + +bool +cloud_init_devices(size_t devices) { for (size_t device = 0; device < devices; ++device) { - if (cloud_context_init(device) == NULL) { - for (size_t i = 0; i < device; ++i) { - oc_cloud_context_t *ctx = oc_cloud_get_context(i); - assert(ctx != NULL); // cloud_context_init for these devices succeeded, - // so the context must exist - cloud_context_deinit(ctx); - } + if (!cloud_init_device(device)) { + cloud_deinit_devices(device); return false; } - oc_cloud_add_resource(oc_core_get_resource_by_index(OCF_P, 0)); - oc_cloud_add_resource(oc_core_get_resource_by_index(OCF_D, device)); } return true; } @@ -519,11 +529,10 @@ oc_cloud_init(void) if (!oc_ri_on_delete_resource_add_callback(oc_cloud_delete_resource)) { return false; } - if (!cloud_init_for_devices(oc_core_get_num_devices())) { + if (!cloud_init_devices(oc_core_get_num_devices())) { oc_ri_on_delete_resource_remove_callback(oc_cloud_delete_resource); return false; } - return true; } diff --git a/api/cloud/oc_cloud_context.c b/api/cloud/oc_cloud_context.c index 5b1ff3ff5..83d0a3fa1 100644 --- a/api/cloud/oc_cloud_context.c +++ b/api/cloud/oc_cloud_context.c @@ -116,6 +116,9 @@ cloud_context_init(size_t device) void cloud_context_deinit(oc_cloud_context_t *ctx) { + if (ctx == NULL) { + return; + } oc_cloud_registration_context_deinit(&ctx->registration_ctx); cloud_rd_deinit(ctx); // In the case of a factory reset, the cloud data may remain on the device diff --git a/api/cloud/oc_cloud_context_internal.h b/api/cloud/oc_cloud_context_internal.h index e6d018039..3a219058c 100644 --- a/api/cloud/oc_cloud_context_internal.h +++ b/api/cloud/oc_cloud_context_internal.h @@ -116,9 +116,9 @@ oc_cloud_context_t *cloud_context_init(size_t device); /** * @brief Deinitialize and deallocate cloud context * - * @param ctx context to deinitialize (cannot be NULL) + * @param ctx context to deinitialize */ -void cloud_context_deinit(oc_cloud_context_t *ctx) OC_NONNULL(); +void cloud_context_deinit(oc_cloud_context_t *ctx); /// @brief Count number of allocated contexts size_t cloud_context_size(void); diff --git a/api/cloud/oc_cloud_internal.h b/api/cloud/oc_cloud_internal.h index 30af48527..26ce52bf0 100644 --- a/api/cloud/oc_cloud_internal.h +++ b/api/cloud/oc_cloud_internal.h @@ -59,6 +59,12 @@ typedef struct const oc_rep_t *ci_servers; /**< List of Cloud Interface Servers. */ } oc_cloud_conf_update_t; +/** Initialize cloud data for devices */ +bool cloud_init_devices(size_t devices); + +/** Deinitialize cloud data for devices */ +void cloud_deinit_devices(size_t devices); + /** Set cloud endpoint from currently selected cloud server address */ bool oc_cloud_set_endpoint(oc_cloud_context_t *ctx) OC_NONNULL(); diff --git a/api/cloud/oc_cloud_schedule.c b/api/cloud/oc_cloud_schedule.c index 6844ce902..6e027cb85 100644 --- a/api/cloud/oc_cloud_schedule.c +++ b/api/cloud/oc_cloud_schedule.c @@ -39,7 +39,8 @@ 32 * MILLISECONDS_PER_SECOND, 64 * MILLISECONDS_PER_SECOND \ } -static uint16_t g_retry_timeout_ms[] = OC_CLOUD_DEFAULT_RETRY_TIMEOUTS; +static uint16_t g_retry_timeout_ms[OC_CLOUD_RETRY_TIMEOUTS_SIZE] = + OC_CLOUD_DEFAULT_RETRY_TIMEOUTS; bool oc_cloud_set_retry_timeouts(const uint16_t *timeouts, uint8_t size) @@ -50,7 +51,7 @@ oc_cloud_set_retry_timeouts(const uint16_t *timeouts, uint8_t size) return true; } - if (size == 0 || size > OC_ARRAY_SIZE(g_retry_timeout_ms)) { + if (size > OC_ARRAY_SIZE(g_retry_timeout_ms)) { return false; } for (int i = 0; i < size; ++i) { @@ -68,9 +69,8 @@ int oc_cloud_get_retry_timeouts(uint16_t *timeouts, uint8_t size) { uint8_t count = 0; - for (uint8_t i = 0; i < (uint8_t)OC_ARRAY_SIZE(g_retry_timeout_ms); ++i) { - if (g_retry_timeout_ms[i] == 0) { - count = i; + for (; count < (uint8_t)OC_ARRAY_SIZE(g_retry_timeout_ms); ++count) { + if (g_retry_timeout_ms[count] == 0) { break; } } diff --git a/api/cloud/oc_cloud_schedule_internal.h b/api/cloud/oc_cloud_schedule_internal.h index 97fba761a..b3d3d91aa 100644 --- a/api/cloud/oc_cloud_schedule_internal.h +++ b/api/cloud/oc_cloud_schedule_internal.h @@ -37,6 +37,8 @@ extern "C" { bool cloud_retry_is_over(uint8_t retry_count); /** Set timeout intervals for the default retry action */ +#define OC_CLOUD_RETRY_TIMEOUTS_SIZE (6) + bool oc_cloud_set_retry_timeouts(const uint16_t *timeouts, uint8_t size); int oc_cloud_get_retry_timeouts(uint16_t *timeouts, uint8_t size) OC_NONNULL(); diff --git a/api/cloud/unittest/cloud_manager_test.cpp b/api/cloud/unittest/cloud_manager_test.cpp index 1dc188bd6..f2c3f763b 100644 --- a/api/cloud/unittest/cloud_manager_test.cpp +++ b/api/cloud/unittest/cloud_manager_test.cpp @@ -63,7 +63,7 @@ class TestCloudManager : public testing::Test { // make the second timeout longer, so we can interrupt the retry std::chrono::duration_cast(5s).count(), }; - oc_cloud_set_retry_timeouts(timeouts.data(), timeouts.size()); + ASSERT_TRUE(oc_cloud_set_retry_timeouts(timeouts.data(), timeouts.size())); memset(&m_context, 0, sizeof(m_context)); m_context.cloud_ep = oc_new_endpoint(); @@ -90,7 +90,7 @@ class TestCloudManager : public testing::Test { void TearDown() override { oc_cloud_set_schedule_action(&m_context, nullptr, nullptr); - oc_cloud_set_retry_timeouts(nullptr, 0); + ASSERT_TRUE(oc_cloud_set_retry_timeouts(nullptr, 0)); oc_free_endpoint(m_context.cloud_ep); oc_cloud_store_deinitialize(&m_context.store); @@ -342,7 +342,7 @@ TEST_F(TestCloudManager, cloud_manager_select_next_server_on_retry) // single try -> the cloud server endpoint should be changed after each try uint16_t timeout = std::chrono::duration_cast(kTimeout).count(); - oc_cloud_set_retry_timeouts(&timeout, 1); + ASSERT_TRUE(oc_cloud_set_retry_timeouts(&timeout, 1)); oc_string_view_t uri = OC_STRING_VIEW("coap://13.3.7.187:5683"); oc_uuid_t sid; diff --git a/api/cloud/unittest/cloud_test.cpp b/api/cloud/unittest/cloud_test.cpp index dee4e1d79..397abb802 100644 --- a/api/cloud/unittest/cloud_test.cpp +++ b/api/cloud/unittest/cloud_test.cpp @@ -22,6 +22,8 @@ #include "api/cloud/oc_cloud_endpoints_internal.h" #include "api/cloud/oc_cloud_internal.h" #include "api/cloud/oc_cloud_resource_internal.h" +#include "api/cloud/oc_cloud_schedule_internal.h" +#include "api/oc_runtime_internal.h" #include "messaging/coap/conf.h" #include "oc_api.h" #include "oc_cloud.h" @@ -30,19 +32,26 @@ #include "tests/gtest/RepPool.h" #include "util/oc_secure_string_internal.h" +#include #include #include #include +#include #ifdef OC_SECURITY #include "security/oc_pstat_internal.h" -#endif +#endif /* OC_SECURITY */ using namespace std::chrono_literals; static constexpr size_t kDeviceID{ 0 }; -class TestCloud : public testing::Test {}; +class TestCloud : public testing::Test { +public: + void SetUp() override { oc_runtime_init(); } + + void TearDown() override { oc_runtime_shutdown(); } +}; TEST_F(TestCloud, cloud_is_connection_error_code) { @@ -59,6 +68,55 @@ TEST_F(TestCloud, cloud_is_connection_error_code) } } +TEST_F(TestCloud, oc_cloud_set_retry_timeouts) +{ + std::vector tooManyTimeouts(OC_CLOUD_RETRY_TIMEOUTS_SIZE + 1, 0); + EXPECT_FALSE(oc_cloud_set_retry_timeouts(tooManyTimeouts.data(), + tooManyTimeouts.size())); + + std::vector invalidTimeouts{ 1, 42, /*invalid*/ 0, 13 }; + EXPECT_FALSE(oc_cloud_set_retry_timeouts(invalidTimeouts.data(), + invalidTimeouts.size())); + + // restore default values by using NULL + EXPECT_TRUE(oc_cloud_set_retry_timeouts(nullptr, 0)); +} + +TEST_F(TestCloud, oc_cloud_get_retry_timeouts) +{ + std::array timeouts{ 1, 42, 13 }; + ASSERT_TRUE(oc_cloud_set_retry_timeouts(timeouts.data(), timeouts.size())); + + std::array tooSmall{}; + EXPECT_EQ(-1, oc_cloud_get_retry_timeouts(tooSmall.data(), tooSmall.size())); + + std::vector result{}; + result.resize(timeouts.size()); + EXPECT_EQ(timeouts.size(), + oc_cloud_get_retry_timeouts(result.data(), result.size())); + for (size_t i = 0; i < timeouts.size(); ++i) { + EXPECT_EQ(timeouts[i], result[i]); + } + + EXPECT_TRUE(oc_cloud_set_retry_timeouts(nullptr, 0)); + result.resize(OC_CLOUD_RETRY_TIMEOUTS_SIZE); + EXPECT_EQ(OC_CLOUD_RETRY_TIMEOUTS_SIZE, + oc_cloud_get_retry_timeouts(result.data(), result.size())); +} + +#ifndef OC_DYNAMIC_ALLOCATION + +TEST_F(TestCloud, cloud_init_devices) +{ + EXPECT_FALSE(cloud_init_devices(1)); + + ASSERT_TRUE(oc::TestDevice::StartServer()); + EXPECT_FALSE(cloud_init_devices(2)); + oc::TestDevice::StopServer(); +} + +#endif /* !OC_DYNAMIC_ALLOCATION */ + class TestCloudWithServer : public testing::Test { public: static void SetUpTestCase() { ASSERT_TRUE(oc::TestDevice::StartServer()); } @@ -805,8 +863,6 @@ TEST_F(TestCloudWithServer, oc_cloud_deregister_with_login_fail_invalid_server) EXPECT_FALSE(cbk_called); } -#endif /* OC_DYNAMIC_ALLOCATION */ - // TODO: async deregister steps with mocked cloud TEST_F(TestCloudWithServer, EndpointAPI) @@ -839,14 +895,12 @@ TEST_F(TestCloudWithServer, EndpointAPI) auto *ep2 = oc_cloud_add_server_address(ctx, uri2.c_str(), uri2.length(), uid2); ASSERT_NE(nullptr, ep2); -#ifdef OC_DYNAMIC_ALLOCATION std::string uri3 = "/uri/3"; oc_uuid_t uid3; oc_gen_uuid(&uid3); auto *ep3 = oc_cloud_add_server_address(ctx, uri3.c_str(), uri3.length(), uid3); ASSERT_NE(nullptr, ep3); -#endif /* OC_DYNAMIC_ALLOCATION */ // first item added to empty list should be selected EXPECT_EQ(ep1, oc_cloud_selected_server_address(ctx)); @@ -873,12 +927,8 @@ TEST_F(TestCloudWithServer, EndpointAPI) }, &uris); -#ifdef OC_DYNAMIC_ALLOCATION ASSERT_EQ(2, uris.size()); EXPECT_NE(uris.end(), uris.find(uri3)); -#else /* !OC_DYNAMIC_ALLOCATION */ - ASSERT_EQ(1, uris.size()); -#endif /* OC_DYNAMIC_ALLOCATION */ EXPECT_NE(uris.end(), uris.find(uri2)); EXPECT_EQ(uris.end(), uris.find(uri1)); @@ -897,15 +947,9 @@ TEST_F(TestCloudWithServer, EndpointAPI) ASSERT_NE(nullptr, toSelect); EXPECT_TRUE(oc_cloud_select_server_address(ctx, toSelect)); -#ifdef OC_DYNAMIC_ALLOCATION EXPECT_EQ(ep3, oc_cloud_selected_server_address(ctx)); EXPECT_STREQ(uri3.c_str(), oc_string(*oc_cloud_get_server_uri(ctx))); EXPECT_TRUE(oc_uuid_is_equal(uid3, *oc_cloud_get_server_id(ctx))); -#else /* !OC_DYNAMIC_ALLOCATION */ - EXPECT_EQ(ep2, oc_cloud_selected_server_address(ctx)); - EXPECT_STREQ(uri2.c_str(), oc_string(*oc_cloud_get_server_uri(ctx))); - EXPECT_TRUE(oc_uuid_is_equal(uid2, *oc_cloud_get_server_id(ctx))); -#endif /* OC_DYNAMIC_ALLOCATION */ oc_uuid_t uid; oc_gen_uuid(&uid); @@ -917,3 +961,5 @@ TEST_F(TestCloudWithServer, EndpointAPI) cloud_context_deinit(ctx); } + +#endif /* OC_DYNAMIC_ALLOCATION */