samples: cellular: udp: cleanup and add way to shutdown modem #13066

MirkoCovizzi · 2023-11-13T12:12:52Z

This commit adds a new Kconfig option for configuring
the number of data transmissions to the server.
Shutdown the modem library when there are no more
transmissions to execute.
Moved sample to CI-iot-samples-test plan.

NordicBuilder · 2023-11-13T12:15:26Z

Test specification

CI/Jenkins/NRF

Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
test-fw-nrfconnect-nrf-iot_samples	X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

lemrey · 2023-11-13T12:44:43Z

samples/cellular/udp/src/main.c

+static bool is_nrf9160(void)
+{
+	char resp[32];
+
+	if (nrf_modem_at_cmd(resp, sizeof(resp), "AT+CGMM") == 0) {
+		if (strstr(resp, "nRF9160") != NULL) {
+			return true;
+		}
+	}
+
+	return false;
+}


This sample only works on nRF 9 series boards. It may have had support for qemu_cortex_m3 earlier, but that is no longer the case. I do not see why this function is necessary anymore, can we remove it?

I'll update the sample accordingly then. I was under the assumption that qemu target was still required.

lemrey · 2023-11-13T12:47:22Z

samples/cellular/udp/Kconfig

+config UDP_DATA_UPLOAD_FOREVER
+	bool "Enable that data is transmitted to the server forever"
+	default y
+
+config UDP_DATA_UPLOAD_ITERATIONS
+	int "NUmber of data transmissions to the server before shutdown" if !UDP_DATA_UPLOAD_FOREVER
+	default 5


We shouldn't need two entries for this; one config for the number of iterations should be sufficient.
If set to -1 let the loop run forever.

I thought about the -1 idea before, but I don't like the fact that a different value has a completely different behavior. While these two options make sense just by reading their name, in the -1 case the user will have to find such behavior in the documentation instead. I'll think more about it.

What about UDP_DATA_UPLOAD_ITERATIONS_BEFORE_EXIT, then -1 means that the application will not exit after n iterations.

@MechanicV1 made me aware that the download_client uses -1 already in a similar way

sdk-nrf/subsys/net/lib/download_client/Kconfig

Line 112 in 8f8c268

range -1 600000

so I updated this sample to align to that.

lemrey · 2023-11-13T12:48:45Z

samples/cellular/udp/src/main.c

@@ -239,5 +269,14 @@ int main(void)

 	k_work_schedule(&socket_transmission_work, K_NO_WAIT);

+#if defined(CONFIG_NRF_MODEM_LIB) && !defined(CONFIG_UDP_DATA_UPLOAD_FOREVER)


You can remove all the occurrences where we check for NRF_MODEM_LIB. This sample runs only on nRF9 series boards, so this is always defined. Keep it a separate commit if you can.

lemrey · 2023-11-13T12:49:13Z

samples/cellular/udp/src/main.c

