modules: memfault: Update coredump upload state machine

Update coredump upload state machine and fix a few issues:
- Unify LTE registration and PDN handling
- Do not reset retry counter when connection is lost. This
  could lead to retries never stopping in intermittent coverage.
- Fix bug where state enum was used of event enum.
- Minor code cleanup.

Signed-off-by: Jan Tore Guggedal <[email protected]>

Author: jtguggedal

modules/memfault-firmware-sdk/memfault_lte_coredump.c

@@ -46,11 +46,10 @@ enum library_event {
 	EVENT_LTE_DEREGISTERED,
 	EVENT_PDN_ACTIVATED,
 	EVENT_PDN_DEACTIVATED,
-	EVENT_NETWORK_CONNECTED,
-	EVENT_NETWORK_DISCONNECTED,
 	EVENT_BACKOFF_TIMER_EXPIRED,
 	EVENT_COREDUMP_SEND_FAIL,
 	EVENT_COREDUMP_SEND_SUCCESS,
+	EVENT_COREDUMP_SEND_RETRY_COUNT_EXCEEDED,
 	EVENT_ERROR
 };
 
@@ -71,6 +70,8 @@ struct fsm_state_object {
 
 	bool lte_registered;
 	bool pdn_active;
+
+	bool heartbeat_triggered;
 };
 
 /* Forward declarations: Local functions */
@@ -206,11 +207,18 @@ static void pdn_event_handler(uint8_t cid, enum pdn_event event, int reason)
 {
 	switch (event) {
 	case PDN_EVENT_ACTIVATED:
-		event_send(EVENT_PDN_ACTIVATED);
+		if (cid == 0) {
+			event_send(EVENT_PDN_ACTIVATED);
+		}
+
 		break;
-	case PDN_EVENT_NETWORK_DETACH:
-		__fallthrough;
 	case PDN_EVENT_DEACTIVATED:
+		if (cid == 0) {
+			event_send(EVENT_PDN_DEACTIVATED);
+		}
+
+		break;
+	case PDN_EVENT_NETWORK_DETACH:
 		event_send(EVENT_PDN_DEACTIVATED);
 		break;
 	default:
@@ -219,17 +227,54 @@ static void pdn_event_handler(uint8_t cid, enum pdn_event event, int reason)
 	}
 }
 
+static void init_state_variables(struct fsm_state_object *state_object)
+{
+	state_object->lte_registered = false;
+	state_object->pdn_active = false;
+	state_object->retry_count = 0;
+	state_object->heartbeat_triggered = false;
+}
+
+/* Update registration and PDN state. Returns true if the state changed. */
+static bool update_connectivity(struct fsm_state_object *state_object,
+					 enum library_event event)
+{
+	switch (event) {
+	case EVENT_LTE_REGISTERED:
+		state_object->lte_registered = true;
+
+		return true;
+	case EVENT_LTE_DEREGISTERED:
+		state_object->lte_registered = false;
+
+		return true;
+	case EVENT_PDN_ACTIVATED:
+		state_object->pdn_active = true;
+
+		return true;
+	case EVENT_PDN_DEACTIVATED:
+		state_object->pdn_active = false;
+
+		return true;
+	default:
+		return false;
+	}
+}
+
+/* Returns true if the modem is registered and a PDN bearer is active. */
+static bool is_registered_and_pdn_active(struct fsm_state_object *state_object,
+					 enum library_event event)
+{
+	return state_object->lte_registered && state_object->pdn_active;
+}
+
 /* SMF State Handlers */
 
 static void state_waiting_for_nrf_modem_lib_init_entry(void *o)
 {
 	ARG_UNUSED(o);
 
 	LOG_DBG("state_waiting_for_nrf_modem_lib_init_entry");
-
-	state_object->lte_registered = false;
-	state_object->pdn_active = false;
-	state_object->retry_count = 0;
 }
 
 static enum smf_state_result state_waiting_for_nrf_modem_lib_init_run(void *o)
@@ -238,9 +283,17 @@ static enum smf_state_result state_waiting_for_nrf_modem_lib_init_run(void *o)
 
 	LOG_DBG("state_waiting_for_nrf_modem_lib_init_run");
 
