Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nRF Cloud CoAP Sample #11648

Closed
wants to merge 13 commits into from

Conversation

plskeggs
Copy link
Contributor

@plskeggs plskeggs commented Jun 28, 2023

This sample is used to demonstrate the nRF Cloud CoAP library in PR: #11535.

This PR contains the sample that previously had been contained within the above PR.

Only the 2 last commits belong to this current PR.

@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval. labels Jun 28, 2023
@plskeggs plskeggs marked this pull request as draft June 28, 2023 21:51
@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 9db8f05 to 6b7dd1c Compare June 29, 2023 00:11
@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 6b7dd1c to fec8aef Compare June 30, 2023 03:42
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jun 30, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-ci-nrfconnect-boot-fw-update X
test-fw-nrfconnect-nrf-iot_cloud X
test-fw-nrfconnect-nrf-iot_mosh X
test-fw-nrfconnect-nrf-iot_positioning X
test-fw-nrfconnect-nrf-iot_serial_lte_modem X
test-fw-nrfconnect-nrf-iot_thingy91 X

Detailed information of selected test modules

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

@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch 2 times, most recently from 5571f40 to 441e46c Compare June 30, 2023 05:22
@NordicBuilder
Copy link
Contributor

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.

@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 441e46c to 4291761 Compare June 30, 2023 19:01
samples/nrf9160/nrf_cloud_coap_client/Kconfig Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of this is copied from the REST sample.
as a separate ticket, can we make a "FOTA helpers" module for the shared stuff?
it might make sense for it to live in the samples folder.
regardless, it would be better to not have to maintain duplicate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did this to expedite testing of FOTA without getting distracted with a refactor.

I agree it should be reusable. I think it should live as a helper module within the nrfcloud library folder.

