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

lib: nrf_modem: trace: fixes for uart trace backend++ #11708

Merged
merged 3 commits into from
Jul 17, 2023
Merged

lib: nrf_modem: trace: fixes for uart trace backend++ #11708

merged 3 commits into from
Jul 17, 2023

Conversation

eivindj-nordic
Copy link
Contributor

@eivindj-nordic eivindj-nordic commented Jul 5, 2023 •
edited
Loading

If the UART trace backend has flow control enabled, the trace backend might abort the current send, hence 0 bytes will be written. Return an error in this case to not assert on the trace backend returning 0 bytes.

@eivindj-nordic eivindj-nordic self-assigned this Jul 5, 2023
@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 Jul 5, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jul 5, 2023 •
edited
Loading

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-nrf-iot_lwm2m X
test-fw-nrfconnect-nrf-iot_mosh X
test-fw-nrfconnect-nrf-iot_samples X
test-fw-nrfconnect-nrf-iot_serial_lte_modem X
test-fw-nrfconnect-nrf-iot_zephyr_lwm2m X

Detailed information of selected test modules

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

@lemrey
Copy link
Contributor

lemrey commented Jul 5, 2023

Isn't that an issue with how we define the trace_backend_write interface?
Why do we allow that to return 0? Is it necessary to differentiate between 0 and errors?

@eivindj-nordic
Copy link
Contributor Author

eivindj-nordic commented Jul 5, 2023 •
edited
Loading

Isn't that an issue with how we define the trace_backend_write interface? Why do we allow that to return 0? Is it necessary to differentiate between 0 and errors?

It depends. The alternative is to let the trace backend loop forever in this case instead, and log the warning there. But it will happen in some cases that the uart aborts due to flow control, where we do not want to assert.

@eivindj-nordic eivindj-nordic changed the title lib: nrf_modem: trace: allow trace backend to write 0 bytes lib: nrf_modem: trace: uart: return ETIMEDOUT when 0 bytes written Jul 6, 2023
@eivindj-nordic
Copy link
Contributor Author

Updated to have the uart trace backend to return an error in case writing 0 bytes. This will prevent the assert and shutdown the trace processing. If the uart trace backend is not able to send traces for 5 seconds, then the traces are probably lacking anyway (the modem will drop traces).

@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Jul 6, 2023
@eivindj-nordic eivindj-nordic changed the title lib: nrf_modem: trace: uart: return ETIMEDOUT when 0 bytes written lib: nrf_modem: trace: fixes for uart trace backend++ Jul 6, 2023
@eivindj-nordic eivindj-nordic force-pushed the modem_trace_write_fix branch 3 times, most recently from f5e4d4d to 703e195 Compare July 6, 2023 13:52
@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.

@eivindj-nordic eivindj-nordic force-pushed the modem_trace_write_fix branch 3 times, most recently from a2e4358 to 15edde8 Compare July 7, 2023 07:10
divipillai
divipillai approved these changes Jul 7, 2023
View reviewed changes
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated
Comment on lines 384 to 414
* Added CEREG event tracking to ``lte_connectivity``.

Updated:

* The :c:func:`nrf_modem_lib_shutdown` function to allow the modem to be in flight mode (``CFUN=4``) when shutting down the modem.
* The trace backends can now return ``-EAGAIN`` if the write operation can be retried.
* The trace backends ``deinit`` function is expected to succeed and can no longer return an error.
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
* Added CEREG event tracking to ``lte_connectivity``.
Updated:
* The :c:func:`nrf_modem_lib_shutdown` function to allow the modem to be in flight mode (``CFUN=4``) when shutting down the modem.
* The trace backends can now return ``-EAGAIN`` if the write operation can be retried.
* The trace backends ``deinit`` function is expected to succeed and can no longer return an error.
* Added CEREG event tracking to ``lte_connectivity``.
* Updated:
* The :c:func:`nrf_modem_lib_shutdown` function to allow the modem to be in flight mode (``CFUN=4``) when shutting down the modem.
* The trace backends can now return ``-EAGAIN`` if the write operation can be retried.
* The trace backends ``deinit`` function is expected to succeed and can no longer return an error.

nit

@eivindj-nordic
snippets: nrf91-modem-trace-uart: add UART1 specific Kconfigs
83b5c96 
Enable ASYNC UART for the UART1 instance.

Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
@eivindj-nordic
lib: nrf_modem: trace: allow trace backend to return EAGAIN on retry
f13a845 
Allow the trace backend to return -EAGAIN if the operation should be
retried. This is advantageous compared to the backend waiting forever
as we need to shut down the tracing if the modem is shut down.

Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
@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 Jul 14, 2023
lemrey
lemrey reviewed Jul 17, 2023
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.

One small comment otherwise looks good

include/modem/trace_backend.h Outdated
*/
int (*deinit)(void);
void (*deinit)(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's best to keep it returning an int, even if current implementations can't fail. That is still a possibility if they change or new ones are added; by removing the return value and the log it's impossible to tell if this operation failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@eivindj-nordic
lib: nrf_modem: trace: cancel work items on deinit
ee2c781 
Cancel work items on deinit.

Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
@rlubos rlubos merged commit 2fc84af into nrfconnect:main Jul 17, 2023
16 checks passed
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

@nordic-mare nordic-mare nordic-mare approved these changes

@divipillai divipillai divipillai approved these changes

@MirkoCovizzi MirkoCovizzi Awaiting requested review from MirkoCovizzi

@rlubos rlubos Awaiting requested review from rlubos

@anangl anangl Awaiting requested review from anangl

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees

@eivindj-nordic eivindj-nordic

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
@eivindj-nordic @NordicBuilder @lemrey @nordic-mare @divipillai @rlubos