-	if (state_object->event == EVENT_NRF_MODEM_LIB_INITED) {
+	switch (state_object->event) {
+	case EVENT_NRF_MODEM_LIB_INITED:
 		smf_set_state(SMF_CTX(state_object), &states[STATE_RUNNING]);
+
+		return SMF_EVENT_HANDLED;
+
+	case EVENT_NRF_MODEM_LIB_SHUTDOWN:
 		return SMF_EVENT_HANDLED;
+
+	default:
+		break;
 	}
 
 	return SMF_EVENT_PROPAGATE;
@@ -254,6 +307,8 @@ static void state_running_entry(void *o)
 
 	LOG_DBG("state_running_entry");
 
+	init_state_variables(state_object);
+
 	/* Subscribe to +CEREG notifications, level 5 */
 	err = nrf_modem_at_printf("AT+CEREG=5");
 	if (err) {
@@ -278,12 +333,27 @@ static enum smf_state_result state_running_run(void *o)
 
 	LOG_DBG("state_running_run");
 
-	if (state_object->event == EVENT_NRF_MODEM_LIB_SHUTDOWN) {
+	switch (state_object->event) {
+	case EVENT_NRF_MODEM_LIB_SHUTDOWN:
 		smf_set_state(SMF_CTX(state_object), &states[STATE_WAITING_FOR_NRF_MODEM_LIB_INIT]);
+
 		return SMF_EVENT_HANDLED;
-	} else if (state_object->event == EVENT_ERROR) {
+
+	case EVENT_ERROR:
 		smf_set_state(SMF_CTX(state_object), &states[STATE_ERROR]);
+
+		return SMF_EVENT_HANDLED;
+
+	case EVENT_COREDUMP_SEND_RETRY_COUNT_EXCEEDED:
+		__fallthrough;
+
+	case EVENT_COREDUMP_SEND_SUCCESS:
+		smf_set_state(SMF_CTX(state_object), &states[STATE_FINISHED]);
+
 		return SMF_EVENT_HANDLED;
+
+	default:
+		break;
 	}
 
 	return SMF_EVENT_PROPAGATE;
@@ -320,26 +390,10 @@ static enum smf_state_result state_waiting_for_network_connection_run(void *o)
 
 	LOG_DBG("state_waiting_for_network_connection_run");
 
-	switch (state_object->event) {
-	case EVENT_LTE_REGISTERED:
-		state_object->lte_registered = true;
-		break;
-	case EVENT_LTE_DEREGISTERED:
-		state_object->lte_registered = false;
-		break;
-	case EVENT_PDN_ACTIVATED:
-		state_object->pdn_active = true;
-		break;
-	case EVENT_PDN_DEACTIVATED:
-		state_object->pdn_active = false;
-		break;
-	default:
-		/* Don't care */
-		break;
-	}
-
-	if (state_object->lte_registered && state_object->pdn_active) {
+	if (update_connectivity(state_object, state_object->event) &&
+	    is_registered_and_pdn_active(state_object, state_object->event)) {
 		smf_set_state(SMF_CTX(state_object), &states[STATE_NETWORK_CONNECTED]);
+
 		return SMF_EVENT_HANDLED;
 	}
 
@@ -351,8 +405,6 @@ static void state_network_connected_entry(void *o)
 	ARG_UNUSED(o);
 
 	LOG_DBG("state_network_connected_entry");
-
-	state_object->retry_count = 0;
 }
 
 static enum smf_state_result state_network_connected_run(void *o)
@@ -361,15 +413,10 @@ static enum smf_state_result state_network_connected_run(void *o)
 
 	LOG_DBG("state_network_connected_run");
 
-	if (state_object->event == EVENT_LTE_DEREGISTERED) {
-		state_object->lte_registered = false;
-
+	if (update_connectivity(state_object, state_object->event) &&
+	    !is_registered_and_pdn_active(state_object, state_object->event)) {
 		smf_set_state(SMF_CTX(state_object), &states[STATE_WAITING_FOR_NETWORK_CONNECTION]);
-		return SMF_EVENT_HANDLED;
-	} else if (state_object->event == EVENT_PDN_DEACTIVATED) {
-		state_object->pdn_active = false;
 
-		smf_set_state(SMF_CTX(state_object), &states[STATE_WAITING_FOR_NETWORK_CONNECTION]);
 		return SMF_EVENT_HANDLED;
 	}
 
@@ -383,10 +430,13 @@ static void state_coredump_send_attempt_entry(void *o)
 	struct fsm_state_object *state_object = o;
 
 	LOG_DBG("state_coredump_send_attempt_entry");
-	LOG_DBG("Triggering heartbeat");
-	LOG_DBG("Attempting to send coredump");
 
-	memfault_metrics_heartbeat_debug_trigger();
+	if (!state_object->heartbeat_triggered) {
+		LOG_DBG("Triggering heartbeat");
+		memfault_metrics_heartbeat_debug_trigger();
+
+		state_object->heartbeat_triggered = true;
+	}
 
 	if (IS_ENABLED(CONFIG_MEMFAULT_NCS_POST_MODEM_TRACE_ON_COREDUMP)) {
 		err = memfault_lte_coredump_modem_trace_init();
@@ -403,6 +453,8 @@ static void state_coredump_send_attempt_entry(void *o)
 		}
 	}
 
+	LOG_DBG("Attempting to send coredump");
+
 	err = memfault_zephyr_port_post_data();
 	if (err) {
 		LOG_DBG("Failed to post data to Memfault");
@@ -421,9 +473,7 @@ static enum smf_state_result state_coredump_send_attempt_run(void *o)
 
 	if (state_object->event == EVENT_COREDUMP_SEND_FAIL) {
 		smf_set_state(SMF_CTX(state_object), &states[STATE_COREDUMP_SEND_BACKOFF]);
-		return SMF_EVENT_HANDLED;
-	} else if (state_object->event == EVENT_COREDUMP_SEND_SUCCESS) {
-		smf_set_state(SMF_CTX(state_object), &states[STATE_FINISHED]);
+
 		return SMF_EVENT_HANDLED;
 	}
 
@@ -489,7 +539,10 @@ static void schedule_send_backoff(struct fsm_state_object *state_object)
 	state_object->retry_count++;
 
 	if (state_object->retry_count >= CONFIG_MEMFAULT_NCS_POST_COREDUMP_RETRIES_MAX) {
-		event_send(STATE_FINISHED);
+		event_send(EVENT_COREDUMP_SEND_RETRY_COUNT_EXCEEDED);
+
+		LOG_DBG("Retry count exceeded");
+
 		return;
 	}
 

modules/memfault-firmware-sdk/memfault_lte_coredump_modem_trace.c

@@ -144,7 +144,6 @@ int memfault_lte_coredump_modem_trace_init(void)
 	static bool initialized;
 
 	if (initialized) {
-		LOG_ERR("Already initialized");
 		return -EALREADY;
 	}