samples/nrf9160/nrf_cloud_coap_client/src/main.c Outdated Show resolved Hide resolved
samples/nrf9160/nrf_cloud_coap_client/src/main.c Outdated Show resolved Hide resolved
samples/nrf9160/nrf_cloud_coap_client/src/main.c Outdated Show resolved Hide resolved
samples/nrf9160/nrf_cloud_coap_client/src/handle_fota.c Outdated Show resolved Hide resolved
LOG_ERR("A-GPS data processing failed, error: %d", err);
} else {
LOG_INF("A-GPS data processed");
got_agps = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is never set back to false, so A-GPS is only done once in the sample's lifetime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have not decided whether I should implement the full set of GNSS interactions with the modem, including handling the assistance needed event, or just wait for the location library update.

samples/nrf9160/nrf_cloud_coap_client/src/main.c Outdated Show resolved Hide resolved
samples/nrf9160/nrf_cloud_coap_client/src/main.c Outdated Show resolved Hide resolved
samples/nrf9160/nrf_cloud_coap_client/src/main.c Outdated Show resolved Hide resolved
@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch 7 times, most recently from 14c7ba4 to 7996a1c Compare July 6, 2023 18:03
Comment on lines 7 to 13
/* Use high drive mode on SPI pins that handle communication with nRF7002
* so that SCK frequency > 4 MHz can be used. */
&spi3_default {
group1 {
nordic,drive-mode = <NRF_DRIVE_H0H1>;
};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be needed once cb74894 merges?

Copy link
Contributor

@glarsennordic glarsennordic Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit I referenced there was from #11559 -- I think rebase has destroyed the linking

It looks like it was merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I have no idea what your suggestion means. What are you thinking I should change?

Comment on lines 91 to 107
if (strncmp(key, FOTA_SETTINGS_KEY_PENDING_JOB, strlen(FOTA_SETTINGS_KEY_PENDING_JOB))) {
return -ENOMSG;
}

if (len_rd > sizeof(pending_job)) {
LOG_INF("FOTA settings size larger than expected");
len_rd = sizeof(pending_job);
}

sz = read_cb(cb_arg, (void *)&pending_job, len_rd);
if (sz == 0) {
LOG_DBG("FOTA settings key-value pair has been deleted");
return -EIDRM;
} else if (sz < 0) {
LOG_ERR("FOTA settings read error: %d", sz);
return -EIO;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments on why each of these functions is called would be helpful I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reused all this file from code inside of the nrf_cloud_rest_fota sample. I didn't write it. Justin made a comment in this PR that I should make it a common module between these samples. I suggested it should live as a helper module in the nrf_cloud library folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a definite plan to incorporate this under a helper library then it is less important that the lines be documented in this PR, makes sense to me

SETTINGS_STATIC_HANDLER_DEFINE(coap_fota, FOTA_SETTINGS_NAME, NULL,
coap_fota_settings_set, NULL, NULL);

static int coap_fota_settings_set(const char *key, size_t len_rd, settings_read_cb read_cb,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments indicating overall purpose of these static functions would be nice as well

}

lte_lc_register_handler(lte_handler);
#if defined(CONFIG_LTE_LINK_CONTROL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to keep this, consider gating the whole function behind a modem availability check and skipping the LTE_LC check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what your suggestion is.

Copy link
Contributor

@glarsennordic glarsennordic Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the CONFIG_LTE_LINK_CONTROL guard on just the section that uses lte_lc, you could guard the entire function for modem being enabled (since it doesn't seem to me like modem will be enabled in this sample without lte_lc being enabled)

It's a minor thing, though. CONFIG_LTE_LINK_CONTROL is arguably more specific, but I do suspect you'll eventually need to guard for modem enablement anyways

Alternatively, if you're gonna assume modem is always enabled, you could probably also assume the same of lte_lc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This originally came from the sdk-nrf coap_client sample.

Copy link
Contributor

@glarsennordic glarsennordic Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, interesting. I'm not sure why they would support modem builds both with and without lte_lc

int err;
int i = 1;

LOG_INF("\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to keep this LOG_INF("\n"); in the final version, or is this a temporary thing?

Comment on lines 417 to 440
static void get_cell_info(void)
{
int err;

if (!connected || !request_cells) {
return;
}

struct lte_lc_ncellmeas_params ncellmeas_params = {
.search_type = search_type,
.gci_count = ARRAY_SIZE(gci_cells)
};

/* Set the result buffers */
cell_info.neighbor_cells = neighbor_cells;
cell_info.gci_cells = gci_cells;

LOG_INF("Requesting neighbor cell measurement");
err = lte_lc_neighbor_cell_measurement(&ncellmeas_params);
if (err) {
LOG_ERR("Failed to start neighbor cell measurement, error: %d", err);
} else {
request_cells = false;
LOG_INF("Waiting for measurement results...");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a scan_cells.c to match with scan_wifi.c would make sense?

}

#if defined(CONFIG_NRF_CLOUD_PGPS)
static int do_pgps(struct gps_pgps_request *pgps_req)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't switch to using location lib, consider giving agps/pgps their own file

}

printk("\n***********************************************\n");
switch (cur_test) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're gonna redo all this, but in case you don't, you could avoid using the coroutine pattern if you make these steps execute synchronously in the main loop and do the button checking, etc, in a function that gets called between each step

After programming the sample and all prerequisites to the development kit, test it by performing the following steps:

1. Connect your nRF9160 DK to the PC using a USB cable and power on or reset your nRF9160 DK.
#. Open a terminal emulator and observe that the following information is displayed::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the double colon intentional?

@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 7996a1c to 55cf61e Compare July 7, 2023 22:15
Define strings related to encoding a device message
in JSON.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 55cf61e to 70cad91 Compare July 18, 2023 01:41
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jul 18, 2023
@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 70cad91 to 531b669 Compare July 18, 2023 19:34
plskeggs and others added 12 commits July 18, 2023 18:05
Add a general purpose JSON message encoder,
nrf_cloud_encode_message().

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
When encoding the shadow, let the caller determine if
the reported object should be included.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Split out update function so it can be used for CoAP.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
This was incorrectly using the ++ operator when atomic_inc()
would be better.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Modem sec tags are unsigned, so use that representation
instead of plain int.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Uses the Zephyr coap_client library. Sends and receives
data with nRF Cloud using CBOR and sometimes JSON.

Supports most nRF Cloud services - FOTA, data messaging,
location services (cellular, Wi-Fi, A-GPS, P-GPS).

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Document the new nrf_cloud_coap library.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Co-authored-by: Pekka Niskanen <pekka.niskanen@nordicsemi.no>
Implemented support for A-GPS and cellular positioning
using nRF Cloud CoAP.

Signed-off-by: Tommi Kangas <tommi.kangas@nordicsemi.no>
Added support for connecting to nRF Cloud using CoAP.

Signed-off-by: Tommi Kangas <tommi.kangas@nordicsemi.no>
Document a sample which uses nRF Cloud CoAP.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
This nRF Cloud CoAP client demonstrates using FOTA, messaging,
cellular positioning, A-GPS, and P-GPS over CoAP.

Get modem info IMEI and mfw. Init the modem, which waits for LTE
connection. Look up server IP. Init the client, including
looking up device IP with modem info.

Send real cell pos parameters, receive, decode, and display
location. Send cell pos back as fake GNSS PVT data.

Send fake temperature sensor data. Receive and process
MODEM and APP FOTA types.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
WIP.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
@plskeggs plskeggs force-pushed the feature-nrfcloud-coap-sample branch from 531b669 to 9b689fa Compare July 19, 2023 02:04
@plskeggs plskeggs closed this Jul 28, 2023
@plskeggs
Copy link
Contributor Author

Closed this when we decided to move towards a modified nrf_cloud_mqtt_multi_service sample for CoAP instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants