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.

Sign up for GitHub

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

Jump to bottom

samples: mmss: fix connection initialization order #8689

Merged
merged 1 commit into from
Sep 12, 2022
Merged

samples: mmss: fix connection initialization order #8689

merged 1 commit into from
Sep 12, 2022

Conversation

glarsennordic
Copy link
Contributor

Slightly alter initialization procedure in connection.c to make sure delta firmware FOTA succeeds

IRIS4777

Signed-off-by: Georges Oates_Larsen georges.larsen@nordicsemi.no

@glarsennordic glarsennordic added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 9, 2022
@glarsennordic glarsennordic added this to the 2.1.0 milestone Sep 9, 2022
@github-actions github-actions bot removed the doc-required PR must not be merged without tech writer approval. label Sep 9, 2022
plskeggs
plskeggs approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@plskeggs plskeggs left a comment

Choose a reason for hiding this comment

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

Generally fine.

Commit message should be:
Jira: IRIS-4777
or
IRIS-4777

not IRIS4777

@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 9, 2022
@glarsennordic glarsennordic added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. and removed doc-required PR must not be merged without tech writer approval. labels Sep 9, 2022
@glarsennordic glarsennordic requested a review from peknis September 9, 2022 01: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.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Sep 9, 2022 •
edited
Loading

Integration test specification

Test Module File based changes Manually selected West overwrite

Test spec set is empty, this is probabley due to no matching entry in .github/test-spec.yml

Detailed information of selected test modules

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

@jayteemo
Copy link
Contributor

jayteemo commented Sep 9, 2022

Adding DNM. I’ll add my review comments tomorrow.

@jayteemo jayteemo added the DNM label Sep 9, 2022
jayteemo
jayteemo reviewed Sep 9, 2022
View reviewed changes
samples/nrf9160/nrf_cloud_mqtt_multi_service/src/connection.c
@@ -642,7 +642,7 @@ static int connect_cloud(void)
*
* @return int - 0 on success, otherwise a negative error code.
*/
static int setup_lte(void)
static int setup_modem(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

if FMFU is enabled and a delta update is performed, the code below will print out an invalid error message.
i think changing the check to if (err < 0) should be sufficient.
For reference, see

sdk-nrf/applications/asset_tracker_v2/src/main.c

Line 235 in 052c17e

static void handle_nrf_modem_lib_init_ret(void)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch -- I've decided to add debug logging for all three possibilities

@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 9, 2022
@glarsennordic glarsennordic force-pushed the IRIS4777 branch 3 times, most recently from 9f68c97 to e8eb006 Compare September 9, 2022 19:13
@jayteemo jayteemo removed the DNM label Sep 9, 2022
@glarsennordic glarsennordic removed the DNM label Sep 10, 2022
peknis
peknis previously requested changes Sep 12, 2022
View reviewed changes
doc/nrf/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved
@wiba-nordic
Copy link
Contributor

The changelog entry should be added to PR#8510 as a comment at this point to prevent merge conflicts.

@greg-fer greg-fer mentioned this pull request Sep 12, 2022
Merged
14 tasks
@glarsennordic @greg-fer
samples: mmss: fix connection initialization order
c2aa2cb 
Slightly alter initialization procedure in connection.c to make sure
delta firmware FOTA succeeds

IRIS-4777

Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>
@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. and removed doc-required PR must not be merged without tech writer approval. labels Sep 12, 2022
@greg-fer greg-fer dismissed peknis’s stale review September 12, 2022 11:01

Changelog entry will be added in the release notes PR directly, with the applied edit.

@wiba-nordic wiba-nordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Sep 12, 2022
@rlubos rlubos merged commit 86f1643 into nrfconnect:main Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayteemo jayteemo jayteemo approved these changes

@plskeggs plskeggs plskeggs approved these changes

@peknis peknis peknis left review comments

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@glarsennordic @NordicBuilder @jayteemo @wiba-nordic @plskeggs @peknis @rlubos