Skip to content

Commit 02d203d

Browse files
technoblytechnobly
committed
addresses final PR comments
- 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
1 parent 06134f9 commit 02d203d

File tree

9 files changed

+162
-104
lines changed

9 files changed

+162
-104
lines changed

‎hal/network/ncp/cellular/cellular_hal.cpp‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,12 +110,17 @@ hal_net_access_tech_t fromCellularAccessTechnology(CellularAccessTechnology rat)
110110
}
111111

112112
struct CellularHalUrcHandler {
113+
CellularHalUrcHandler(const char* prefix, hal_cellular_urc_callback_t callback, void* context) :
114+
prefix(prefix),
115+
callback(callback),
116+
context(context) {
117+
}
113118
const char* prefix;
114119
hal_cellular_urc_callback_t callback;
115120
void* context;
116121
};
117122

118-
Vector<CellularHalUrcHandler> sUrcHandlers;
123+
Vector<std::unique_ptr<CellularHalUrcHandler>> sUrcHandlers;
119124

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

545550
const NcpClientLock lock(client);
546551

547-
sUrcHandlers.append({
548-
.prefix = prefix,
549-
.callback = cb,
550-
.context = context
551-
});
552-
auto& handler = sUrcHandlers.last();
552+
auto entry = std::make_unique<CellularHalUrcHandler>(prefix, cb, context);
553+
CHECK_TRUE(entry, SYSTEM_ERROR_NO_MEMORY);
554+
sUrcHandlers.append(std::move(entry));
555+
auto handler = sUrcHandlers.last().get();
553556

554-
return parser->addUrcHandler(prefix, commonUrcHandler, &handler);
557+
return parser->addUrcHandler(prefix, commonUrcHandler, handler);
555558
}
556559

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

