Cleanup of the 93m1 applications by SyverHaraldsen · Pull Request #29 · nrfconnect/ncs-serial-modem-host-applications

SyverHaraldsen · 2026-06-23T11:05:26Z

Making the applications more readable and uniform to our other samples.

jtguggedal · 2026-06-23T12:16:34Z

+		return;
+	}
+
+	(void)snprintf(cmd, sizeof(cmd),


Is it guaranteed that it is always okay to void the return value?
snprintk() should be used instead

jtguggedal · 2026-06-23T12:17:21Z

 	}
 	*chg_status = val.val1;

 	struct sensor_value vbus_val;


Can be moved to top of function scope

jtguggedal · 2026-06-23T12:18:01Z

+			      (enum sensor_channel)SENSOR_CHAN_NPM13XX_CHARGER_VBUS_STATUS,
 			      (enum sensor_attribute)SENSOR_ATTR_NPM13XX_CHARGER_VBUS_PRESENT,
 			      &vbus_val);
 	*vbus = (err == 0) && (vbus_val.val1 != 0);


Is it fine to return 0 even though this fails?

jtguggedal · 2026-06-23T12:19:45Z

 	}

-	return (int)soc;
+	LOG_INF("Battery %d%% sent to nRF Cloud", (int)soc);


Is it intentional to communicate with cloud from battery module?
Anyway, usually want to avoid INF level in modules. Could be DBG instead

jtguggedal · 2026-06-23T12:23:45Z

+	uint8_t msg_buf[MAX_MSG_SIZE];
+	bool connected = false;

+	err = modem_at_urc_subscribe("%NRFCLOUDLOCATION: ", on_location, NULL);


Is it safe with multi-thread access to modem APIs?

jtguggedal · 2026-06-23T12:24:53Z

+	struct modem_backend_uart uart_backend;
+	struct modem_pipe *uart_pipe;
+	struct modem_chat chat;
+	uint8_t uart_rx_buf[CONFIG_APP_MODEM_AT_UART_RECEIVE_BUF_SIZE] __aligned(4);


Needed to explicitly align this?

jtguggedal · 2026-06-23T12:28:57Z

 	}

