Skip to content

Commit

Permalink
addresses final PR comments
Browse files Browse the repository at this point in the history 
- Adds SaraNcpFwUpdateLock to make user interfaces thread-safe
- Fixes issue with sUrcHandlers not retaining unique pointers
- Allows cellular disconnect timeouts to proceed
- Adds cellular_lock() before accessing variables used in urcHandlers
- Fixes errors with comm. unit_tests caused by some initial code added for ncp_fw_update testing
- Moves all NCPFW_LOG statements during SAFE_MODE to NCPFW_LOG_DEBUG to reduce size
  • Loading branch information
@technobly
technobly committed Dec 15, 2021
1 parent 06134f9 commit 02d203d
Show file tree
Hide file tree
Showing 9 changed files with 162 additions and 104 deletions.
21 changes: 12 additions & 9 deletions hal/network/ncp/cellular/cellular_hal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,17 @@ hal_net_access_tech_t fromCellularAccessTechnology(CellularAccessTechnology rat)
}

struct CellularHalUrcHandler {
CellularHalUrcHandler(const char* prefix, hal_cellular_urc_callback_t callback, void* context) :
prefix(prefix),
callback(callback),
context(context) {
}
const char* prefix;
hal_cellular_urc_callback_t callback;
void* context;
};

Vector<CellularHalUrcHandler> sUrcHandlers;
Vector<std::unique_ptr<CellularHalUrcHandler>> sUrcHandlers;

