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

net: lib: aws_iot: Overhaul #11507

Merged
merged 1 commit into from
Oct 27, 2023
Merged

net: lib: aws_iot: Overhaul #11507

merged 1 commit into from
Oct 27, 2023

Conversation

simensrostad
Copy link
Contributor

@simensrostad simensrostad commented Jun 13, 2023 •
edited
Loading

  • Use MQTT helper library to abstract MQTT socket handling.
  • Removed internal error code translations.
  • Removed preprocessor statements to increase readability.
  • Simplified run-time client ID and hostname configurations.
  • Added library tests.
  • Update documentation accordingly.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 13, 2023
@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.

@simensrostad simensrostad force-pushed the aws-iot-fixes branch 3 times, most recently from fb3839f to aa60a2c Compare August 7, 2023 17:51
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Aug 7, 2023
@simensrostad simensrostad force-pushed the aws-iot-fixes branch 6 times, most recently from f17c388 to e40d546 Compare August 9, 2023 13:46
@simensrostad simensrostad force-pushed the aws-iot-fixes branch 11 times, most recently from 86d7e26 to 86d87bb Compare August 22, 2023 12:01
@simensrostad simensrostad force-pushed the aws-iot-fixes branch 5 times, most recently from 800d6c3 to 0879f0e Compare August 25, 2023 14:58
@simensrostad simensrostad changed the title net: lib: aws_iot: Use MQTT helper library net: lib: aws_iot: Overhaul Aug 25, 2023
@simensrostad
Copy link
Contributor Author

@jtguggedal should be good to review. @balaji-nordic Nice if you can go over the tests.

jtguggedal
jtguggedal reviewed Sep 4, 2023
View reviewed changes
Copy link
Contributor

@jtguggedal jtguggedal left a comment

Choose a reason for hiding this comment

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

I have reviewed the non-.c files, will have to take those on a later pass

include/net/aws_iot.h Show resolved Hide resolved
include/net/aws_iot.h Outdated Show resolved Hide resolved
include/net/aws_iot.h Show resolved Hide resolved
include/net/aws_iot.h Outdated Show resolved Hide resolved
include/net/mqtt_helper.h Outdated Show resolved Hide resolved
jtguggedal
jtguggedal reviewed Sep 8, 2023
View reviewed changes
Copy link
Contributor

@jtguggedal jtguggedal left a comment

Choose a reason for hiding this comment

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

Gone over the library files now as well, looks very good!
Just curious, did you see if the flash and RAM sizes were affected by the changes here?

samples/net/aws_iot/src/main.c Show resolved Hide resolved
samples/net/aws_iot/src/main.c Outdated Show resolved Hide resolved
subsys/net/lib/aws_iot/src/aws_iot.c Show resolved Hide resolved
subsys/net/lib/aws_iot/src/aws_iot.c Outdated Show resolved Hide resolved
subsys/net/lib/aws_iot/src/aws_iot.c Outdated Show resolved Hide resolved
subsys/net/lib/aws_iot/src/aws_iot.c Outdated Show resolved Hide resolved
subsys/net/lib/aws_iot/src/aws_iot.c Outdated Show resolved Hide resolved
@simensrostad
Copy link
Contributor Author

Gone over the library files now as well, looks very good! Just curious, did you see if the flash and RAM sizes were affected by the changes here?

After:

Memory region         Used Size  Region Size  %age Used
           FLASH:      192212 B       448 KB     41.90%
             RAM:       60280 B     195224 B     30.88%
        IDT_LIST:          0 GB         2 KB      0.00%

Before:

Memory region         Used Size  Region Size  %age Used
           FLASH:      188556 B       448 KB     41.10%
             RAM:       62440 B     195224 B     31.98%
        IDT_LIST:          0 GB         2 KB      0.00%

divipillai
divipillai reviewed Sep 14, 2023
View reviewed changes
applications/asset_tracker_v2/doc/cloud_module.rst Show resolved Hide resolved
doc/nrf/libraries/networking/aws_iot.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/networking/aws_iot.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/networking/aws_iot.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/networking/aws_iot.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/networking/aws_iot.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/networking/aws_iot.rst Outdated Show resolved Hide resolved
@simensrostad
Copy link
Contributor Author

@divipillai Comments addressed, thanks.

@divipillai
Copy link
Contributor

@divipillai Comments addressed, thanks.

Looks good. Do we need a changelog entry for these changes?

rlubos
rlubos reviewed Oct 6, 2023
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Looks good, just some nit regarding API docs.

include/net/aws_iot.h Outdated Show resolved Hide resolved
@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 Oct 6, 2023
@simensrostad
Copy link
Contributor Author

@divipillai Comments addressed, thanks.

Looks good. Do we need a changelog entry for these changes?

Changelog entry added.

@simensrostad simensrostad force-pushed the aws-iot-fixes branch 4 times, most recently from 0d101d5 to a5d5266 Compare October 10, 2023 05:47
@simensrostad
net: lib: aws_iot: Overhaul
9c63646 
 - Use MQTT helper library to abstract MQTT socket handling.
 - Removed internal error code translations.
 - Removed preprocessor statements to increase readability.
 - Simplified run-time client ID and hostname configurations.
 - Added library tests.
 - Update documentation accordingly.

Signed-off-by: Simen S. Røstad <simen.rostad@nordicsemi.no>
@cvinayak cvinayak merged commit fc1ba49 into nrfconnect:main Oct 27, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divipillai divipillai divipillai approved these changes

@rlubos rlubos rlubos approved these changes

@jtguggedal jtguggedal jtguggedal approved these changes

@coderbyheart coderbyheart Awaiting requested review from coderbyheart coderbyheart is a code owner

@anangl anangl Awaiting requested review from anangl

@carlescufi carlescufi Awaiting requested review from carlescufi

@jhn-nordic jhn-nordic Awaiting requested review from jhn-nordic

@fnawratil fnawratil Awaiting requested review from fnawratil

@balaji-nordic balaji-nordic Awaiting requested review from balaji-nordic

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@simensrostad @NordicBuilder @divipillai @jtguggedal @rlubos @cvinayak