-	if (strlen(req) >= sizeof(request_buf)) {
+	if (snprintf((char *)at_ctx.request_buf, sizeof(at_ctx.request_buf), "%s", req) >=


Suggested change

if (snprintf((char *)at_ctx.request_buf, sizeof(at_ctx.request_buf), "%s", req) >=

if (snprintk((char *)at_ctx.request_buf, sizeof(at_ctx.request_buf), "%s", req) >=

jtguggedal · 2026-06-23T12:29:36Z

 	}

-	if (strlen(req) >= sizeof(request_buf)) {
+	if (snprintf((char *)at_ctx.request_buf, sizeof(at_ctx.request_buf), "%s", req) >=


I would do this in two steps, first assign to a variable and then check it

jtguggedal · 2026-06-23T12:30:19Z

-	k_sem_give(&cereg_sem);
+	struct cereg_msg msg = { .stat = atoi(argv[1]) };
+
+	(void)zbus_chan_pub(&cereg_chan, &msg, K_NO_WAIT);


I would check the return value

jtguggedal · 2026-06-23T12:31:29Z

 	while (true) {
-		bool reg = (atomic_get(&cereg_stat) == 1 || atomic_get(&cereg_stat) == 5);
-		k_timeout_t wake = (reg && !connected) ? K_MSEC(500) : wait;
+		bool reg = (cereg_stat == 1 || cereg_stat == 5);


I would check return value of network_attach() above

simensrostad

To make network handling easier and reduce overall complexity I would use zephyr cellular modem driver and net mgmt for basic connectivity control. The rest can be pure AT. IMO its worth the extra overhead.

The 93m1 driver also exposes ip,apn, psm control that can be re-used.

simensrostad · 2026-06-24T07:37:25Z

+	(void)snprintf(cmd, sizeof(cmd),
+		       "AT%%NRFCLOUDMESSAGE={\"appId\":\"BATTERY\",\"data\":\"%d\"}", (int)soc);
+
+	err = modem_at_run(cmd, NULL, 0, CONFIG_APP_BATTERY_AT_TIMEOUT_SECONDS);


Make sure that modem_at_run() is thread safe when calling from multiple modules. I think end-goal should be to have one module handle cloud related calls. This would also make it easier for users to attach an aggregator/storage type module. AND you'll worry less about conccurency issues like thread safety.

simensrostad · 2026-06-24T07:38:18Z

-	(void)snprintf(cmd, sizeof(cmd),
-		       "AT%%NRFCLOUDMESSAGE={\"appId\":\"BATTERY\",\"data\":\"%d\"}", soc);
+	while (true) {
+		err = zbus_sub_wait_msg(&battery, &chan, msg_buf, K_FOREVER);


Make sure that the latest changes applied to ATT are in here. cc @Trond-F-Christiansen

simensrostad · 2026-06-24T07:38:59Z

+
+CHANNEL_LIST(ADD_OBSERVERS)
+
+static bool is_coordinate(const char *s);


Suggested change

static bool is_coordinate(const char *s);

/* Forward declarations */

static bool is_coordinate(const char *s);

simensrostad · 2026-06-24T07:41:41Z

+{
+	struct location_msg msg = { .type = LOCATION_FIX_REQUEST, .mode = LOCATION_MODE_ALL };
+
+	(void)zbus_chan_pub(&location_chan, &msg, PUB_TIMEOUT);


Make sure that return values are explicitly handled throughout the code to avoid undefined behavior.

simensrostad · 2026-06-24T07:42:38Z

+}
+#endif
+
+#if defined(CONFIG_APP_BATTERY)


Consider making battery and other modules in the app non-optional to avoid ifdeffing if it makes the code harder to read.

simensrostad · 2026-06-24T07:44:04Z

 	LOG_INF("Serial Modem Host 93m1 starting");
+
+#if defined(CONFIG_APP_BATTERY)
+	k_work_reschedule(&battery_sample_work, K_NO_WAIT);


Easy to miss checking retvals for these. A lot of bad practise in NCS leaving these unchecked.

A general cleanup of the 93m1_ppp application. This includes removing unused code, and moving logic to be more uniform. Signed-off-by: Syver Haraldsen <syver.haraldsen@nordicsemi.no>

General cleanup and refactoring of the 93m1_at application. Signed-off-by: Syver Haraldsen <syver.haraldsen@nordicsemi.no>

sonarqubecloud · 2026-06-25T11:08:20Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
27.4% Duplication on New Code

See analysis details on SonarQube Cloud

SyverHaraldsen force-pushed the applications-93-cleanup branch from 8f45573 to 379f687 Compare June 23, 2026 11:16

jtguggedal reviewed Jun 23, 2026

View reviewed changes

simensrostad reviewed Jun 24, 2026

View reviewed changes

SyverHaraldsen force-pushed the applications-93-cleanup branch 3 times, most recently from 3d943d4 to 5561f83 Compare June 25, 2026 08:46

applications: 93m1_ppp: General cleanup of the 93m1_ppp application

7bf96f4

A general cleanup of the 93m1_ppp application. This includes removing unused code, and moving logic to be more uniform. Signed-off-by: Syver Haraldsen <syver.haraldsen@nordicsemi.no>

SyverHaraldsen force-pushed the applications-93-cleanup branch from 5561f83 to d3a4129 Compare June 25, 2026 08:46

applications: 93m1_at: General cleanup of the 93m1_at application

ab5c6f8

General cleanup and refactoring of the 93m1_at application. Signed-off-by: Syver Haraldsen <syver.haraldsen@nordicsemi.no>

SyverHaraldsen force-pushed the applications-93-cleanup branch from d3a4129 to ab5c6f8 Compare June 25, 2026 10:33

SyverHaraldsen merged commit 8e49b20 into main Jun 25, 2026
13 of 22 checks passed

	if (snprintf((char *)at_ctx.request_buf, sizeof(at_ctx.request_buf), "%s", req) >=
	if (snprintk((char *)at_ctx.request_buf, sizeof(at_ctx.request_buf), "%s", req) >=


		CHANNEL_LIST(ADD_OBSERVERS)

		static bool is_coordinate(const char *s);

	static bool is_coordinate(const char *s);
	/* Forward declarations */
	static bool is_coordinate(const char *s);

Uh oh!

Conversation

SyverHaraldsen commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simensrostad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jun 25, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants