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: nrf9160: Add modem traces to flash sample #9364

Merged
merged 1 commit into from Nov 24, 2022
Merged

samples: nrf9160: Add modem traces to flash sample #9364

merged 1 commit into from Nov 24, 2022

Conversation

gregersrygg
Copy link
Contributor

Sample that demonstrates how to create a modem trace backend that writes the modem traces to the external flash on the nRF9160 DK, and how to read it out again.

Signed-off-by: Gregers Gram Rygg gregers.gram.rygg@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. doc-required PR must not be merged without tech writer approval. labels Nov 21, 2022
@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.

@gregersrygg gregersrygg force-pushed the add-modem-trace-flash-sample-stream branch 7 times, most recently from 67320da to 58c7553 Compare November 22, 2022 13:34
@gregersrygg gregersrygg marked this pull request as ready for review November 22, 2022 13:34
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Nov 22, 2022 •
edited

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
desktop52_verification X
test-ci-nrfconnect-boot-fw-update X
test-fw-nrfconnect-apps X
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_mesh X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-nfc X
test-fw-nrfconnect-nrf-iot_libmodem X
test-fw-nrfconnect-nrf-iot_lwm2m X
test-fw-nrfconnect-nrf-iot_mosh X
test-fw-nrfconnect-nrf-iot_positioning X
test-fw-nrfconnect-nrf-iot_samples X
test-fw-nrfconnect-nrf-iot_thingy91 X
test-fw-nrfconnect-nrf-iot_zephyr_lwm2m X
test-fw-nrfconnect-nrf_crypto X
test-fw-nrfconnect-proprietary_esb X
test-fw-nrfconnect-rpc X
test-fw-nrfconnect-rs X
test-fw-nrfconnect-tfm X
test-fw-nrfconnect-thread X
test-fw-nrfconnect-zigbee X
test-sdk-audio X
test-sdk-find-my X
test-sdk-homekit X
test-sdk-wifi X

All integration tests: null

Detailed information of selected test modules

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

lemrey
lemrey reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@lemrey lemrey left a comment

Choose a reason for hiding this comment

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

If it this is intended to be used by customers, the trace backed should be put in nrf_modem_lib/trace_backends instead of the sample's folder.

@gregersrygg gregersrygg force-pushed the add-modem-trace-flash-sample-stream branch from 58c7553 to a86591c Compare November 22, 2022 13:52
@gregersrygg
Copy link
Contributor Author

If it this is intended to be used by customers, the trace backed should be put in nrf_modem_lib/trace_backends instead of the sample's folder.

@lemrey I agree that would make sense, but I think it needs further refining before it can be a generic backend in nrf_modem_lib/trace_backends. The sample is meant as a starting point for customers to copy into their applications and adjust to their needs.

@gregersrygg gregersrygg force-pushed the add-modem-trace-flash-sample-stream branch from a86591c to 67827fe Compare November 22, 2022 15:15
balaji-nordic
balaji-nordic reviewed Nov 22, 2022
View reviewed changes
samples/nrf9160/modem_trace_flash/src/main.c Outdated

static void print_traces(void)
{
uint8_t buf[1024];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint8_t buf[1024];
uint8_t buf[READ_BUF_SIZE];

And define READ_BUF_SIZE to 1024 at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just use a const inside the function? It's only needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to clearly explain why a magic number was chosen. A macro in place of a magic number becomes a self-explanatory way of doing that.

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 agree to avoid the magic number, but I don't see the reason to use a define that would be file global instead of a const that would only be visible inside the function scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah! sorry I misunderstood. A const works too! :)

balaji-nordic
balaji-nordic requested changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@balaji-nordic balaji-nordic left a comment •
edited

Choose a reason for hiding this comment

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

Some cleanup remains. See comments

hakonfam
hakonfam approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@hakonfam hakonfam left a comment

Choose a reason for hiding this comment

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

Partition manager changes looks OK

samples/nrf9160/modem_trace_flash/Kconfig.trace_flash Show resolved Hide resolved
subsys/partition_manager/pm.yml.modem_trace Show resolved Hide resolved
lemrey
lemrey reviewed Nov 23, 2022
View reviewed changes
samples/nrf9160/modem_trace_flash/src/trace_flash.c Outdated
return (int)len;
}

int trace_flash_flush(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is not part of the trace_backend.h interface, it's a bit confusing that it is here, but exported by trace_flash.h. Same for trace_read.h

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest moving it to a separate C file

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 have moved the flash handling logic to trace_storage.h and trace_storage.c, and the trace backend logic to trace_flash_backend.c (which calls trace_storage_write to write trace data to flash).

samples/nrf9160/modem_trace_flash/src/trace_flash.c Outdated
Comment on lines 164 to 165
LOG_DBG("Written %d bytes in %lld ms", traces_size, tot_write_time);
LOG_DBG("Throughput = %lld kb/s ", (traces_size * 8 / tot_write_time));
Copy link
Contributor

Choose a reason for hiding this comment

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

We have some built-in facility for this now (in nrf_modem_lib_trace.c), both for trace backend throughput and trace throughput from modem. You could see if that works for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I wasn't aware of that. Tried it now, but I prefer the average speed for all trace writes instead of printing periodically. Perhaps possible to configure, but have to look into that later.

@jhn-nordic
Copy link
Contributor

You need to add a entry in https://github.com/nrfconnect/sdk-nrf/blob/main/doc/nrf/releases/release-notes-changelog.rst

@jhn-nordic jhn-nordic added this to the 2.2.0 milestone Nov 23, 2022
@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 Nov 23, 2022
balaji-nordic
balaji-nordic requested changes Nov 23, 2022
View reviewed changes
samples/nrf9160/modem_trace_flash/src/main.c Outdated

static void print_traces(void)
{
uint8_t buf[1024];
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to clearly explain why a magic number was chosen. A macro in place of a magic number becomes a self-explanatory way of doing that.

@gregersrygg gregersrygg force-pushed the add-modem-trace-flash-sample-stream branch 3 times, most recently from 04b4419 to 4dd3082 Compare November 23, 2022 23:20
lemrey
lemrey approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@lemrey lemrey left a comment

Choose a reason for hiding this comment

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

Thanks

@gregersrygg gregersrygg force-pushed the add-modem-trace-flash-sample-stream branch from 7871f65 to 52007cb Compare November 24, 2022 09:39
@gregersrygg @balaji-nordic @divipillai
samples: nrf9160: Add modem traces to flash sample
76ff415 
Sample that demonstrates how to create a modem trace backend that writes
the modem traces to the external flash on the nRF9160 DK, and how to
read it out again.

Signed-off-by: Gregers Gram Rygg <gregers.gram.rygg@nordicsemi.no>
Co-Authored-By: Balaji Srinivasan <balaji.srinivasan@nordicsemi.no>
Co-Authored-By: divya pillai <divya.pillai@nordicsemi.no>
@gregersrygg gregersrygg force-pushed the add-modem-trace-flash-sample-stream branch from 52007cb to 76ff415 Compare November 24, 2022 09:40
@gregersrygg gregersrygg self-assigned this Nov 24, 2022
@rlubos rlubos merged commit 25a31a0 into nrfconnect:main Nov 24, 2022
@gregersrygg gregersrygg deleted the add-modem-trace-flash-sample-stream branch November 24, 2022 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemrey lemrey lemrey approved these changes

@hakonfam hakonfam hakonfam approved these changes

@jhn-nordic jhn-nordic jhn-nordic approved these changes

@balaji-nordic balaji-nordic balaji-nordic approved these changes

@divipillai divipillai divipillai approved these changes

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

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

Assignees

@gregersrygg gregersrygg

Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
2.2.0
8 participants
@gregersrygg @NordicBuilder @jhn-nordic @lemrey @hakonfam @balaji-nordic @divipillai @rlubos