BLE Eats too much RAM... #390
Comments
|
Comparing 2.0.0 with 2.1.0, the low level BLE code hasn't changed, has it? |
|
I created local makecode v0 builds for v2.1.0 and v2.0.0 and added debugging to the hexes. For v2.0.0 I checked out the pxt-microbit commit before "Move to dal integration branch for testing #1154". Having created a new hex from each based on the gamepad-demo project, v2.0.0 ran OK, v2.1.0 OOMed. Comparing the debug output, I could match most of the allocations between the 2 versions. Of the unmatched ones, all except one of 24 bytes were in the v2.1.0 output and of those, the ones that weren't freed totalled 1717 bytes and appeared to occur probably during uBit:init. Up to the point where v2.1.0 OOMed, it seemed v2.1.0 allocated some 600-700 bytes more than v2.0.0 (out of the 1717). After that point, v2.0.0 allocated another 380 bytes (mostly native:128,20,40,192) then freed 280 bytes (mostly non-native:10*24 + 40). The top block on the native heap ended at 0x20003BA0. |
|
A v2.1.0 build without the partial flashing service allocates 192 bytes less. The remaining sizes match but these are missing: 20, 84, 40, 24, 24. Should PF be optional? An option would be even more useful if MakeCode could adjust the GATT table size. This version only fails to get the final 192 bytes, but that would still leave it some 500 bytes short of the v2.0.0 build. |
|
v2.1.0 uses 360 bytes more RAM than 2.0.0: partial flashing 212, mag + accel 140, others 8. I thought of a better way to investigate this, involving less bad guesswork! Starting with a sample built on 2.0.0, I added the new heap code to get a baseline, then added the other changes and finally enabled the partial flashing service. Adding the changes allocated an extra 204 bytes which used 220 bytes of the heap. Enabling partial flashing allocated an extra 192 bytes which used 212 bytes of the heap. So 432 bytes more heap was used but, mostly because uBit now has references to the mag + accel heap blocks, the heap start ended up shifted down 72 bytes and the net effect on RAM usage was an increase of 360 bytes. Having added everything except the mag + accel changes, the only effect was to push the heap start up by 8 bytes. Adding the 3 new "pins" to MicroBitIO pushed the heap start up by 48 bytes and added 32 to the heap (in verify_stack_size, I think). When I added the rest of the mag + accel changes, that extra 32 bytes went away and the heap start shifted down. @finneyj Does this sound about right? |
|
Is there any news on this memory issue or any new code to try out. I don't mean to be pushy it's just I was planning to use midi Bluetooth in class and I'm not sure if this is a long term problem and I should totally change me syllabus or if this is something that will be fixed very soon. thanks, jason |
|
What about #185 ? |
|
The 3 new "pins" added to MicroBitIO do not seem to be used. Removing int1,2&3 would save 48 bytes. |
|
Thanks for analyzing @martinwork. Good stuff... Exactly what I wanted to do too. You saved me a job there, thanks! You're right about the interrupt pins in MicroBitIO. No need for them to be in there anymore - a cleanup would be well worth it to reclaim those 48 bytes. This is a legacy artifact of the codal backport of the new sensor drivers... I suspect the rest of the additional RAM taken by those sensor drivers is due to the c++ vtable necessary for abstract classes now in there to transparently support the different accel/mag devices. The cost of the partial flashing service is a bit of a surprise. Having said that, we should also recognize that while looking to trim back a little memory from the recent changes is worthwhile, it is the BLE stack itself that is the root cause of the low memory conditions we're seeing. Let's not forget that the combined footprint of that is around 12K even before we add any services... and users were running out of memory on 2.0.0 too. So I think there are a number of actions we've identified that we could do:
The first seems like a no brainer, so i've tagged that one as something we can commit to doing. Investigating the second also seems sensible, although there may not be much that can be done. Something you can take a look at @microbit-sam? I'm generally in favour of the third, but worry a little about implications for some existing BLE applications that might assume the presence of one or two of those. The fourth feels potentially unsolvable (as SoftDevice is a black box, and we don't actually know how much it has used of the GATT table - or in fact which parts), but could save up to 1K of RAM for applications only running one BLE service... if it's possible. One of the Nordic engineers might be able to advise here... The fifth seems plausible to me, and could save around 200 bytes for a typical MakeCode application that doesn't make use of event handlers that block. We'd obviously have to make some changes to a core piece of code too, so would bring some considerable testing overhead. The final one would be invasive, and a fairly big job, but could potentially save the the most memory. I don't have hard data, but from what I've seen in the past, I wouldn't be surprised if we could save 1 - 1.5K for all applications. Thoughts everyone? tagging @jaustin here as some of these decisions could affect other stakeholders. |
|
I understand. Depending on how close you are to having enough memory for your application, some of the ideas above might help. It's difficult to be certain though I'm afraid. When are you planning to use the midi service in your teaching? |
|
I think (3) can be done on the MakeCode side (assuming all BLE services are guarded by yotta constants) We can be the "bluetooth" package into smaller packages with specific yotta defines. That way the user only pays for what it is using. |
|
thanks @pelikhan. Yes, I think all the current "mandatory" services can be disabled through a CONFIG option. |
|
...and if not, they easily can be. :) |
|
For reference |
|
@bluetooth-mdw @finneyj which BLE service precisely are optional? Looking at https://github.com/lancaster-university/microbit-dal/blob/master/source/bluetooth/MicroBitBLEManager.cpp#L384, it seems that the event service is the only one that can be turned off. Any definite list? |
|
@finneyj could you win back some bytes by making these services conditional fields in the BLE manager class (instead of dynamic allocation) |
|
@finneyj |
|
Here is a custom build that allows to turn off various services. I haven't been able to get MIDI going with this but I'm not getting 0x20 either. https://makecode.microbit.org/app/ceebbcc016c631523a6f1ea8d5ba9523b0e5c213-c728c358f3 (after adding the Bluetooth package, go to project settings) |
|
Thanks @pelikhan! That's pretty much exactly what I was thinking of. It may be possible that the GATT table is a now a bit too small for your midi service I guess? Or it could be that the app on the phone is expecting one of the services we just removed? Is there a way I can try to reproduce? |
|
Thanks for the info @machinehistories. Education is what we do, so we'll do our best to help. If you could also try out @pelikhan's build above, that would help. Do report back anything you find out. To the best of my knowledge, there are no plans to release a 32K variant of the micro:bit. Sorry! p.s. it might be interesting to talk about some related educational activities we have going on here at Lancaster sometime... |
|
@bluetooth-mdw Thanks - but I worry a little about complexity of that. Conditional compilation of individual characteristics would be very fiddly, and unlikely to yield much benefit... |
|
@pelikhan FYI - I just tried a build of your code above, as indicated in the image and a simple program:
I took a look at the micro:bit using Nordic nrfConnect BLE diagnostic tool, and with a GATT table size of 0x300, the DFU service was still running and there was no sign of your midi service (probably because it didn't fit). With a larger GATT table I could see your service, but it didn't seem to be doing much. The micro:bit didn't OOM for me, but didn't do much either... Look like the switch to turn of the DFUService didn't take, and there may also be something going on that's preventing any read/notifications from your midi service.... |
|
Could the sensor drivers give back the vtable RAM by using a less elegant implementation? The partial flashing service has variables (e.g. 64 byte buffer) that only get used in pairing mode. Maybe merging it's application mode features with the DFU service is an alternative to making it optional? The GattServer code has some large arrays of pointers to mostly non-existent objects. There are some buffers in the BLE service code that might be unnecessary because the data gets stored by soft device, but I haven't actually tested that and it's only a few bytes. GATT table... Could we have 0x400, 0x500 and 0x600? I think we should have individual control over device info and event services. Maybe minimal device info (version) rather than none? The Eddystone configs don't seem to affect RAM. |
|
Thanks @martinwork. Rapid thoughts on those ideas:
|
|
@finneyj Catching up and looking now |
|
When I was working with the config options on MakeCode to generate these two files (neither of which seem to have helped) microsoft/pxt-microbit#1281 (comment) I found GATT table below 0x500 resulted in the midi service being missing or incomplete. Sam found the same with 0x300 and the OOB hex - the services are just cut off. I think that makes this a potentially really confusing option. I'd be more in favor of creating some more specific 'build profiles' for known setups, so for example
And do these as things we validate to fit, rather than a generic option. I guess the problem is that it's very hard to do this without custom packages that only include certain blocks (but maybe that's a good option - a bluetooth-uart-only package?) I'd also like to investigate the stack/heap split, because that was what got me the biggest available heap win during testing. @finneyj when I was debugging with C++, I made the following change --- a/inc/core/MicroBitHeapAllocator.h
+++ b/inc/core/MicroBitHeapAllocator.h
@@ -78,6 +78,7 @@ DEALINGS IN THE SOFTWARE.
* simply use the standard heap.
*/
int microbit_create_heap(uint32_t start, uint32_t end);
+void microbit_heap_print();This allowed me to call microbit_heap_print() at various points during init and and after everything had come up and it was really useful. Would you consider that as a permanent change? If so, @pelikhan could we call it from MakeCode? For anyone debugging, the following in your project settings on MakeCode is really handy Added like this: Your micro:bit will tell you about every allocation over serial. |
|
@martinwork do we need 'Will break iOS and Android app flashing' with the device info toggle, or can you still flash without that service? @smartyw is that service really actually optional? |
|
thanks @jaustin. Yes, microbit_heap_print() is handy. Happy to make it externally visible. The behaviour of SoftDevice when the GATT table is too small is to fit in all the services/characteristics it can, and then it simply stops adding. So yes, services do get cut off or cut out completely. I'd much prefer a mechanism to determine how much GATT Table has been used, then reclaim the rest to avoid such guesswork, but we'll need some investigation to determine if this is possible. i.e. we always bring up the GATT table at full size, then have some sort of optional "finished add BLE stuff" block, which then reclaims any unused space... |
|
I think what you propose would work cleanly but unless we start to slim or make-optional some of the services is unlikely to save us much space in the table anyway... So no silver bullet but likely useful as part of a wider scheme. Do you have a sense of the memory overhead of the heap printing/serial debug? I presume it's not the whole chunk that the mbed printf would induce? |
|
The thing that really strikes me about this is that DFU is such a beast, and we currently always have it. If we were to cut out the DFU service except in pairing mode (so don't bring it up with MakeCode programs but only bring it up with pairing mode), that might make a huge difference. @microbit-sam would you consider the same with partial flashing? Is there a way to get partial flashing service to be smaller when it's in 'user' mode before it reboots? @martinwork can you share which commit you did that work on top of please? Sam's put partial flashing on a crash diet but not sure if that's included. |
|
This was the v2.1.0 release. Sam saved 64 bytes since. I wonder if PF could save more by only adding the event listener when required or by using an idle tick handler? It would be a big shame to always have to manually A+B+Reset to DFU or partial flash - not always easy and quickly gets tedious. In the basic configuration with event + DFU + PF services, I think 2.1.0 is using 360 (now 296) more bytes than 2.0.0. One possible way to get that back for most applications would be to delete the unused nRF5xGattServer::p_descriptors, nrfDescriptorHandles and descriptorCount and to drop the GATT table to 0x600, saving 124 + 256 = 380 bytes. |
|
I changed MicroBitBleManager to do |
|
@martinwork I can't square that result with your table above - is your list sorted by size, or incremental? |
|
The list is sorted by size. It's the extra bytes for each service on its own compared to no services. I guess including the DFU service in the compilation links in some library code with static data maybe like this: |
|
Calibrating the compass after connecting with BLE causes OOM or crashes microbit, possibly caused by stack overflow. compass.hex includes the MakeCode "calibrate compass" block - on button A pressed, calibrate. Doing this when BLE is or has been connected and receiving compass values (as with the iOS app monitor) tends to generate the panic 020. It seems connecting and linking up to services is allocating RAM (about 288 bytes?) which is not freed by disconnecting. If I remove the LED service from the hex, it doesn't panic anymore but it crashes microbit. I think this may be caused by stack overflow. The calibration code puts around 900 bytes on the stack. The same thing happens using the magnetometer service calibration characteristic. If calibration with BLE connected is not practical, maybe the magnetometer service calibration characteristic should be optional and not included by MakeCode, saving 124 bytes of RAM. One way to trigger calibration without USB flashing, would be to flash a MakeCode hex with On start, Calibrate. |
|
Is there an ETA for a slimmed down configuration of BLE? Chrome 70 just just got release which means WebBLE is now mainstream. |
|
MakeCode v1 appears to use more RAM than v0. The two attached hexes are the same but compiled in v0 and v1. They work with the iOS app monitor gamepad panel. I have removed 1,2,3,4 from the original sample because that was causing OOM. With the v1 one, it's still quite easy to cause an OOM panic by pressing A, B, C, D in rapid succession. The v0 one doesn't panic, though it grinds to a halt when the events queue up. |
|
Seems like we need an infrastructrure to track all memory allocation (before everyone builds one more). Could we have a flag that sends memory info to the serial? |
|
Is it possible to track the stack pointer more elegantly than by adding serial trace in multiple locations? |
|
Does the gcc -finstrument-functions option help at all? It should allow you to put your serial trace in every function entry/exit without changing the code. |
|
@pelikhan finneyj has done this - the settings in #390 (comment) will output all heap allocations over serial. As per #391 you can get a full heap status dump too but we don't have that wired in MakeCode yet I don't think (it's not in a released DAL version so not ready to use but is merged in master) |
|
@remay Thanks. I'll see if I can make that work. |
|
I failed to get finstrument-functions to work (I had an error from srec_cat) but adding stack size trace on entry to microbit_alloc was very informative. Using the compass sample above, during uBit init the stack reaches at least 1168, which exceeds the highest value I see when triggering compass calibration. A major contributor to this is DeviceInformationService. Using "delete new DeviceInformationService(..." reduces the stack usage by 496 and doesn't cause heap fragmentation. Stack size peaks occur in the BLE service constructors. Using new and delete for the characteristic and service objects in MicroBitEventService reduced its peak by 272 bytes. To avoid fragmentation, the characteristics need to be freed before calling OnDataWritten. There's a peak inside ble->init(), which I guess gives the level to aim at. It looks like it might be possible, without being too awkward, to reduce the start-up stack usage to 400ish. Although this might not be useful for MakeCode without a UI for stack-size, it could provide up to 800 extra bytes for specific C++ programs. I think the compass calibration code could reduce its stack usage by >400 bytes simply by using malloc for the data and visited arrays. I wonder if microbit_alloc should check the stack pointer against MICROBIT_HEAP_END and panic for overflow? |
|
Answering on behalf of @bluetooth-mdw Apologies for being quiet on this. In truth, github messages have been disappearing into my spam folder! @pelikhan asked which services are optional and which are mandatory. From a Bluetooth specification point of view, only generic access and generic attribute are mandatory. In other respects, that's open to discussion and debate. Essentially, they're all optional but
Afaik, that's it. You may have discussed all of this already; again apologies for being so far behind. In the spirit of brainstorming and nothing being off the table, Soft Device is not the only LE stack that runs on micro:bits. I use Zephyr a lot these days and can get the LE controller code, Bluetooth mesh stack and hundreds of lines of application layer code onto a micro:bit. It's a bit tight but wonder how it compares in terms of memory utilisation with Soft Device. I realise we probably have dependencies in tools like the mbed stuff but thought I'd mention it. |
|
We always flash in pairing mode, so DFU and partial flashing don’t have to be loaded in application mode (assuming we can put the device in pairing mode via the event service).
|
|
I have returned to the opinion that the memory issue is related to the change in the heap code and that things that were possible with BLE and the old heap code, will only be possible with the new heap code if it could be run in a mode that lets it get dangerously close to the stack. MakeCode's current stack size is about the minimum it needs for startup (and too small for some code e.g. #390 (comment)), but then many applications will only need 400 bytes or so to run. The old heap code used to make use of this 800 bytes danger zone. |
|
Too many disconnected discussion on BLE, we need a meeting where we nail all the issues.
All of those are interconnected and we should have a "master plan" rather than piecing fixes independently. |
|
Joe and Sam are doing a review together on Friday 14th and Joe and I
picking it up on 17th. Once we have the info Sam's been working on putting
together I think we can work out those next steps Peli :)
…On Thu, 6 Dec 2018 at 12:47, Peli de Halleux ***@***.***> wrote:
Too many disconnected discussion on BLE, we need a meeting where we nail
all the issues.
- default services
- flashing procedure (app mode detection, etc...)
- memory allocation
All of those are interconnected and we should have a "master plan" rather
than piecing fixes independently.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#390 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI-qcpyt3X0heX7xQe4FP-dH2Jd4SMnks5u2RHagaJpZM4W7S3g>
.
|
|
How's about agreeing the goal? That would help focus efforts. Should the goal be that all Bluetooth services can be instantiated? Much as I like that capability and regard losing it as an unintended backwards step.... We should discuss whether or not this is a realistic goal and needed capability. |
|
Great to hear! I’ve got getting DFU in the Webapp on my radar.
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Jonathan Austin <[email protected]>
Sent: Thursday, December 6, 2018 5:49 AM
To: lancaster-university/microbit-dal
Cc: Peli de Halleux; Mention
Subject: Re: [lancaster-university/microbit-dal] BLE Eats too much RAM... (#390)
Joe and Sam are doing a review together on Friday 14th and Joe and I
picking it up on 17th. Once we have the info Sam's been working on putting
together I think we can work out those next steps Peli :)
On Thu, 6 Dec 2018 at 12:47, Peli de Halleux ***@***.***> wrote:
Too many disconnected discussion on BLE, we need a meeting where we nail
all the issues.
- default services
- flashing procedure (app mode detection, etc...)
- memory allocation
All of those are interconnected and we should have a "master plan" rather
than piecing fixes independently.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#390 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI-qcpyt3X0heX7xQe4FP-dH2Jd4SMnks5u2RHagaJpZM4W7S3g>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flancaster-university%2Fmicrobit-dal%2Fissues%2F390%23issuecomment-444876233&data=02%7C01%7Cjhalleux%40microsoft.com%7Cb2d4531db70e441f65a108d65b819291%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636797009406674046&sdata=CfxqWv0lzdNk4H2vq%2B6wnWnjKRUf6himrno%2BMe3aqo8%3D&reserved=0>, or mute the thread<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAD-4KX6DqWOgADbGO4oF-3CnsyKwvRD7ks5u2SBKgaJpZM4W7S3g&data=02%7C01%7Cjhalleux%40microsoft.com%7Cb2d4531db70e441f65a108d65b819291%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636797009406684055&sdata=cXmUXYQxea9BBiB9AzWBun51khpUAqvMdKHESQHY46w%3D&reserved=0>.
|
|
I've had problems with memory with the new release with code that only uses a few services but which used to be fine. So thinking about goals once again, maybe it would be better to define it in terms of remaining ram after Bluetooth and the GATT stuff has been instantiated rather than the number of services? It would be nice to base this on whatever that number tended to be prior to the recent changes. |
|
Discussion from #410 @pelikhan Get Outlook for iOShttps://aka.ms/o0ukef For a standard (I think) C++ config.json the micro:bit has DFU, Event, and Device Information Service in both application and pairing so I'm not sure it will be possible to determine mode by looking at services @martinwork @pelikhan @martinwork @pelikhan Get Outlook for iOShttps://aka.ms/o0ukef @pelikhan @microbit-sam I'm heading up to Lancaster on Friday 14th to discuss Bluetooth with Joe, so hopefully we can merge into master before Christmas @martinwork With the existing implementation of DFU, most of it's RAM usage would not be saved by avoiding starting the DFU service in application mode - at least according to my tests, which could be rubbish of course! That's why adding the reboot command to the DFU service could be the best option. See #390 (comment) and the next few comments. If rethinking this and making breaking changes, combining some services might be another possibility. If the DAL was changed to avoid starting the PF service in application mode, with the reboot functionality in the event service or elsewhere, the presence of the PF service would mean either you're in pairing mode or it's a hex built with the previous version of the DAL, so you would still need to ask the PF service what the mode is. To be most generally useful, a pairing mode indication needs to be in the advertising info, so you don't have to connect to find out and so it doesn't depend on what services have been started. |
|
Hi, as I am debugging heap issues as well, I have updated my mbed allocator analyze script to be easier to use and also print a heap overview and a list of free blocks: https://github.com/thinkberg/mbed-memtrace-logger The heap overview etc. is available if |
Creating this as a catch-all issue pertaining to all activity relating to making more RAM available to applications when running BLE services
The text was updated successfully, but these errors were encountered: