diff --git a/communication/inc/communication_dynalib.h b/communication/inc/communication_dynalib.h index f2a43cb346..97c7e1122a 100644 --- a/communication/inc/communication_dynalib.h +++ b/communication/inc/communication_dynalib.h @@ -68,7 +68,7 @@ DYNALIB_FN(BASE_IDX + 2, communication, extract_public_ec_key, int(uint8_t*, siz #define BASE_IDX2 (BASE_IDX + 1) #endif -DYNALIB_FN(BASE_IDX2 + 0, communication, spark_protocol_set_connection_property, int(ProtocolFacade*, unsigned, unsigned, particle::protocol::connection_properties_t*, void*)) +DYNALIB_FN(BASE_IDX2 + 0, communication, spark_protocol_set_connection_property, int(ProtocolFacade*, unsigned, unsigned, const void*, void*)) DYNALIB_FN(BASE_IDX2 + 1, communication, spark_protocol_command, int(ProtocolFacade* protocol, ProtocolCommands::Enum cmd, uint32_t data, void* reserved)) DYNALIB_FN(BASE_IDX2 + 2, communication, spark_protocol_time_request_pending, bool(ProtocolFacade*, void*)) DYNALIB_FN(BASE_IDX2 + 3, communication, spark_protocol_time_last_synced, system_tick_t(ProtocolFacade*, time_t*, void*)) @@ -84,6 +84,7 @@ DYNALIB_FN(BASE_IDX3 + 0, communication, spark_protocol_get_describe_data, int(P DYNALIB_FN(BASE_IDX3 + 1, communication, spark_protocol_post_description, int(ProtocolFacade*, int, void*)) DYNALIB_FN(BASE_IDX3 + 2, communication, spark_protocol_to_system_error, int(int)) DYNALIB_FN(BASE_IDX3 + 3, communication, spark_protocol_get_status, int(ProtocolFacade*, protocol_status*, void*)) +DYNALIB_FN(BASE_IDX3 + 4, communication, spark_protocol_get_connection_property, int(ProtocolFacade*, unsigned, unsigned*, void*, void*)) DYNALIB_END(communication) diff --git a/communication/inc/protocol.h b/communication/inc/protocol.h index 8d2938410f..d0c27c582a 100644 --- a/communication/inc/protocol.h +++ b/communication/inc/protocol.h @@ -317,14 +317,24 @@ class Protocol pinger.init(interval, timeout); } - void set_keepalive(system_tick_t interval, keepalive_source_t source) + bool set_keepalive(system_tick_t interval, keepalive_source_t source) { - pinger.set_interval(interval, source); + return pinger.set_interval(interval, source); } - void set_fast_ota(unsigned data) + system_tick_t get_keepalive(keepalive_source_t* source = nullptr) const { - chunkedTransfer.set_fast_ota(data); + return pinger.get_interval(source); + } + + void set_fast_ota(bool enabled) + { + chunkedTransfer.set_fast_ota(enabled); + } + + bool get_fast_ota() const + { + return chunkedTransfer.get_fast_ota(); } void set_handlers(CommunicationsHandlers& handlers) diff --git a/communication/inc/protocol_defs.h b/communication/inc/protocol_defs.h index 4bf150df09..534c05b450 100644 --- a/communication/inc/protocol_defs.h +++ b/communication/inc/protocol_defs.h @@ -50,6 +50,8 @@ enum ProtocolError /* 24 */ IO_ERROR_LIGHTSSL_HANDSHAKE_NONCE, /* 25 */ IO_ERROR_LIGHTSSL_HANDSHAKE_RECV_KEY, /* 26 */ NOT_IMPLEMENTED, + /* 27 */ INVALID_PARAM, + /* 28 */ NOT_CHANGED, /* * NOTE: when adding more ProtocolError codes, be sure to update toSystemError() in protocol_defs.cpp diff --git a/communication/inc/spark_protocol_functions.h b/communication/inc/spark_protocol_functions.h index 9a0c83f9fb..59a5d43cda 100644 --- a/communication/inc/spark_protocol_functions.h +++ b/communication/inc/spark_protocol_functions.h @@ -188,7 +188,8 @@ void spark_protocol_set_product_id(ProtocolFacade* protocol, product_id_t produc void spark_protocol_set_product_firmware_version(ProtocolFacade* protocol, product_firmware_version_t product_firmware_version, unsigned int param=0, void* reserved = NULL); void spark_protocol_get_product_details(ProtocolFacade* protocol, product_details_t* product_details, void* reserved=NULL); -int spark_protocol_set_connection_property(ProtocolFacade* protocol, unsigned property_id, unsigned data, particle::protocol::connection_properties_t* conn_prop, void* reserved); +int spark_protocol_set_connection_property(ProtocolFacade* protocol, unsigned property_id, unsigned value, const void* data, void* reserved); +int spark_protocol_get_connection_property(ProtocolFacade* protocol, unsigned property_id, unsigned* value, void* data, void* reserved); bool spark_protocol_time_request_pending(ProtocolFacade* protocol, void* reserved=NULL); system_tick_t spark_protocol_time_last_synced(ProtocolFacade* protocol, time_t* tm, void* reserved=NULL); diff --git a/communication/src/chunked_transfer.h b/communication/src/chunked_transfer.h index d16fb8eb8c..8c562085a2 100644 --- a/communication/src/chunked_transfer.h +++ b/communication/src/chunked_transfer.h @@ -144,12 +144,17 @@ class ChunkedTransfer ProtocolError idle(MessageChannel& channel); - void set_fast_ota(unsigned data) + void set_fast_ota(bool enabled) { - fast_ota_value = (data > 0) ? true : false; + fast_ota_value = enabled; fast_ota_override = true; } + bool get_fast_ota() const + { + return fast_ota_value; + } + bool is_updating() { return updating; diff --git a/communication/src/ping.h b/communication/src/ping.h index 8746e147aa..1dd7d55a05 100644 --- a/communication/src/ping.h +++ b/communication/src/ping.h @@ -24,7 +24,15 @@ class Pinger this->keepalive_source = KeepAliveSource::SYSTEM; } - void set_interval(system_tick_t interval, keepalive_source_t source) + /** + * Sets the ping interval. + * + * @param interval New interval in milliseconds. + * @param source Source of the interval change. The interval set by the user application takes + * precedence over the interval set by the system. + * @return `true` if the current interval has been changed or `false` otherwise. + */ + bool set_interval(system_tick_t interval, keepalive_source_t source) { /** * LAST CURRENT UPDATE? @@ -34,11 +42,30 @@ class Pinger * USER SYS NO * USER USER YES */ + // TODO: It feels that this logic should have been implemented in the system layer if ( !(this->keepalive_source == KeepAliveSource::USER && source == KeepAliveSource::SYSTEM) ) { - this->ping_interval = interval; this->keepalive_source = source; + if (this->ping_interval != interval) { + this->ping_interval = interval; + return true; + } + } + return false; + } + + /** + * Returns the current ping interval. + * + * @param source[out] Source of the last interval change. + * @return Interval in milliseconds. + */ + system_tick_t get_interval(keepalive_source_t* source = nullptr) const + { + if (source) { + *source = keepalive_source; } + return ping_interval; } void reset() diff --git a/communication/src/protocol_defs.cpp b/communication/src/protocol_defs.cpp index 49b02c05de..c8379e72fd 100644 --- a/communication/src/protocol_defs.cpp +++ b/communication/src/protocol_defs.cpp @@ -49,6 +49,10 @@ system_error_t particle::protocol::toSystemError(ProtocolError error) { return SYSTEM_ERROR_TOO_LARGE; case NOT_IMPLEMENTED: return SYSTEM_ERROR_NOT_SUPPORTED; + case INVALID_PARAM: + return SYSTEM_ERROR_INVALID_ARGUMENT; + case NOT_CHANGED: + return SYSTEM_ERROR_NOT_CHANGED; default: return SYSTEM_ERROR_PROTOCOL; // Generic protocol error } diff --git a/communication/src/spark_protocol_functions.cpp b/communication/src/spark_protocol_functions.cpp index 9d196e3ef0..5109527a52 100644 --- a/communication/src/spark_protocol_functions.cpp +++ b/communication/src/spark_protocol_functions.cpp @@ -190,19 +190,60 @@ void spark_protocol_get_product_details(ProtocolFacade* protocol, product_detail protocol->get_product_details(*details); } -int spark_protocol_set_connection_property(ProtocolFacade* protocol, unsigned property_id, - unsigned data, particle::protocol::connection_properties_t* conn_prop, void* reserved) +int spark_protocol_set_connection_property(ProtocolFacade* protocol, unsigned property_id, unsigned value, + const void* data, void* reserved) { ASSERT_ON_SYSTEM_THREAD(); - if (property_id == particle::protocol::Connection::PING) - { - protocol->set_keepalive(data, conn_prop->keepalive_source); - } else if (property_id == particle::protocol::Connection::FAST_OTA) - { - protocol->set_fast_ota(data); + switch (property_id) { + case particle::protocol::Connection::PING: { + particle::protocol::keepalive_source_t source = particle::protocol::KeepAliveSource::SYSTEM; + if (data) { + const auto d = static_cast(data); + source = d->keepalive_source; + } + const bool changed = protocol->set_keepalive(value, source); + if (!changed) { + return particle::protocol::NOT_CHANGED; + } + return 0; + } + case particle::protocol::Connection::FAST_OTA: { + protocol->set_fast_ota(value); + return 0; + } + default: + return particle::protocol::INVALID_PARAM; + } +} + +int spark_protocol_get_connection_property(ProtocolFacade* protocol, unsigned property_id, unsigned* value, void* data, + void* reserved) +{ + ASSERT_ON_SYSTEM_THREAD(); + switch (property_id) { + case particle::protocol::Connection::PING: { + particle::protocol::keepalive_source_t source = particle::protocol::KeepAliveSource::SYSTEM; + const system_tick_t interval = protocol->get_keepalive(&source); + if (value) { + *value = interval; + } + if (data) { + const auto d = static_cast(data); + d->keepalive_source = source; + } + return 0; + } + case particle::protocol::Connection::FAST_OTA: { + if (value) { + *value = protocol->get_fast_ota(); + } + return 0; + } + default: + return particle::protocol::INVALID_PARAM; } - return 0; } + int spark_protocol_command(ProtocolFacade* protocol, ProtocolCommands::Enum cmd, uint32_t data, void* reserved) { ASSERT_ON_SYSTEM_THREAD(); @@ -214,6 +255,7 @@ bool spark_protocol_time_request_pending(ProtocolFacade* protocol, void* reserve (void)reserved; return protocol->time_request_pending(); } + system_tick_t spark_protocol_time_last_synced(ProtocolFacade* protocol, time_t* tm, void* reserved) { (void)reserved; @@ -348,10 +390,16 @@ void spark_protocol_get_product_details(CoreProtocol* protocol, product_details_ protocol->get_product_details(*details); } -int spark_protocol_set_connection_property(CoreProtocol* protocol, unsigned property_id, - unsigned data, particle::protocol::connection_properties_t* conn_prop, void* reserved) +int spark_protocol_set_connection_property(CoreProtocol* protocol, unsigned property_id, unsigned value, const void* data, + void* reserved) +{ + return particle::protocol::NOT_IMPLEMENTED; +} + +int spark_protocol_get_connection_property(CoreProtocol* protocol, unsigned property_id, unsigned* value, void* data, + void* reserved) { - return 0; + return particle::protocol::NOT_IMPLEMENTED; } int spark_protocol_command(CoreProtocol* protocol, ProtocolCommands::Enum cmd, uint32_t data, void* reserved) diff --git a/services/inc/system_error.h b/services/inc/system_error.h index 2d6590ab06..6fd7de4495 100644 --- a/services/inc/system_error.h +++ b/services/inc/system_error.h @@ -27,6 +27,7 @@ (BUSY, "Resource busy", -110), \ (NOT_SUPPORTED, "Not supported", -120), \ (NOT_ALLOWED, "Not allowed", -130), \ + (NOT_CHANGED, "Not changed", -131), \ (CANCELLED, "Operation cancelled", -140), \ (ABORTED, "Operation aborted", -150), \ (TIMEOUT, "Timeout error", -160), \ diff --git a/system/inc/system_cloud.h b/system/inc/system_cloud.h index df676147c3..a30b63037e 100644 --- a/system/inc/system_cloud.h +++ b/system/inc/system_cloud.h @@ -242,7 +242,7 @@ bool spark_cloud_flag_auto_connect(void); ProtocolFacade* system_cloud_protocol_instance(void); -int spark_set_connection_property(unsigned property_id, unsigned data, particle::protocol::connection_properties_t* conn_prop, void* reserved); +int spark_set_connection_property(unsigned property_id, unsigned value, const void* data, void* reserved); int spark_set_random_seed_from_cloud_handler(void (*handler)(unsigned int), void* reserved); diff --git a/system/inc/system_dynalib_cloud.h b/system/inc/system_dynalib_cloud.h index fb130334f5..459c4e679e 100644 --- a/system/inc/system_dynalib_cloud.h +++ b/system/inc/system_dynalib_cloud.h @@ -48,7 +48,7 @@ DYNALIB_FN(10, system_cloud, spark_unsubscribe, void(void*)) DYNALIB_FN(11, system_cloud, spark_sync_time, bool(void*)) DYNALIB_FN(12, system_cloud, spark_sync_time_pending, bool(void*)) DYNALIB_FN(13, system_cloud, spark_sync_time_last, system_tick_t(time_t*, void*)) -DYNALIB_FN(14, system_cloud, spark_set_connection_property, int(unsigned, unsigned, particle::protocol::connection_properties_t*, void*)) +DYNALIB_FN(14, system_cloud, spark_set_connection_property, int(unsigned, unsigned, const void*, void*)) DYNALIB_FN(15, system_cloud, spark_set_random_seed_from_cloud_handler, int(void (*handler)(unsigned int), void*)) DYNALIB_FN(16, system_cloud, spark_publish_vitals, int(system_tick_t, void*)) diff --git a/system/src/system_cloud.cpp b/system/src/system_cloud.cpp index 37a1811781..ce0248117c 100644 --- a/system/src/system_cloud.cpp +++ b/system/src/system_cloud.cpp @@ -53,6 +53,7 @@ extern void (*random_seed_from_cloud_handler)(unsigned int); namespace { +using namespace particle; using namespace particle::system; #if PLATFORM_THREADING @@ -245,10 +246,21 @@ String spark_deviceID(void) return bytes2hex(id, len); } -int spark_set_connection_property(unsigned property_id, unsigned data, particle::protocol::connection_properties_t* conn_prop, void* reserved) +int spark_set_connection_property(unsigned property_id, unsigned value, const void* data, void* reserved) { - SYSTEM_THREAD_CONTEXT_SYNC(spark_set_connection_property(property_id, data, conn_prop, reserved)); - return spark_protocol_set_connection_property(sp, property_id, data, conn_prop, reserved); + SYSTEM_THREAD_CONTEXT_SYNC(spark_set_connection_property(property_id, value, data, reserved)); + const auto r = spark_protocol_set_connection_property(sp, property_id, value, data, reserved); + if (r != 0) { + return spark_protocol_to_system_error(r); + } + // Publish the new keepalive interval if the connection to the cloud is established or the + // system handshake is in progress + if (property_id == protocol::Connection::PING && (spark_cloud_flag_connected() || SPARK_CLOUD_HANDSHAKE_PENDING)) { + if (!publishKeepaliveInterval(value)) { + LOG(WARN, "Unable to publish the keepalive interval"); + } + } + return 0; } int spark_set_random_seed_from_cloud_handler(void (*handler)(unsigned int), void* reserved) diff --git a/system/src/system_cloud_internal.cpp b/system/src/system_cloud_internal.cpp index 6ce6bb6be9..7aade6b563 100644 --- a/system/src/system_cloud_internal.cpp +++ b/system/src/system_cloud_internal.cpp @@ -54,6 +54,7 @@ using particle::CloudDiagnostics; using particle::publishEvent; +using particle::publishKeepaliveInterval; #ifndef SPARK_NO_CLOUD @@ -358,6 +359,7 @@ constexpr const char KEY_RESTORE_EVENT[] = "spark/device/key/restore"; constexpr const char DEVICE_UPDATES_EVENT[] = "particle/device/updates/"; constexpr const char FORCED_EVENT[] = "forced"; constexpr const char UPDATES_PENDING_EVENT[] = "pending"; +constexpr const char KEEPALIVE_INTERVAL_EVENT[] = "particle/device/keepalive"; inline bool is_suffix(const char* eventName, const char* prefix, const char* suffix) { // todo - sanity check parameters? @@ -1001,6 +1003,7 @@ int Spark_Handshake(bool presence_announce) } Send_Firmware_Update_Flags(); + publishKeepaliveInterval(); if (presence_announce) { Multicast_Presence_Announcement(); @@ -1018,6 +1021,7 @@ int Spark_Handshake(bool presence_announce) publishSafeModeEventIfNeeded(); Send_Firmware_Update_Flags(); + publishKeepaliveInterval(); if (!HAL_RTC_Time_Is_Valid(nullptr) && spark_sync_time_last(nullptr, nullptr) == 0) { spark_protocol_send_time_request(sp); @@ -1114,3 +1118,28 @@ void Spark_Wake(void) CloudDiagnostics* CloudDiagnostics::instance() { return &g_cloudDiagnostics; } + +namespace particle { + +#ifndef SPARK_NO_CLOUD + +bool publishKeepaliveInterval(unsigned interval) { + if (!interval) { + // Get the current interval + const auto r = spark_protocol_get_connection_property(spark_protocol_instance(), protocol::Connection::PING, + &interval, nullptr, nullptr); + if (r != 0) { + return false; + } + } + // TODO: Even though the keepalive interval is not supposed to be changed frequently, it would be + // nice to make sure the previously published event is either sent or cancelled before publishing + // a new event. This would help to mitigate the effect of possible out of order delivery + char buf[16] = {}; + snprintf(buf, sizeof(buf), "%u", interval); + return publishEvent(KEEPALIVE_INTERVAL_EVENT, buf); +} + +#endif // !defined(SPARK_NO_CLOUD) + +} // particle diff --git a/system/src/system_cloud_internal.h b/system/src/system_cloud_internal.h index 915977482c..857aacb64c 100644 --- a/system/src/system_cloud_internal.h +++ b/system/src/system_cloud_internal.h @@ -136,6 +136,8 @@ inline bool publishEvent(const char* event, const char* data = nullptr, unsigned return spark_send_event(event, data, DEFAULT_CLOUD_EVENT_TTL, flags | PUBLISH_EVENT_FLAG_PRIVATE, nullptr); } +bool publishKeepaliveInterval(unsigned interval = 0); + } // namespace particle #ifdef __cplusplus diff --git a/test/unit_tests/communication/ping.cpp b/test/unit_tests/communication/ping.cpp index cdff11638a..6c327deeca 100644 --- a/test/unit_tests/communication/ping.cpp +++ b/test/unit_tests/communication/ping.cpp @@ -88,11 +88,18 @@ SCENARIO("ping requests and responses are managed") THEN("The ping interval can be updated by the SYSTEM and then the USER") { - pinger.set_interval(30000, KeepAliveSource::SYSTEM); + keepalive_source_t source; + REQUIRE(pinger.set_interval(30000, KeepAliveSource::SYSTEM)); + REQUIRE(pinger.get_interval(&source) == 30000); + REQUIRE(source == KeepAliveSource::SYSTEM); + REQUIRE(pinger.process(15001, []{return IO_ERROR;})==NO_ERROR); REQUIRE(!pinger.is_expecting_ping_ack()); - pinger.set_interval(60000, KeepAliveSource::USER); + REQUIRE(pinger.set_interval(60000, KeepAliveSource::USER)); + REQUIRE(pinger.get_interval(&source) == 60000); + REQUIRE(source == KeepAliveSource::USER); + REQUIRE(pinger.process(30001, []{return IO_ERROR;})==NO_ERROR); REQUIRE(!pinger.is_expecting_ping_ack()); @@ -104,11 +111,17 @@ SCENARIO("ping requests and responses are managed") THEN("The ping interval can be updated by the USER but then not the SYSTEM") { - pinger.set_interval(30000, KeepAliveSource::USER); + keepalive_source_t source; + REQUIRE(pinger.set_interval(30000, KeepAliveSource::USER)); + REQUIRE(pinger.get_interval(&source) == 30000); + REQUIRE(source == KeepAliveSource::USER); + REQUIRE(pinger.process(15001, []{return IO_ERROR;})==NO_ERROR); REQUIRE(!pinger.is_expecting_ping_ack()); - pinger.set_interval(60000, KeepAliveSource::SYSTEM); + REQUIRE(pinger.set_interval(60000, KeepAliveSource::SYSTEM) == false); + REQUIRE(pinger.get_interval(&source) == 30000); + REQUIRE(source == KeepAliveSource::USER); bool callback_called = false; REQUIRE(pinger.process(30001, [&]()->ProtocolError{callback_called = true; return NO_ERROR;})==NO_ERROR);