lancaster-university / microbit-dal

http://lancaster-university.github.io/microbit-docs
Other
254 stars 130 forks source link

BLE Eats too much RAM... #390

Open finneyj opened 5 years ago

finneyj commented 5 years ago

Creating this as a catch-all issue pertaining to all activity relating to making more RAM available to applications when running BLE services

martinwork commented 5 years ago

Comparing 2.0.0 with 2.1.0, the low level BLE code hasn't changed, has it? It seems something has increased RAM usage such that there isn't enough for BLE anymore. What has stolen all the RAM that BLE used to eat?!

martinwork commented 5 years ago

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.

martinwork commented 5 years ago

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.

martinwork commented 5 years ago

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?

machinehistories commented 5 years ago

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

ghost commented 5 years ago

What about https://github.com/lancaster-university/microbit-dal/issues/185 ?

martinwork commented 5 years ago

The 3 new "pins" added to MicroBitIO do not seem to be used. Removing int1,2&3 would save 48 bytes.

finneyj commented 5 years ago

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.

finneyj commented 5 years ago

@machinehistories

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?

pelikhan commented 5 years ago

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.

finneyj commented 5 years ago

thanks @pelikhan. Yes, I think all the current "mandatory" services can be disabled through a CONFIG option.

finneyj commented 5 years ago

...and if not, they easily can be. :)

pelikhan commented 5 years ago

For reference image

pelikhan commented 5 years ago

@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?

pelikhan commented 5 years ago

@finneyj could you win back some bytes by making these services conditional fields in the BLE manager class (instead of dynamic allocation) https://github.com/lancaster-university/microbit-dal/blob/master/source/bluetooth/MicroBitBLEManager.cpp#L385

machinehistories commented 5 years ago

@finneyj I was planning to have my students purchase their microbits next week and we would begin work on the 17th of oct. The class is at Art Center College of Design in Pasadena CA. The students are grad level in Environmental Design department and the goal is for them to prototype an interactive spatial project. The students are not programmers but some have a bit of arduino awareness. I feel like I can get them to do better projects faster with the microbit. We will mainly focus on sound, lighting, and sensing. The bluetooth aspect is important since many will develop a basic app as a controller. Thanks for all the hard work in getting this resolved. Out of curiosity any plans on a 32k version and if so when might it happen.

pelikhan commented 5 years ago

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) image

ghost commented 5 years ago

@finneyj What about #185 ? If some characteristics were designated optional, wouldn't that help a bit?

finneyj commented 5 years ago

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?

finneyj commented 5 years ago

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...

finneyj commented 5 years ago

@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...

finneyj commented 5 years ago

@pelikhan FYI - I just tried a build of your code above, as indicated in the image and a simple program:

image

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....

martinwork commented 5 years ago

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.

finneyj commented 5 years ago

Thanks @martinwork. Rapid thoughts on those ideas:

microbit-sam commented 5 years ago

@finneyj Catching up and looking now

jaustin commented 5 years ago

When I was working with the config options on MakeCode to generate these two files (neither of which seem to have helped) https://github.com/Microsoft/pxt-microbit/issues/1281#issuecomment-424935097 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

    "yotta": {
        "config": {
            "microbit-dal": {
                "debug": 1,
                "heap_debug": 1,
                "gatt_table_size": "0x600",
                "stack_size": 1024
            }
        }

Added like this: debug-print-memory

Your micro:bit will tell you about every allocation over serial.

jaustin commented 5 years ago

@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?

finneyj commented 5 years ago

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...

jaustin commented 5 years ago

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?

finneyj commented 5 years ago

Indeed - this would be most sensible alongside making most/all of the services optional.

heap_print() doesn't use much RAM, no. Anything that maps to uBit.serial.printf() is pretty light. It's the static printf() that tries to allocate a large buffer on first use.

jaustin commented 5 years ago

Okay, here's a slightly counter proposal to discuss - I know it'd have some nontrivial implications for the apps, but that might be better than some of the other options

Upsides

Downsides

Based on the downsides perhaps we need to phase this in with a new 'lean' Bluetooth package, or do it at the v0/v1 change (are we too late?)

martinwork commented 5 years ago

@jaustin I don't think device info is required for DFU. It will be interesting to see if the app copes - it was designated mandatory but with all optional characteristics.

martinwork commented 5 years ago

The event service is probably one of the big ones, RAM wise: it has 4 characteristics, one of which needs a live GattCharacteristic object.

jaustin commented 5 years ago

Good point about the size of the Event Service. I picked that because I wanted one of the two services I was poposing as default to be able to trigger a reboot into update mode of some kind. I didn't want to mess with the Device Information Service's characteristics as that's a standardised service. We could just have a super minimal service to trigger reboot though - we'd then need to let users turn on the even service, which I think they're often going to want to do and that'll get us into a worse place then before?

pelikhan commented 5 years ago

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...

Currently, most optional BLE services are added on the fly so you don't really know how much GATT you need.

finneyj commented 5 years ago

right - that's why I was thinking we could perhaps measure it -which might be possible, if and only if, we have some way to clearly state that the user has stopped adding more services...

Not sure if this would be elegant from a user's perspective, but is probably better than we have.

pelikhan commented 5 years ago

Added the "debug" option in settings, removed GATT https://makecode.microbit.org/app/d91d57bba20c60dd4c9c43917241abb480521c93-bb9b7e94bf

pelikhan commented 5 years ago

@finneyj https://github.com/lancaster-university/microbit-dal/issues/390#issuecomment-426221098 you need to add "bluetooth start midi service"

finneyj commented 5 years ago

:) oops.

finneyj commented 5 years ago

OK, so this kinda works for me (although just reading data from nrfConnect, not a real midi device). Curiously though, notifications don't seem to be working. I can read data, but if I register for a notification, I don't see any data streamed...

image

image

pelikhan commented 5 years ago

@machinehistories we had a separate bug in the ble-midi package that surfaced in v1. I have updated the package to 2.0.8 with the fix. Here is an example. https://makecode.microbit.org/_C1xUmyK6APRH I will have the options in the settings editor later on to be able to disable some BLE services and reclaim space.

pelikhan commented 5 years ago

Here is the PR for additional options: https://github.com/Microsoft/pxt-microbit/pull/1327

jaustin commented 5 years ago

@pelikhan was that bug related to notifications or just the build? Edit: answered my own question: yes :) https://github.com/Microsoft/pxt-bluetooth-midi/commit/120441c29d58cd00f76847b23c31740781515cbf

pelikhan commented 5 years ago

It was notifications. Somehow I'm not getting the 0x20 in those tiny programs.

microbit-sam commented 5 years ago

I've reduced the PFS by 60 bytes, want to test it a bit more the ensure nothings broken

@finneyj Is there a way to reduce the amount used by a message bus listener?

SERIAL_DEBUG->printf("***<Partial Flashing MB>***\r\n");
// Set up listener for SD writing
messageBus.listen(MICROBIT_ID_PARTIAL_FLASHING, MICROBIT_EVT_ANY, this, &MicroBitPartialFlashingService::partialFlashingEvent);
SERIAL_DEBUG->printf("***</Partial Flashing MB>***\r\n");
***<Partial Flashing MB>***

malloc: ALLOCATED: 40 [0x2000320c]

malloc: ALLOCATED: 24 [0x20003238]

malloc: ALLOCATED: 24 [0x20003254]

***</Partial Flashing MB>***
pelikhan commented 5 years ago

@machinehistories would love to learn more about how you use the micro:bit. I am adding MIDI support in the simulator so that you don't have to constantly pair with the phone.

machinehistories commented 5 years ago

@pelikhan That's amazing thanks so much I'll try it right away. I would be happy to tell you more about how we use the microbit.

pelikhan commented 5 years ago

@machinehistories can we continue this conversation somewhere else? I'm curious to understand what's needed to get micro:bit and firefly to work great together.

machinehistories commented 5 years ago

@pelikhan Sure sorry for that ridiculously long reply. You can email me at jason@machinehistories.com. - jason

On Tue, Oct 2, 2018 at 4:35 PM Peli de Halleux notifications@github.com wrote:

@machinehistories https://github.com/machinehistories can we continue this conversation somewhere else? I'm curious to understand what's needed to get micro:bit and firefly to work great together.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lancaster-university/microbit-dal/issues/390#issuecomment-426465115, or mute the thread https://github.com/notifications/unsubscribe-auth/AZlqVIQEsnQD6m5VXe91Mb6NXBHzZ5IDks5ug_hOgaJpZM4W7S3g .

machinehistories commented 5 years ago

I used the new code in the link above https://makecode.microbit.org/_C1xUmyK6APRH but I couldn't get anything to recognize the microbit as a bluetooth midi device. I couldn't get the microbit to pair on windows 10 at all although it could pair on android and ios. However on ios and android it wasn't showing up as a midi device. Can you verify that in the new code bluetooth.onBluetoothConnected(function () { bluetooth.startMidiService() }) is no longer needed. Also I still got the 020 error but perhaps that was because I hit both buttons at the same time. Hopefully some of the advanced settings can help. Sorry for lack of good news but I didn't have any luck. On a positive note the serial midi debugging in the serial plotter is very cool. thanks