static int commonUrcHandler(AtResponseReader* reader, const char* prefix, void* data) {
auto handler = static_cast<CellularHalUrcHandler*>(data);
Expand Down Expand Up @@ -544,14 +549,12 @@ int cellular_add_urc_handler(const char* prefix, hal_cellular_urc_callback_t cb,

const NcpClientLock lock(client);

sUrcHandlers.append({
.prefix = prefix,
.callback = cb,
.context = context
});
auto& handler = sUrcHandlers.last();
auto entry = std::make_unique<CellularHalUrcHandler>(prefix, cb, context);
CHECK_TRUE(entry, SYSTEM_ERROR_NO_MEMORY);
sUrcHandlers.append(std::move(entry));
auto handler = sUrcHandlers.last().get();

return parser->addUrcHandler(prefix, commonUrcHandler, &handler);
return parser->addUrcHandler(prefix, commonUrcHandler, handler);
}

int cellular_remove_urc_handler(const char* prefix) {
Expand All @@ -565,7 +568,7 @@ int cellular_remove_urc_handler(const char* prefix) {

parser->removeUrcHandler(prefix);
for (int i = 0; i < sUrcHandlers.size(); ++i) {
if (strcmp(sUrcHandlers.at(i).prefix, prefix) == 0) {
if (strcmp(sUrcHandlers.at(i).get()->prefix, prefix) == 0) {
sUrcHandlers.removeAt(i);
break;
}
Expand Down
60 changes: 56 additions & 4 deletions services/inc/ncp_fw_update.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,38 @@

#pragma once

#if MODULE_FUNCTION != 2 // BOOTLOADER

#include "hal_platform.h"

#if HAL_PLATFORM_NCP_FW_UPDATE
// STATIC_ASSERT already defined from static_assert.h as PARTICLE_STATIC_ASSERT
#ifdef STATIC_ASSERT
#undef STATIC_ASSERT
#endif
// app_util.h unconditionally defines STATIC_ASSERT
#include "static_recursive_mutex.h"
// Allow STATIC_ASSERT to be defined as PARTICLE_STATIC_ASSERT in cellular_hal.h below
#ifdef STATIC_ASSERT
#undef STATIC_ASSERT
#endif
#endif // HAL_PLATFORM_NCP_FW_UPDATE

#include "system_tick_hal.h"
#include "system_defs.h"
#include "system_mode.h"
// #include "system_network.h"
#include "cellular_hal.h"
#include "platform_ncp.h"
#include "diagnostics.h"
#include "spark_wiring_diagnostics.h"
#include "ncp_fw_update_dynalib.h"
#include "static_assert.h"

#if HAL_PLATFORM_NCP
#include "at_parser.h"
#include "at_response.h"
#endif // HAL_PLATFORM_NCP


#if HAL_PLATFORM_NCP_FW_UPDATE

// Change to 0 for debugging faster
Expand Down Expand Up @@ -89,7 +104,7 @@ enum SaraNcpFwUpdateState {
FW_UPDATE_STATE_FINISHED_CLOUD_CONNECTED = 16,
FW_UPDATE_STATE_FINISHED_IDLE = 17,
};
static_assert(FW_UPDATE_STATE_IDLE == 0 && FW_UPDATE_STATE_FINISHED_IDLE == 17, "SaraNcpFwUpdateState size changed!");
PARTICLE_STATIC_ASSERT(SaraNcpFwUpdateState_size, FW_UPDATE_STATE_IDLE == 0 && FW_UPDATE_STATE_FINISHED_IDLE == 17);

enum SaraNcpFwUpdateStatus {
FW_UPDATE_STATUS_IDLE = 0,
Expand All @@ -99,7 +114,7 @@ enum SaraNcpFwUpdateStatus {
FW_UPDATE_STATUS_FAILED = 4,
FW_UPDATE_STATUS_NONE = 5, // for diagnostics
};
static_assert(FW_UPDATE_STATUS_IDLE == 0 && FW_UPDATE_STATUS_NONE == 5, "SaraNcpFwUpdateStatus size changed!");
PARTICLE_STATIC_ASSERT(SaraNcpFwUpdateStatus_size, FW_UPDATE_STATUS_IDLE == 0 && FW_UPDATE_STATUS_NONE == 5);

const system_tick_t NCP_FW_MODEM_INSTALL_ATOK_INTERVAL = 10000;
const system_tick_t NCP_FW_MODEM_INSTALL_START_TIMEOUT = 5 * 60000;
Expand Down Expand Up @@ -170,6 +185,8 @@ class SaraNcpFwUpdate {
int checkUpdate(uint32_t version = 0);
int enableUpdates();
int updateStatus();
int lock();
int unlock();

protected:
SaraNcpFwUpdate();
Expand Down Expand Up @@ -201,6 +218,8 @@ class SaraNcpFwUpdate {
SaraNcpFwUpdateCallbacks saraNcpFwUpdateCallbacks_ = {};
HTTPSresponse httpsResp_ = {};

StaticRecursiveMutex mutex_;

static int cbUHTTPER(int type, const char* buf, int len, HTTPSerror* data);
static int cbULSTFILE(int type, const char* buf, int len, int* data);
static int httpRespCallback(AtResponseReader* reader, const char* prefix, void* data);
Expand All @@ -219,6 +238,36 @@ class SaraNcpFwUpdate {
void logSaraNcpFwUpdateData(SaraNcpFwUpdateData& data);
};

class SaraNcpFwUpdateLock {
public:
SaraNcpFwUpdateLock()
: locked_(false) {
lock();
}
~SaraNcpFwUpdateLock() {
if (locked_) {
unlock();
}
}
SaraNcpFwUpdateLock(SaraNcpFwUpdateLock&& lock)
: locked_(lock.locked_) {
lock.locked_ = false;
}
void lock() {
SaraNcpFwUpdate::instance()->lock();
locked_ = true;
}
void unlock() {
SaraNcpFwUpdate::instance()->unlock();
locked_ = false;
}
SaraNcpFwUpdateLock(const SaraNcpFwUpdateLock&) = delete;
SaraNcpFwUpdateLock& operator=(const SaraNcpFwUpdateLock&) = delete;

private:
bool locked_;
};

#ifndef UNIT_TEST
class NcpFwUpdateStatusDiagnostics: public AbstractUnsignedIntegerDiagnosticData {
public:
Expand Down Expand Up @@ -249,3 +298,6 @@ class NcpFwUpdateErrorDiagnostics: public AbstractIntegerDiagnosticData {
} // namespace particle

#endif // #if HAL_PLATFORM_NCP_FW_UPDATE

#endif // #if MODULE_FUNCTION != 2 // BOOTLOADER

20 changes: 9 additions & 11 deletions services/inc/system_error.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@
(SARA_NCP_FW_UPDATE_QUALIFY_FLAGS, "Qualify flags", -400), /* -499 ... -400: SaraNcpFwUpdate errors */ \
(SARA_NCP_FW_UPDATE_CLOUD_CONNECT_ON_ENTRY_TIMEOUT, "Cloud conn. on entry timeout", -405), \
(SARA_NCP_FW_UPDATE_PUBLISH_START, "Publish start err", -410), \
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_DISCONNECT_TIMEOUT, "Setup cell disconn. timeout", -415), \
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_STILL_CONNECTED, "Setup cell still conn.", -420), \
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_CONNECT_TIMEOUT, "Setup cell conn. timeout", -425), \
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_STILL_CONNECTED, "Setup cell still conn.", -415), \
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_CONNECT_TIMEOUT, "Setup cell conn. timeout", -420), \
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_1, "HTTPS err 1", -430), \
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_2, "HTTPS err 2", -431), \
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_3, "HTTPS err 3", -432), \
Expand All @@ -66,14 +65,13 @@
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_8, "HTTPS err 8", -437), \
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_9, "HTTPS err 9", -438), \
(SARA_NCP_FW_UPDATE_DOWNLOAD_RETRY_MAX, "DL retry max err", -440), \
(SARA_NCP_FW_UPDATE_INSTALL_CELLULAR_DISCONNECT_TIMEOUT,"Install cell disconn. timeout",-445), \
(SARA_NCP_FW_UPDATE_START_INSTALL_TIMEOUT, "Install start timeout", -450), \
(SARA_NCP_FW_UPDATE_INSTALL_AT_ERROR, "Install AT err", -455), \
(SARA_NCP_FW_UPDATE_SAME_VERSION, "Same version err", -460), \
(SARA_NCP_FW_UPDATE_INSTALL_TIMEOUT, "Install timeout", -465), \
(SARA_NCP_FW_UPDATE_POWER_OFF_TIMEOUT, "Power off timeout", -470), \
(SARA_NCP_FW_UPDATE_CLOUD_CONNECT_ON_EXIT_TIMEOUT, "Cloud conn. on exit timeout", -475), \
(SARA_NCP_FW_UPDATE_PUBLISH_RESULT, "Publish result err", -480), \
(SARA_NCP_FW_UPDATE_START_INSTALL_TIMEOUT, "Install start timeout", -445), \
(SARA_NCP_FW_UPDATE_INSTALL_AT_ERROR, "Install AT err", -450), \
(SARA_NCP_FW_UPDATE_SAME_VERSION, "Same version err", -455), \
(SARA_NCP_FW_UPDATE_INSTALL_TIMEOUT, "Install timeout", -460), \
(SARA_NCP_FW_UPDATE_POWER_OFF_TIMEOUT, "Power off timeout", -465), \
(SARA_NCP_FW_UPDATE_CLOUD_CONNECT_ON_EXIT_TIMEOUT, "Cloud conn. on exit timeout", -470), \
(SARA_NCP_FW_UPDATE_PUBLISH_RESULT, "Publish result err", -475), \
(COAP, "CoAP error", -1000), /* -1199 ... -1000: CoAP errors */ \
(COAP_4XX, "CoAP: 4xx", -1100), \
(COAP_5XX, "CoAP: 5xx", -1132), \
Expand Down
Loading

0 comments on commit 02d203d

Please sign in to comment.