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

net: lib: nrf_cloud: make MQTT client ID prefix configurable #2961

Merged
merged 2 commits into from
Oct 2, 2020
Merged

net: lib: nrf_cloud: make MQTT client ID prefix configurable #2961

merged 2 commits into from
Oct 2, 2020

Conversation

coderbyheart
Copy link
Contributor

The nrf- prefix is reserved for Nordic devices: https://api.nrfcloud.com/v1#operation/CreateDeviceCertificate:

If you want to create a certificate for a non-Nordic device, any deviceId is sufficient that does not start with nrf- (we recommend using a GUID).

This change allows customers to use the asset_tracker application with nRF Connect for Cloud with their own SIPs without modifying the source. This is especially relevant for customers that are building development kit like products, and allows them to work out of the box with the stock asset_tracker and nRF Connect for Cloud.

@coderbyheart
Copy link
Contributor Author

Recreated from fork by request from @carlescufi

@coderbyheart coderbyheart changed the title net: lib: nrf_cloud: make MQTT client ID prefix configurable #2958 net: lib: nrf_cloud: make MQTT client ID prefix configurable Sep 22, 2020
Copy link
Contributor

@sigvartmh sigvartmh left a comment

Choose a reason for hiding this comment

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

Without changes suggested in the PR you'll get compile warnings/errors for different configurations

main.c:391:24: warning: '%s' directive output may be truncated writing up to 18 bytes into a region of size 16 [-Wformat-truncation=]

etc.

samples/nrf9160/aws_fota/src/main.c Show resolved Hide resolved
samples/nrf9160/aws_fota/src/main.c Outdated Show resolved Hide resolved
samples/nrf9160/aws_fota/src/main.c Outdated Show resolved Hide resolved
@jaredwolff
Copy link

Perfect timing for this merge request. ❤️❤️❤️

samples/nrf9160/aws_fota/src/main.c Outdated Show resolved Hide resolved
subsys/net/lib/nrf_cloud/Kconfig Outdated Show resolved Hide resolved
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.

Looks good

Copy link
Contributor

@sigvartmh sigvartmh left a comment

Choose a reason for hiding this comment

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

../src/main.c: In function 'main':
../src/main.c:392:24: warning: '__builtin_snprintf' output may be truncated before the last format character [-Wformat-truncation=]
  392 |  snprintf(id_buf, len, "%s%s", CONFIG_NRF_CLOUD_CLIENT_ID_PREFIX,
      |                        ^~~~~~
../src/main.c:392:29: note: format string is defined here
  392 |  snprintf(id_buf, len, "%s%s", CONFIG_NRF_CLOUD_CLIENT_ID_PREFIX,
      |                             ^
In file included from /opt/tools/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/stdio.h:800,
                 from ../src/main.c:8:
../src/main.c:392:2: note: '__builtin_snprintf' output between 5 and 20 bytes into a destination of size 19
  392 |  snprintf(id_buf, len, "%s%s", CONFIG_NRF_CLOUD_CLIENT_ID_PREFIX,
      |  ^~~~~~~~

Still compiler warnings on aws_fota, looks like an off by 1 error.

@coderbyheart
Copy link
Contributor Author

@sigvartmh what are you compiling? Why do I not see those from CI?

@sigvartmh
Copy link
Contributor

I'm using USE_NRF_CLOUD=y but I could be on an old version of the branch maybe.

Copy link
Contributor

@sigvartmh sigvartmh left a comment

Choose a reason for hiding this comment

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

LGTM now

samples/nrf9160/aws_fota/Kconfig Outdated Show resolved Hide resolved
samples/nrf9160/aws_fota/Kconfig Outdated Show resolved Hide resolved
samples/nrf9160/aws_fota/Kconfig Outdated Show resolved Hide resolved
@coderbyheart
Copy link
Contributor Author

CI/Jenkins/FULL_CI fails with OSError: [Errno 28] No space left on device

@coderbyheart
Copy link
Contributor Author

All tests green now.

@rlubos
Copy link
Contributor

rlubos commented Sep 30, 2020

@tejlmand Any other feedback? The PR is pending your ACK.

coderbyheart and others added 2 commits October 2, 2020 13:15
The $srctree environment variable is already set to point to the Zephyr
root, so no need to do

    source "$(ZEPHYR_BASE)/Kconfig.zephyr"

in samples. Just

    source "Kconfig.zephyr"

works.

Signed-off-by: Markus Tacker <Markus.Tacker@NordicSemi.no>
The `nrf-` prefix is reserved for Nordic devices:
https://api.nrfcloud.com/v1#operation/CreateDeviceCertificate:

    If you want to create a certificate for a non-Nordic device,
    any deviceId is sufficient that does not start with
    `nrf-` (we recommend using a GUID).

This change allows customers to use the `asset_tracker` application
with nRF Connect for Cloud with their own SIPs without modifying
the source.

This is especially relevant for customers that are building
development kit like products, and allows them to work out of the
box with the stock `asset_tracker` and nRF Connect for Cloud.

Signed-off-by: Markus Tacker <Markus.Tacker@NordicSemi.no>
Co-authored-by: Sigvart Hovland <Sigvart Hovland@NordicSemi.no>
@rlubos rlubos merged commit bb259bf into nrfconnect:master Oct 2, 2020
@coderbyheart coderbyheart deleted the nrfcloud-configurable-mqtt-client-id-prefix branch October 2, 2020 12:57
hakonfam pushed a commit to sigvartmh/fw-nrfconnect-nrf-1 that referenced this pull request Oct 21, 2020
Changes introduced in
nrfconnect#2349
got reverted by
nrfconnect#2961

This commit re-introduces those changes.

NCSDK-6750

Signed-off-by: Sigvart Hovland <sigvart.hovland@nordicsemi.no>
Signed-off-by: Haakon Oeye Amundsen <haakon.amundsen@nordicsemi.no>
tejlmand pushed a commit that referenced this pull request Oct 21, 2020
Changes introduced in
#2349
got reverted by
#2961

This commit re-introduces those changes.

NCSDK-6750

Signed-off-by: Sigvart Hovland <sigvart.hovland@nordicsemi.no>
Signed-off-by: Haakon Oeye Amundsen <haakon.amundsen@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
9 participants