566569
parser->removeUrcHandler(prefix);
567570
for (int i = 0; i < sUrcHandlers.size(); ++i) {
568-
if (strcmp(sUrcHandlers.at(i).prefix, prefix) == 0) {
571+
if (strcmp(sUrcHandlers.at(i).get()->prefix, prefix) == 0) {
569572
sUrcHandlers.removeAt(i);
570573
break;
571574
}

‎services/inc/ncp_fw_update.h‎

Lines changed: 56 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,23 +17,38 @@
1717

1818
#pragma once
1919

20+
#if MODULE_FUNCTION != 2 // BOOTLOADER
21+
2022
#include "hal_platform.h"
23+
24+
#if HAL_PLATFORM_NCP_FW_UPDATE
25+
// STATIC_ASSERT already defined from static_assert.h as PARTICLE_STATIC_ASSERT
26+
#ifdef STATIC_ASSERT
27+
#undef STATIC_ASSERT
28+
#endif
29+
// app_util.h unconditionally defines STATIC_ASSERT
30+
#include "static_recursive_mutex.h"
31+
// Allow STATIC_ASSERT to be defined as PARTICLE_STATIC_ASSERT in cellular_hal.h below
32+
#ifdef STATIC_ASSERT
33+
#undef STATIC_ASSERT
34+
#endif
35+
#endif // HAL_PLATFORM_NCP_FW_UPDATE
36+
2137
#include "system_tick_hal.h"
2238
#include "system_defs.h"
2339
#include "system_mode.h"
24-
// #include "system_network.h"
2540
#include "cellular_hal.h"
2641
#include "platform_ncp.h"
2742
#include "diagnostics.h"
2843
#include "spark_wiring_diagnostics.h"
2944
#include "ncp_fw_update_dynalib.h"
30-
#include "static_assert.h"
3145

3246
#if HAL_PLATFORM_NCP
3347
#include "at_parser.h"
3448
#include "at_response.h"
3549
#endif // HAL_PLATFORM_NCP
3650

51+
3752
#if HAL_PLATFORM_NCP_FW_UPDATE
3853

3954
// Change to 0 for debugging faster
@@ -89,7 +104,7 @@ enum SaraNcpFwUpdateState {
89104
FW_UPDATE_STATE_FINISHED_CLOUD_CONNECTED = 16,
90105
FW_UPDATE_STATE_FINISHED_IDLE = 17,
91106
};
92-
static_assert(FW_UPDATE_STATE_IDLE == 0 && FW_UPDATE_STATE_FINISHED_IDLE == 17, "SaraNcpFwUpdateState size changed!");
107+
PARTICLE_STATIC_ASSERT(SaraNcpFwUpdateState_size, FW_UPDATE_STATE_IDLE == 0 && FW_UPDATE_STATE_FINISHED_IDLE == 17);
93108

94109
enum SaraNcpFwUpdateStatus {
95110
FW_UPDATE_STATUS_IDLE = 0,
@@ -99,7 +114,7 @@ enum SaraNcpFwUpdateStatus {
99114
FW_UPDATE_STATUS_FAILED = 4,
100115
FW_UPDATE_STATUS_NONE = 5, // for diagnostics
101116
};
102-
static_assert(FW_UPDATE_STATUS_IDLE == 0 && FW_UPDATE_STATUS_NONE == 5, "SaraNcpFwUpdateStatus size changed!");
117+
PARTICLE_STATIC_ASSERT(SaraNcpFwUpdateStatus_size, FW_UPDATE_STATUS_IDLE == 0 && FW_UPDATE_STATUS_NONE == 5);
103118

104119
const system_tick_t NCP_FW_MODEM_INSTALL_ATOK_INTERVAL = 10000;
105120
const system_tick_t NCP_FW_MODEM_INSTALL_START_TIMEOUT = 5 * 60000;
@@ -170,6 +185,8 @@ class SaraNcpFwUpdate {
170185
int checkUpdate(uint32_t version = 0);
171186
int enableUpdates();
172187
int updateStatus();
188+
int lock();
189+
int unlock();
173190

174191
protected:
175192
SaraNcpFwUpdate();
@@ -201,6 +218,8 @@ class SaraNcpFwUpdate {
201218
SaraNcpFwUpdateCallbacks saraNcpFwUpdateCallbacks_ = {};
202219
HTTPSresponse httpsResp_ = {};
203220

221+
StaticRecursiveMutex mutex_;
222+
204223
static int cbUHTTPER(int type, const char* buf, int len, HTTPSerror* data);
205224
static int cbULSTFILE(int type, const char* buf, int len, int* data);
206225
static int httpRespCallback(AtResponseReader* reader, const char* prefix, void* data);
@@ -219,6 +238,36 @@ class SaraNcpFwUpdate {
219238
void logSaraNcpFwUpdateData(SaraNcpFwUpdateData& data);
220239
};
221240

241+
class SaraNcpFwUpdateLock {
242+
public:
243+
SaraNcpFwUpdateLock()
244+
: locked_(false) {
245+
lock();
246+
}
247+
~SaraNcpFwUpdateLock() {
248+
if (locked_) {
249+
unlock();
250+
}
251+
}
252+
SaraNcpFwUpdateLock(SaraNcpFwUpdateLock&& lock)
253+
: locked_(lock.locked_) {
254+
lock.locked_ = false;
255+
}
256+
void lock() {
257+
SaraNcpFwUpdate::instance()->lock();
258+
locked_ = true;
259+
}
260+
void unlock() {
261+
SaraNcpFwUpdate::instance()->unlock();
262+
locked_ = false;
263+
}
264+
SaraNcpFwUpdateLock(const SaraNcpFwUpdateLock&) = delete;
265+
SaraNcpFwUpdateLock& operator=(const SaraNcpFwUpdateLock&) = delete;
266+
267+
private:
268+
bool locked_;
269+
};
270+
222271
#ifndef UNIT_TEST
223272
class NcpFwUpdateStatusDiagnostics: public AbstractUnsignedIntegerDiagnosticData {
224273
public:
@@ -249,3 +298,6 @@ class NcpFwUpdateErrorDiagnostics: public AbstractIntegerDiagnosticData {
249298
} // namespace particle
250299

251300
#endif // #if HAL_PLATFORM_NCP_FW_UPDATE
301+
302+
#endif // #if MODULE_FUNCTION != 2 // BOOTLOADER
303+

‎services/inc/system_error.h‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,8 @@
5353
(SARA_NCP_FW_UPDATE_QUALIFY_FLAGS, "Qualify flags", -400), /* -499 ... -400: SaraNcpFwUpdate errors */ \
5454
(SARA_NCP_FW_UPDATE_CLOUD_CONNECT_ON_ENTRY_TIMEOUT, "Cloud conn. on entry timeout", -405), \
5555
(SARA_NCP_FW_UPDATE_PUBLISH_START, "Publish start err", -410), \
56-
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_DISCONNECT_TIMEOUT, "Setup cell disconn. timeout", -415), \
57-
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_STILL_CONNECTED, "Setup cell still conn.", -420), \
58-
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_CONNECT_TIMEOUT, "Setup cell conn. timeout", -425), \
56+
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_STILL_CONNECTED, "Setup cell still conn.", -415), \
57+
(SARA_NCP_FW_UPDATE_SETUP_CELLULAR_CONNECT_TIMEOUT, "Setup cell conn. timeout", -420), \
5958
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_1, "HTTPS err 1", -430), \
6059
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_2, "HTTPS err 2", -431), \
6160
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_3, "HTTPS err 3", -432), \
@@ -66,14 +65,13 @@
6665
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_8, "HTTPS err 8", -437), \
6766
(SARA_NCP_FW_UPDATE_HTTPS_SETUP_9, "HTTPS err 9", -438), \
6867
(SARA_NCP_FW_UPDATE_DOWNLOAD_RETRY_MAX, "DL retry max err", -440), \
69-
(SARA_NCP_FW_UPDATE_INSTALL_CELLULAR_DISCONNECT_TIMEOUT,"Install cell disconn. timeout",-445), \
70-
(SARA_NCP_FW_UPDATE_START_INSTALL_TIMEOUT, "Install start timeout", -450), \
71-
(SARA_NCP_FW_UPDATE_INSTALL_AT_ERROR, "Install AT err", -455), \
72-
(SARA_NCP_FW_UPDATE_SAME_VERSION, "Same version err", -460), \
73-
(SARA_NCP_FW_UPDATE_INSTALL_TIMEOUT, "Install timeout", -465), \
74-
(SARA_NCP_FW_UPDATE_POWER_OFF_TIMEOUT, "Power off timeout", -470), \
75-
(SARA_NCP_FW_UPDATE_CLOUD_CONNECT_ON_EXIT_TIMEOUT, "Cloud conn. on exit timeout", -475), \
76-
(SARA_NCP_FW_UPDATE_PUBLISH_RESULT, "Publish result err", -480), \
68+
(SARA_NCP_FW_UPDATE_START_INSTALL_TIMEOUT, "Install start timeout", -445), \
69+
(SARA_NCP_FW_UPDATE_INSTALL_AT_ERROR, "Install AT err", -450), \
70+
(SARA_NCP_FW_UPDATE_SAME_VERSION, "Same version err", -455), \
71+
(SARA_NCP_FW_UPDATE_INSTALL_TIMEOUT, "Install timeout", -460), \
72+
(SARA_NCP_FW_UPDATE_POWER_OFF_TIMEOUT, "Power off timeout", -465), \
73+
(SARA_NCP_FW_UPDATE_CLOUD_CONNECT_ON_EXIT_TIMEOUT, "Cloud conn. on exit timeout", -470), \
74+
(SARA_NCP_FW_UPDATE_PUBLISH_RESULT, "Publish result err", -475), \
7775
(COAP, "CoAP error", -1000), /* -1199 ... -1000: CoAP errors */ \
7876
(COAP_4XX, "CoAP: 4xx", -1100), \
7977
(COAP_5XX, "CoAP: 5xx", -1132), \

0 commit comments

Comments
 (0)