+static int modem_shutdown(void)
+{
+	int err;
+
+	err = nrf_modem_lib_shutdown();
+	if (err) {
+		printk("Failed to shutdown modem library, error: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}


There is no point to define this function to call nrf_modem_lib_shutdown(); just call nrf_modem_lib_shutdown() directly.

NordicBuilder · 2023-11-13T13:13:24Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

samples/cellular/udp/Kconfig

eivindj-nordic · 2023-11-23T09:54:06Z

samples/cellular/udp/Kconfig

+config UDP_DATA_UPLOAD_FOREVER
+	bool "Enable that data is transmitted to the server forever"
+	default y
+
+config UDP_DATA_UPLOAD_ITERATIONS
+	int "NUmber of data transmissions to the server before shutdown" if !UDP_DATA_UPLOAD_FOREVER
+	default 5


What about UDP_DATA_UPLOAD_ITERATIONS_BEFORE_EXIT, then -1 means that the application will not exit after n iterations.

samples/cellular/udp/Kconfig

eivindj-nordic · 2023-11-23T15:56:03Z

samples/cellular/udp/src/main.c

+	if (data_upload_iterations > 0) {
+		data_upload_iterations--;
+		goto transmit;
+	} else if (data_upload_iterations == 0) {
+		k_sem_give(&modem_shutdown_sem);
+		return;
+	} else if (data_upload_iterations < 0) {
+		goto transmit;
+	}


The last else if is not strictly necessary. We could instead do:

Suggested change

if (data_upload_iterations > 0) {

data_upload_iterations--;

goto transmit;

} else if (data_upload_iterations == 0) {

k_sem_give(&modem_shutdown_sem);

return;

} else if (data_upload_iterations < 0) {

goto transmit;

}

if (data_upload_iterations == 0) {

k_sem_give(&modem_shutdown_sem);

return;

}

if (data_upload_iterations > 0) {

data_upload_iterations--;

}

Not sure what is clearest.

eivindj-nordic · 2023-11-24T14:22:27Z

samples/cellular/udp/src/main.c

+	/* Transmit indefinitely. */
+	if (CONFIG_UDP_DATA_UPLOAD_ITERATIONS == -1) {
+		goto transmit;
+	}


This is not necessary

lemrey · 2023-11-24T16:24:26Z

samples/cellular/udp/Kconfig

+config UDP_DATA_UPLOAD_ITERATIONS
+	int "Number of data transmissions to the server before shutdown"
+	range -1 2147483647
+	default -1


I think the sample should terminate by default, so that we can run it in CI.

@MechanicV1 From CI point of view should this sample terminate by default? Or can CI configure it locally?

CI can change the Kconfig value when building it. Though many other samples are terminating by default.

Test automation will take care of setting the Kconfig ! @MirkoCovizzi

I think we should implement the same behavior as other sample. It's easy enough to change that for users if needed.
CI might want to override the value anyway to shorten the time it takes to complete the test.

MechanicV1 · 2023-11-27T08:37:08Z

samples/cellular/udp/Kconfig

+config UDP_DATA_UPLOAD_ITERATIONS
+	int "Number of data transmissions to the server before shutdown"
+	range -1 2147483647
+	default -1


Test automation will take care of setting the Kconfig ! @MirkoCovizzi

divipillai · 2023-12-19T15:15:19Z

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

@@ -316,6 +316,10 @@ Cellular samples

  * Added the configuration overlay file :file:`overlay-supl.conf` for building the sample with SUPL assistance support.

+* :ref:`udp` sample:
+
+  * Added the :kconfig:option:`CONFIG_UDP_DATA_UPLOAD_ITERATIONS` Kconfig option for configuring the number of data transmissions to the server.


Suggested change

* Added the :kconfig:option:`CONFIG_UDP_DATA_UPLOAD_ITERATIONS` Kconfig option for configuring the number of data transmissions to the server.

* Added the :ref:`CONFIG_UDP_DATA_UPLOAD_ITERATIONS <CONFIG_UDP_DATA_UPLOAD_ITERATIONS>` Kconfig option for configuring the number of data transmissions to the server.

@divipillai Is this the new standard syntax to use from now on?

This is the way to represent the application-specific Kconfig options. For example, for UDP sample, the ones that are defined here: https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/samples/cellular/udp/README.html#configuration. The Kconfig options defined in the Kconfig reference (library-specific) are provided with the ":kconfig:option:" syntax.

divipillai · 2023-12-19T15:16:27Z

samples/cellular/udp/README.rst

+
+CONFIG_UDP_DATA_UPLOAD_ITERATIONS - UDP data upload iterations configuration
+   This configuration option sets the number of times the sample transmits data to the server before shutting down.
+   Set to -1 to transmit indefinitely.


Suggested change

Set to -1 to transmit indefinitely.

Set to ``-1`` to transmit indefinitely.

This commit adds a new Kconfig option for configuring the number of data transmissions to the server. Shutdown the modem library when there are no more transmissions to execute. Signed-off-by: Mirko Covizzi <[email protected]>

This commit aligns the semaphore naming convention. Signed-off-by: Mirko Covizzi <[email protected]>

This commit moves the cellular/udp sample from `CI-thingy91-test` to `CI-iot-samples-test`. This is because now the testing automation for `samples/cellular` is under the `CI-iot-samples-test` plan. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi self-assigned this Nov 13, 2023

MirkoCovizzi requested review from rlubos and lemrey as code owners November 13, 2023 12:12

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 13, 2023

MirkoCovizzi removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 13, 2023

MirkoCovizzi requested a review from carlescufi as a code owner November 13, 2023 12:23

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Nov 13, 2023

MirkoCovizzi changed the title ~~samples: cellular: udp: move function~~ samples: cellular: udp: cleanup and add way to shutdown modem Nov 13, 2023

MirkoCovizzi requested review from MechanicV1 and eivindj-nordic November 13, 2023 12:32

lemrey reviewed Nov 13, 2023

View reviewed changes

eivindj-nordic reviewed Nov 23, 2023

View reviewed changes

eivindj-nordic approved these changes Nov 23, 2023

View reviewed changes

samples/cellular/udp/Kconfig Outdated Show resolved Hide resolved

samples/cellular/udp/Kconfig Outdated Show resolved Hide resolved

eivindj-nordic reviewed Nov 23, 2023

View reviewed changes

samples/cellular/udp/Kconfig Show resolved Hide resolved

MirkoCovizzi requested review from eivindj-nordic and lemrey November 23, 2023 14:02

eivindj-nordic reviewed Nov 23, 2023

View reviewed changes

MechanicV1 approved these changes Nov 24, 2023

View reviewed changes

MirkoCovizzi requested a review from a team as a code owner November 24, 2023 14:14

eivindj-nordic reviewed Nov 24, 2023

View reviewed changes

lemrey requested changes Nov 24, 2023

View reviewed changes

MechanicV1 approved these changes Nov 27, 2023

View reviewed changes

MirkoCovizzi requested a review from stig-bjorlykke November 27, 2023 11:27

shanthanordic requested review from jorgenmk and removed request for a team November 30, 2023 20:47

MirkoCovizzi requested a review from lemrey December 19, 2023 13:56

lemrey approved these changes Dec 19, 2023

View reviewed changes

divipillai reviewed Dec 19, 2023

View reviewed changes

MirkoCovizzi added 3 commits December 19, 2023 22:26

samples: cellular: udp: add modem shutdown

9c9cf42

This commit adds a new Kconfig option for configuring the number of data transmissions to the server. Shutdown the modem library when there are no more transmissions to execute. Signed-off-by: Mirko Covizzi <[email protected]>

samples: cellular: udp: align sem naming

e1a7cfc

This commit aligns the semaphore naming convention. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi requested a review from divipillai December 19, 2023 21:27

divipillai approved these changes Dec 20, 2023

View reviewed changes

rlubos merged commit 04a2450 into nrfconnect:main Dec 20, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: cellular: udp: cleanup and add way to shutdown modem #13066

samples: cellular: udp: cleanup and add way to shutdown modem #13066

MirkoCovizzi commented Nov 13, 2023 •

edited

Loading

NordicBuilder commented Nov 13, 2023 •

edited

Loading

lemrey Nov 13, 2023 •

edited

Loading

MirkoCovizzi Nov 13, 2023

lemrey Nov 13, 2023

MirkoCovizzi Nov 13, 2023 •

edited

Loading

eivindj-nordic Nov 23, 2023

MirkoCovizzi Nov 23, 2023

lemrey Nov 13, 2023

lemrey Nov 13, 2023 •

edited

Loading

NordicBuilder commented Nov 13, 2023

eivindj-nordic Nov 23, 2023

eivindj-nordic Nov 23, 2023 •

edited

Loading

eivindj-nordic Nov 24, 2023

lemrey Nov 24, 2023

MirkoCovizzi Nov 24, 2023 •

edited

Loading

eivindj-nordic Nov 25, 2023

MechanicV1 Nov 27, 2023

lemrey Nov 27, 2023

MechanicV1 Nov 27, 2023

divipillai Dec 19, 2023

MirkoCovizzi Dec 19, 2023

divipillai Dec 19, 2023

divipillai Dec 19, 2023

		@@ -239,5 +269,14 @@ int main(void)

		k_work_schedule(&socket_transmission_work, K_NO_WAIT);

		#if defined(CONFIG_NRF_MODEM_LIB) && !defined(CONFIG_UDP_DATA_UPLOAD_FOREVER)

	* Added the :kconfig:option:`CONFIG_UDP_DATA_UPLOAD_ITERATIONS` Kconfig option for configuring the number of data transmissions to the server.
	* Added the :ref:`CONFIG_UDP_DATA_UPLOAD_ITERATIONS <CONFIG_UDP_DATA_UPLOAD_ITERATIONS>` Kconfig option for configuring the number of data transmissions to the server.

	Set to -1 to transmit indefinitely.
	Set to ``-1`` to transmit indefinitely.

samples: cellular: udp: cleanup and add way to shutdown modem #13066

samples: cellular: udp: cleanup and add way to shutdown modem #13066

Conversation

MirkoCovizzi commented Nov 13, 2023 • edited Loading

NordicBuilder commented Nov 13, 2023 • edited Loading

Test specification

lemrey Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MirkoCovizzi Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemrey Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

NordicBuilder commented Nov 13, 2023

Choose a reason for hiding this comment

eivindj-nordic Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MirkoCovizzi Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MirkoCovizzi commented Nov 13, 2023 •

edited

Loading

NordicBuilder commented Nov 13, 2023 •

edited

Loading

lemrey Nov 13, 2023 •

edited

Loading

MirkoCovizzi Nov 13, 2023 •

edited

Loading

lemrey Nov 13, 2023 •

edited

Loading

eivindj-nordic Nov 23, 2023 •

edited

Loading

MirkoCovizzi Nov 24, 2023 •

edited

Loading