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

modules: memfault: Enable user-provided heartbeat collect #12819

Merged
merged 1 commit into from
Feb 6, 2024
Merged

modules: memfault: Enable user-provided heartbeat collect #12819

merged 1 commit into from
Feb 6, 2024

Conversation

noahp
Copy link
Contributor

@noahp noahp commented Oct 21, 2023

Permit overriding the built-in NCS
memfault_metrics_heartbeat_collect_data() with a custom version.

Application will need to select
CONFIG_MEMFAULT_NCS_IMPLEMENT_METRICS_COLLECTION=n, then implement the function elsewhere.

Expose the memfault_ncs_metrics_collect_data() data collection function so a user can run that in addition to their own metrics.

@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 Oct 21, 2023
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Oct 21, 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.

@jtguggedal jtguggedal added this to the 2.6.0 milestone Nov 29, 2023
@jtguggedal jtguggedal added the CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. label Nov 29, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Nov 29, 2023 •
edited
Loading

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite

Detailed information of selected test modules

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

jtguggedal
jtguggedal reviewed Nov 29, 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.

Looks good, thanks for the patch!
Added a couple of suggestions in the Kconfig, nothing major

modules/memfault-firmware-sdk/Kconfig Outdated Show resolved Hide resolved
modules/memfault-firmware-sdk/Kconfig Outdated Show resolved Hide resolved
@github-actions github-actions bot added CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. and removed CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. labels Dec 20, 2023
@noahp
Copy link
Contributor Author

noahp commented Dec 20, 2023

Thank you @jtguggedal for the review! I've applied the changes, should be ready to go now.

@noahp noahp force-pushed the noahp/memfault-custom-metrics-collection branch from 738ceb4 to a804871 Compare December 20, 2023 17:22
@github-actions github-actions bot added CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. and removed CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. labels Dec 20, 2023
@noahp
modules: memfault: Enable user-provided heartbeat collect
00051d2 
Permit overriding the built-in NCS
`memfault_metrics_heartbeat_collect_data()` with a custom version.

Application will need to select
`CONFIG_MEMFAULT_NCS_IMPLEMENT_METRICS_COLLECTION=n`, then implement the
function elsewhere.

Expose the `memfault_ncs_metrics_collect_data()` data collection
function so a user can run that in addition to their own metrics.

Signed-off-by: Noah Pendleton <[email protected]>
@noahp noahp force-pushed the noahp/memfault-custom-metrics-collection branch from a804871 to 00051d2 Compare February 2, 2024 15:52
@github-actions github-actions bot added CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. and removed CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. labels Feb 2, 2024
noahp added a commit to memfault/memfault-asset-tracker that referenced this pull request Feb 2, 2024
@noahp
MFLT-12469 battery soc drop + connection time
4088dfe 
For Device Vitals, populate these metrics.

I had to pull in an extra commit from this in-flight PR:

nrfconnect/sdk-nrf#12819

I created a new branch `noahp/dogfood` off `gminn/v2.5.99-lte-metrics`
and rebased it on top of 7e318acebcc7d44faf9da13f2183f279c95055f2 of
upstream NCS main, and hacked and slashed until it worked, but it is NOT
clean, TODO need to update a new `dogfood` branch that contains the
correct combination of upstream and memfault changes.

Note that the charger-connect event is not wired up, but that should
still enable decent quality soc drop data.
@noahp noahp mentioned this pull request Feb 2, 2024
Merged
noahp added a commit to memfault/memfault-asset-tracker that referenced this pull request Feb 3, 2024
@noahp
MFLT-12469 battery soc drop + connection time
f103963 
For Device Vitals, populate these metrics.

I had to pull in an extra commit from this in-flight PR:

nrfconnect/sdk-nrf#12819

I created a new branch `noahp/dogfood` off `gminn/v2.5.99-lte-metrics`
and rebased it on top of 7e318acebcc7d44faf9da13f2183f279c95055f2 of
upstream NCS main, and hacked and slashed until it worked, but it is NOT
clean, TODO need to update a new `dogfood` branch that contains the
correct combination of upstream and memfault changes.

Note that the charger-connect event is not wired up, but that should
still enable decent quality soc drop data.
@jtguggedal jtguggedal added CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. and removed CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. labels Feb 6, 2024
@cvinayak cvinayak merged commit e2d89ee into nrfconnect:main Feb 6, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@jtguggedal jtguggedal jtguggedal approved these changes

@tejlmand tejlmand Awaiting requested review from tejlmand

@anangl anangl Awaiting requested review from anangl

@rlubos rlubos Awaiting requested review from rlubos

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. CI-trusted-author The author of the PR is trusted. All future commits will be tested in PR. external External contribution
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@noahp @NordicBuilder @jtguggedal @jhn-nordic @cvinayak