Memfault-SDK integration with MyNewt (Alternate) #2686

t3zeng · 2021-09-23T13:04:08Z

In this alternate PR I have utilized a slightly different strategy.

A new OS_COREDUMP_CB syscfg flag in conjunction with a new os feature to register a custom callback allow the memfault integration to register its own fault handling API.

vrahane · 2021-09-23T13:23:53Z

kernel/os/syscfg.yml

@@ -190,6 +195,9 @@ syscfg.defs:
            If set, run time is measured in cpu time ticks rather than OS time
            ticks.
        value: 0
+    MEMFAULT_ENABLE:
+        description: 'Enables Memfault SDK features'


This should be moved to util/memfault_sdk/syscfg.yml

util/memfault_sdk is currently only included inside kernel/os/pkg.yml if MEMFAULT_ENABLE is set. Putting MEMFAULT_ENABLE inside util/memfault_sdk/syscfg.yml would create a catch-22 circular dependency

We do not need the dependency there

Right, os no longer needs to pull in the package, I can move it to the app side. Changed

vrahane · 2021-09-23T13:27:23Z

I have already reviewed most of it. Please consider this approach based on our discussion on slack. @wes3 @mkiiskila @andrzej-kaczmarek @utzig

kernel/os/src/arch/cortex_m4/os_fault.c

kernel/os/src/arch/cortex_m33/os_fault.c

kernel/os/src/arch/cortex_m4/os_fault.c

andrzej-kaczmarek · 2021-09-23T20:12:16Z

Registering callback via API is useless for any crash that happens before registration. This should be done in the same way as os_assert_cb, i.e. just have prototype for that function and call it - some package in the project should provide definition.

Also we do not really need any code related to Memfault in mynewt-core for this to work - the package implementing os_coredump_cb callback can reside in Memfault repo since you need to include it anyway. So the way this would be used is: set OS_COREDUMP_CB: 1, include Memfault package from other repo, done.

t3zeng · 2021-09-23T21:10:35Z

Registering callback via API is useless for any crash that happens before registration. This should be done in the same way as os_assert_cb, i.e. just have prototype for that function and call it - some package in the project should provide definition.

Also we do not really need any code related to Memfault in mynewt-core for this to work - the package implementing os_coredump_cb callback can reside in Memfault repo since you need to include it anyway. So the way this would be used is: set OS_COREDUMP_CB: 1, include Memfault package from other repo, done.

I can see how replacing the registration process with a compile time function pointer to the callback is beneficial but I don't see the benefit of keeping the package itself out of mynewt. Isn't it easier for a mynewt user to utilize os_coredump_cb if all the options to set it up also existed in mynewt rather than needing to scour the internet for potential packages? It would also mean that a change that affects the usage of os_coredump_cb can easily be done for all packages that use this functionality

andrzej-kaczmarek · 2021-09-24T08:54:18Z

I don't really see good reason why we should include support for paid-for proprietary product in Mynewt tree given that it can be easily done out-of-tree. Also I don't like that proposed solution pulls Memfault repo via some other 3rd party repo, so the benefit is that in your own repo you can do integration in whatever way you like and no one cares. And the clean and simple solution to have full official integration would be to add callback on our side and then let Memfault make use of it in their repository.

vrahane · 2021-09-27T17:15:06Z

@andrzej-kaczmarek
I have a few thoughts of my own on this:

We should not restrict support for anything because its for profit/non-profit, that is really not what open source is all about. People should be free to use what they want. If the code is open, it is open source and in this case the SDK code is open. If somebody wants to write tools to utilize the SDK, that's their call. Mynewt is an open source RTOS and adds support and code for the OS and services in the OS, should not be caring about how it gets utilized on the other side.
We should also cater to changing needs of the demographic, if today people are using something, we need to make them aware that it is being supported. We do not have a dedicated marketing effort going on in mynewt. Don't you think we should do things that promote mynewt. Adding memfault as a utility does not really make us liable for support for it, that is maintained by the SDK. MCUmgr is our official device management protocol and will always remain. What we are doing here is showing that other device management protocol/services also work with mynewt, it's not just MCUmgr.

I do understand your point but it is a very narrow thought process and I do not agree with it.
Please try to understand the bigger picture. How do people get end -> end device management using mynewt ?

Either ways @t3zeng has gone ahead and done his part and opened up a new PR with what you want.
#2687

