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

Add Memfault integration #2286

Closed
wants to merge 2 commits into from

Conversation

michal-narajowski
Copy link

@michal-narajowski michal-narajowski force-pushed the memfault branch 5 times, most recently from 51c8da2 to 81b0a8a Compare May 11, 2020 11:55
@michal-narajowski michal-narajowski force-pushed the memfault branch 4 times, most recently from 6bbb4df to 05611ff Compare May 25, 2020 11:50
@michal-narajowski michal-narajowski changed the title [WIP] Add Memfault integration Add Memfault integration May 25, 2020
@michal-narajowski michal-narajowski marked this pull request as ready for review May 25, 2020 13:40
@apache-mynewt-bot
Copy link

Style check summary

Our coding style is here!

sys/memfault/src/memfault_platform_core.c

@@ -42,7 +42,7 @@
  * }
  */
 static uint8_t s_reboot_tracking[MEMFAULT_REBOOT_TRACKING_REGION_SIZE]
-    __attribute__((section(".mflt_reboot_info")));
+__attribute__((section(".mflt_reboot_info")));
 
 static struct os_callout metrics_callout;
 static uint32_t metrics_period_sec;

Copy link
Contributor

@kasjer kasjer left a comment

Choose a reason for hiding this comment

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

There are pieces missing to do the review.

/* Note: MCU reset reason register bits are usually "sticky" and
* need to be cleared.
*/
NRF_POWER->RESETREAS |= NRF_POWER->RESETREAS;
Copy link
Contributor

Choose a reason for hiding this comment

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

On memfault page they claim that not only Nordic is supported. Is our implementation is limited to NRF?

uint64_t
memfault_platform_get_time_since_boot_ms(void)
{
return (uint64_t) (os_get_uptime_usec() / 1000);
Copy link
Contributor

@kasjer kasjer May 30, 2020

Choose a reason for hiding this comment

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

cast here seems superfluous

metrics_callout_cb, NULL);
SYSINIT_PANIC_ASSERT(metrics_callout.c_evq != NULL);

const sResetBootupInfo reset_reason = {
Copy link
Contributor

Choose a reason for hiding this comment

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

variables are declared all over the place in this function

- "@mcuboot/boot/bootutil"

pkg.deps.MEMFAULT_MGMT:
- "@apache-mynewt-mcumgr/cmd/memfault_mgmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

non existing package

@michal-narajowski
Copy link
Author

Depends on these pull requests:
apache/mynewt-mcumgr#84
apache/mynewt-newtmgr#165

@vrahane
Copy link
Contributor

vrahane commented Oct 18, 2021

@michal-narajowski @andrzej-kaczmarek Perhaps we can close this out too since we have decided a path forward now ?

@michal-narajowski
Copy link
Author

Closing since this feature has been added to memfault repo as a newt package: https://github.com/memfault/memfault-firmware-sdk/tree/master/ports/mynewt

@michal-narajowski michal-narajowski deleted the memfault branch March 30, 2022 12:53
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.

4 participants