-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add Memfault integration #2286
Conversation
51c8da2
to
81b0a8a
Compare
6bbb4df
to
05611ff
Compare
05611ff
to
6be6ef8
Compare
RAT Report (2020-05-26 14:50:58)New files with unknown licenses
22 new files were excluded from check (.rat-excludes)Detailed analysisNew files in this PR |
Style check summaryOur 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; |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non existing package
Depends on these pull requests: |
@michal-narajowski @andrzej-kaczmarek Perhaps we can close this out too since we have decided a path forward now ? |
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 |
https://memfault.com/