vrahane · 2021-09-27T20:04:48Z

I do like this approach:

I can see how replacing the registration process with a compile time function pointer to the callback is beneficial but I don't see the benefit of keeping the package itself out of mynewt. Isn't it easier for a mynewt user to utilize os_coredump_cb if all the options to set it up also existed in mynewt rather than needing to scour the internet for potential packages? It would also mean that a change that affects the usage of os_coredump_cb can easily be done for all packages that use this functionality

andrzej-kaczmarek · 2021-09-28T17:26:24Z

@vrahane
I said explicitly that I do not like idea of having that package in Mynewt tree. I do not have any issue with adding APIs to support it, thus proposed callback, and I do not want to restrict anyone from using Memfault or any other tool with Mynewt, not even sure how you came to such a ridiculous conclusion.

Once we add os_coredump_cb, adding required support to Memfault repository is trivial and imo it's the best way to do this: we do not need to maintain it and we have proper support that we can advertise e.g. on webpage. Otherwise, as proposed here, we have a package that requires a 3rd party repo that includes some fork of actual memfault-sdk repo... good luck with maintaining that.

t3zeng · 2021-09-28T17:54:37Z

@vrahane I said explicitly that I do not like idea of having that package in Mynewt tree. I do not have any issue with adding APIs to support it, thus proposed callback, and I do not want to restrict anyone from using Memfault or any other tool with Mynewt, not even sure how you came to such a ridiculous conclusion.

Once we add os_coredump_cb, adding required support to Memfault repository is trivial and imo it's the best way to do this: we do not need to maintain it and we have proper support that we can advertise e.g. on webpage. Otherwise, as proposed here, we have a package that requires a 3rd party repo that includes some fork of actual memfault-sdk repo... good luck with maintaining that.

There are some misconceptions here. First off, the purpose of the wrapper repo is to house the necessary mynewt repo yaml files. This is there because mynewt only allows for the repo files to be at the top level and it provides the power to choose when to update the commit hash in case things break.

Second, it currently includes some fork of the actual memfault-sdk right now because that fork contains changes that I am currently working on merging into the official memfault-sdk repo. Once that merge is complete, the wrapper repo will host the official memfault-sdk repo and not some fork.

Taking out the memfault package from mynewt tree does not eliminate the need for the wrapper repo because you still need to pull in the memfault repo to mynewt to use it. The only difference is in your proposed solution the coredump callback is defined in the memfault sdk, users need to use the internet to search for what callback functions are available for them to use, and any changes to the mechanism on mynewt side will break all packages that leverage the new coredump callback since they will all live outside of mynewt.

What removing the memfault package from mynewt-core doesn't change is the need to have a repo that has the yaml files needed for mynewt to recognize it as a repo that can be pulled in for use.

lance-proxy · 2021-09-29T06:53:38Z

util/memfault_sdk/src/memfault_sdk.c

+#if MYNEWT_VAL(MEMFAULT_ENABLE)
+#if MYNEWT_VAL(MEMFAULT_COREDUMP_CB)
+void os_coredump_cb(void *tf) {
+    memfault_fault_handler((sMfltRegState *)tf, kMfltRebootReason_HardFault);


This should also handle assertions, not just hardfaults I would think.

Assertions also call the fault handler so we will be able to capture core dumps in those scenarios

The reset reason set here will be wrong though?

andrzej-kaczmarek · 2021-09-29T15:41:10Z

There are some misconceptions here.

No, I know exactly why you "need" all those hacks to make it work. What I propose instead is doing this properly and add everything to Memfault repo and get rid of all those intermediate repos/packages/whatever. And that also includes repository.yml...

t3zeng · 2021-09-29T17:33:44Z

There are some misconceptions here.

No, I know exactly why you "need" all those hacks to make it work. What I propose instead is doing this properly and add everything to Memfault repo and get rid of all those intermediate repos/packages/whatever. And that also includes repository.yml...

Getting rid of the intermediate repo has no bearing to the changes in mynewt. All that changes is the path of the package that's included which is a very minor difference not worth fixating on in the context of this review.

Not including the package or any reference to this support on mynewt side simply seems strange since a user would not be able to easily know what is available to them if they want to use the feature. They would need to search on the internet which makes using mynewt less attractive to both hobbyists and companies alike in comparison to some other RTOS that does include support. Also, this os_coredump_cb functionality can't change in the future without breaking all of the different individual packages that live all across the internet because nobody can easily know what packages were supporting it and even if they did, they would need to make a PR to all of them or else they would just stop working forever.

I already made a PR that removes the memfault package so you're free to approve it if you support that strategy
#2687

utzig · 2021-09-29T17:46:57Z

Getting rid of the intermediate repo has no bearing to the changes in mynewt. All that changes is the path of the package that's included which is a very minor difference not worth fixating on in the context of this review.

Why would I want to pull in proxyco/mynewt-memfault-sdk instead of just using memfault/memfault-firmware-sdk directly? Why not PR the yaml files there? And btw, I think version.yml is not required anymore.

t3zeng · 2021-09-29T18:19:33Z

Getting rid of the intermediate repo has no bearing to the changes in mynewt. All that changes is the path of the package that's included which is a very minor difference not worth fixating on in the context of this review.

Why would I want to pull in proxyco/mynewt-memfault-sdk instead of just using memfault/memfault-firmware-sdk directly? Why not PR the yaml files there? And btw, I think version.yml is not required anymore.

Okay I can get rid of version.yml thanks. The wrapper repo is mainly because the memfault people didn't like that the yaml is required to be at the top level but it also acts as an anchor to commit ids that we know work well with mynewt

utzig · 2021-09-29T19:08:21Z

The wrapper repo is mainly because the memfault people didn't like that the yaml is required to be at the top level

Please link in the discussion.

t3zeng · 2021-09-29T21:35:12Z

The wrapper repo is mainly because the memfault people didn't like that the yaml is required to be at the top level

Please link in the discussion.

@chrisc11

t3zeng · 2021-10-06T21:16:55Z

The official memfault-firmware-sdk now has repository.yml at the top level for mynewt. I have adjusted paths to point directly to it

andrzej-kaczmarek · 2021-10-07T12:51:16Z

So just add remaining code from this PR to memfault repo and close this PR, we'll merge os_coredump_cb from the other PR.

chrisc11 · 2021-10-07T22:16:11Z

So just add remaining code from this PR to memfault repo and close this PR, we'll merge os_coredump_cb from the other PR.

hi @andrzej-kaczmarek, catching up on this thread, thanks for the feedback! We can definitely go ahead and pull the util/memfault_sdk part into the memfault-firmware-sdk directly.

However, I do see benefit in having a memfault porting layer directly in mynewt (similar to how tinyusb looks like it's integrated today). It would make it easier for an end user to get started, follow mynewt core API changes, and integrate directly into tools like mcumgr (there was an early prototype for that here). Let me know if you see a path forward for including the port directly to make things easier for end users in the future. thanks!

andrzej-kaczmarek · 2021-10-08T11:02:45Z

There's no need to add util/memfault_sdk package to your repository, you just need to add os_coredump_cb and os_assert_cb definitions to already existing ports/mynewt - it's that simple.

I checked few other ports available in Memfault repository and they seem to require some kind of patching in target OS (e.g. FreeRTOS, Zephyr, Dialog SDK) since there's no support provided by those OSes, so I do not understand why you guys push on adding code to Mynewt so much and claim that Mynewt will be "less attractive (...) in comparison to some other RTOS that does include support". In fact, in case of Mynewt all you have to do is to tweak app/target configuration, no need to apply any patches.

chrisc11 · 2021-10-12T14:20:48Z

There's no need to add util/memfault_sdk package to your repository, you just need to add os_coredump_cb and os_assert_cb definitions to already existing ports/mynewt - it's that simple.

I checked few other ports available in Memfault repository and they seem to require some kind of patching in target OS (e.g. FreeRTOS, Zephyr, Dialog SDK) since there's no support provided by those OSes, so I do not understand why you guys push on adding code to Mynewt so much and claim that Mynewt will be "less attractive (...) in comparison to some other RTOS that does include support". In fact, in case of Mynewt all you have to do is to tweak app/target configuration, no need to apply any patches.

Thanks for the feedback @andrzej-kaczmarek ... you are right that some of our ports do work this way. For newer ports such as the one with the nRF Connect SDK we've included a memfault module within the SDK for easier inclusion of SDK specific metric collection and build system addition. Both approaches work though so we have added a os_coredump_cb implementation into the memfault-firmware-sdk so only the changes from #2687 are needed now.

vrahane · 2021-10-18T15:56:26Z

@t3zeng We can close this one now.