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

samples: nrfcloud_multi_service: Decrease heap usage #15625

Merged

Conversation

jorgenmk
Copy link
Contributor

Not enough RAM to build the coap + nrf_provisioning + modem_trace combination. Lowering heap usage to make space.

@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 May 31, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 31, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

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

Detailed information of selected test modules

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

@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.

@glarsennordic
Copy link
Contributor

glarsennordic commented May 31, 2024

AFAIK we were already quite close to using the entire system heap under certain circumstances

Did you test the following:

  1. What happens when the message queue becomes full? (Both on MQTT and on CoAP)
  2. What happens during PGPS requests? (Both on MQTT and on CoAP)

It may be necessary to reduce the message queue size in tandem with the system heap. And testing PGPS is a must

Also note that this change will be completely incompatible with Wi-Fi scanning and with auto-onboarding, although I think that is likely unavoidable

@plskeggs
Copy link
Contributor

AFAIK we were already quite close to using the entire system heap under certain circumstances

Did you test the following:

  1. What happens when the message queue becomes full? (Both on MQTT and on CoAP)
  2. What happens during PGPS requests? (Both on MQTT and on CoAP)

It may be necessary to reduce the message queue size in tandem with the system heap. And testing PGPS is a must

Also note that this change will be completely incompatible with Wi-Fi scanning and with auto-onboarding, although I think that is likely unavoidable

Perhaps we should reduce CONFIG_MAX_OUTGOING_MESSAGES if running out of heap is a problem.

PGPS allocates a single 4KB block of RAM upon initialization. It does not allocate any from the heap otherwise.

@@ -56,6 +56,20 @@ tests:
- nrf9160dk/nrf9160/ns
extra_args: EXTRA_CONF_FILE="overlay_coap.conf;overlay_min_coap.conf"
tags: ci_build sysbuild
sample.cellular.nrf_cloud_multi_service.coap.trace:
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to enable this.

@glarsennordic
Copy link
Contributor

glarsennordic commented May 31, 2024

Perhaps we should reduce CONFIG_MAX_OUTGOING_MESSAGES if running out of heap is a problem.

Yep, that's what I meant might be necessary

PGPS allocates a single 4KB block of RAM upon initialization. It does not allocate any from the heap otherwise.

Its possible I was simply mistaken when I wrote the comment Extended memory heap size needed both for PGPS and for encoding JSON-based nRF Cloud Device Messages.

Although hypothetically the memory use would be from MQTT messages sent/received in relation to PGPS.

It looks like our CI actually does cover specifically the PGPS thing, so I think we can just update that comment and move along.

But the device message queue size is still a concern.

@plskeggs
Copy link
Contributor

plskeggs commented Jun 3, 2024

Although hypothetically the memory use would be from MQTT messages sent/received in relation to PGPS.

PGPS MQTT messages are quite small -- some parameters to the server and a hostname and path from the server. The majority of the data is received using HTTPS.

@plskeggs
Copy link
Contributor

plskeggs commented Jun 3, 2024

@glarsennordic here are stats from running mss with the coap overlay -- it needed bytes of heap for 50 messages in the queue.

7936 - 4604 = 3332 consumed by 50 messages

[00:20:09.334,533] message_queue: enqueue_device_message: Messages queue status: 0/50
[00:20:09.334,564] message_queue: enqueue_device_message: Adding device message to queue
[00:20:09.334,594] application: kernel heap statistics:
[00:20:09.334,594] application: free: 14232
[00:20:09.334,594] application: allocated: 4604
[00:20:09.334,625] application: max. allocated: 6020
...
[01:12:09.336,090] message_queue: enqueue_device_message: Messages queue status: 50/50
[01:12:09.336,120] message_queue: Message queue is full
[01:12:09.336,120] message_queue: Cannot add message to queue
[01:12:09.336,181] application: kernel heap statistics:
[01:12:09.336,181] application: free: 10312
[01:12:09.336,181] application: allocated: 7936
[01:12:09.336,212] application: max. allocated: 8148

@glarsennordic
Copy link
Contributor

@plskeggs Thanks for checking that. Then I would say there should not be any problem with this heap allocation change

glarsennordic
glarsennordic previously approved these changes Jun 3, 2024
@glarsennordic glarsennordic self-requested a review June 3, 2024 18:50
@glarsennordic
Copy link
Contributor

Actually, my bad, that was specifically with CoAP. I think it is with MQTT where we are much more likely to see a memory overflow

@plskeggs
Copy link
Contributor

plskeggs commented Jun 3, 2024

OK, with MQTT, we run out of RAM at 44 messages in the queue. So, @jorgenmk, I recommend you reduce CONFIG_MAX_OUTGOING_MESSAGES to 25.

Not enough RAM to build the coap + nrf_provisioning + modem_trace
combination. Lowering heap usage to make space.

Lowering MAX_OUTGOING_MESSAGES to fit new heap.

Added twister build of trace configuration.

Signed-off-by: Jorgen Kvalvaag <[email protected]>
@jorgenmk jorgenmk force-pushed the nrfcloud-multi-service-trace-build branch from ce50a06 to e9357a0 Compare June 4, 2024 07:08
Copy link
Contributor

@glarsennordic glarsennordic left a comment

Choose a reason for hiding this comment

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

LGTM

@glarsennordic glarsennordic self-requested a review June 4, 2024 18:22
@jorgenmk jorgenmk removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 5, 2024
@rlubos rlubos merged commit fcc1aca into nrfconnect:main Jun 5, 2024
15 checks passed
@jorgenmk jorgenmk deleted the nrfcloud-multi-service-trace-build branch June 6, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants