search

lancaster-university / microbit-dal

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

Reduce BLE memory usage. MicroBitEvent to restart into pairing mode. #409

Open microbit-sam opened 5 years ago

microbit-sam commented 5 years ago

Combines existing PRs #394 #340 #395

Changes include:

Even with these changes (+ local changes to Nordic BLE layer) the micro:bit still throws an 020 error when using all services - a build with all services apart from the UART works, but is probably running very close to running out of memory

pelikhan commented 5 years ago

What is the strategy to determine if a device is in pairing mode vs application mode?

microbit-sam commented 5 years ago

Sorry, had intended to add a better description yesterday but I was travelling

Currently it's possible to determine mode using the Partial Flashing Service. However, this is unavailable in C++ (and MakeCode radio builds?)

Options I've been thinking about are:

pelikhan commented 5 years ago

Could you add a bit of info in the information service?

microbit-sam commented 5 years ago

Appending an asterisk to the device name has been discussed before but there was no conclusion wrt. the best way of sharing the info #332 #331 CC: @martinwork @smartyw @finneyj

Modifying the model number would be possible, and should have very little impact for clients, but I don't think it would be the correct place to add the info as it wouldn't match the spec

martinwork commented 5 years ago

Changing the device name (advertised or GAP) may not work for iOS: see https://forums.developer.apple.com/thread/19381. The GAP name is cached and might obscure the advertised name. What about the manufacturer data? Getting this info from the advertisement without connecting would be good, but in the same post there is a question over how often the advert is updated.

pelikhan commented 5 years ago

Yes device name is a bad idea. We use the name as a filter in WebBLE. I guess the presence of the EventBus service indicates that we are in Application land.

From: Martin Williams notifications@github.com Sent: Monday, December 3, 2018 8:21 AM To: lancaster-university/microbit-dal microbit-dal@noreply.github.com Cc: Peli de Halleux jhalleux@microsoft.com; Comment comment@noreply.github.com Subject: Re: [lancaster-university/microbit-dal] Reduce BLE memory usage. MicroBitEvent to restart into pairing mode. (#409)

Changing the device name (advertised or GAP) may not work for iOS: see https://forums.developer.apple.com/thread/19381https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforums.developer.apple.com%2Fthread%2F19381&data=02%7C01%7Cjhalleux%40microsoft.com%7C8ae94cc774a443c80cba08d6593b4e73%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636794508587313960&sdata=cn1IfZdyJjD7Q6LpQMoVuF6ko1eQyV1QZR3f26VrebU%3D&reserved=0. The GAP name is cached and might obscure the advertised name. What about the manufacturer data? Getting this info from the advertisement without connecting would be good, but in the same post there is a question over how often the advert is updated.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flancaster-university%2Fmicrobit-dal%2Fpull%2F409%23issuecomment-443770049&data=02%7C01%7Cjhalleux%40microsoft.com%7C8ae94cc774a443c80cba08d6593b4e73%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636794508587323972&sdata=gow%2FBGh8MSbiTls8Qjugj%2B010h8860ZuDk%2FJcevPKAI%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAD-4KY5wO-kMAL1tMTN1OuN01Y4GZF2Rks5u1U9ngaJpZM4Y4A-3&data=02%7C01%7Cjhalleux%40microsoft.com%7C8ae94cc774a443c80cba08d6593b4e73%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636794508587333980&sdata=%2BGvlIjHAyB8T7%2BsDPUKFgp8kZ1t9YwWSuRQ%2B%2FsdtRcA%3D&reserved=0.

microbit-sam commented 5 years ago

@martinwork @smartyw Here's a build of the all services hex using this branch if you're interested in testing it So far I've tested it (Android + nRF Connect) and it appears to work fine

ble-dal-integration_all-services.hex.zip

smartyw commented 5 years ago

Thanks... if I have time at the weekend I'll give it a go.

martinwork commented 5 years ago

The magnetometer service has only one characteristic and the temperature service has none.

microbit-sam commented 5 years ago

Sorry, wasn't thorough enough

This has all characteristics: all-services-ble-dal-integration.hex.zip

martinwork commented 5 years ago

I'm sorry, but that panics OOM when the message finishes scrolling. I'm more used to being on the receiving end of this kind of message!

microbit-sam commented 5 years ago

Thanks for catching this Martin, apologies for so much back and forth

Which micro:bit are you using? I think we must be very close to the memory limit. I've currently only got two micro:bits with me, one 1.5 and one 1.5 RC board with the FXOS acc/mag on. It runs correctly on the RC board so it's possible that the FXOS driver uses slightly less memory than the LSM303/MAG3110

The largest I'm able to set the gatt table without an 020 error is 0x6C8, this still isn't enough for the temperature characteristic, and if it's running that close to limit I'd be worried about the other services not crashing.

Removing descriptors from the BLE layer as you suggested hasn't been included in this build, but that may be required to save the necessary memory.. I'll rebuild and test across all devices and get back to you

martinwork commented 5 years ago

My micro:bit is old. If you can't use a full size GATT table, I'd say more memory needs to be saved!

If you mean the arrays in nRF5xGattServer, I have got rid of the p_descriptors array in my code, but not nrfDescriptorHandles - removing that seemed to cause problems.

smartyw commented 5 years ago

Totally agree with @martinwork. If we can't then this is a dreadful backwards step.

microbit-sam commented 5 years ago

@martinwork @smartyw I've only got a working 1.5RC1 with me at the moment, but this is an all services hex that works for me

All characteristics appear and can be used (1.5RC w/ LSM303)

BLE_All_Services_DAL_2-1-1.hex.zip

martinwork commented 5 years ago

On a v1.3 I see panic 020 on micro:bit as soon as the iOS monitor connects and a callback to didFailToConnectToPeripheral in the code.

microbit-sam commented 5 years ago

Cool, thanks for the heads up, I'll remove the mag service and see if that frees enough memory

microbit-sam commented 5 years ago

Without magnetometer service: BLE_All_Services_DAL_2-1-1.hex.zip

As of tomorrow I'll have access to all my micro:bits again so will also do some testing!

pelikhan commented 5 years ago

What is the status on this?

microbit-sam commented 5 years ago

The hex without a the mag service works for me on a v1.3 using nRF Connect

@pelikhan I'm not certain on a release timescale, probably when I'm next in Lancaster with Joe, I'm hoping to organise a trip up for the end of the month

martinwork commented 5 years ago

The version without magnetometer service works for me too with the iOS app but I have lost track of what this tests.

I can build the bluetooth-services sample including the magnetometer service with DAL 2.1.0, by reducing the stack size to 1280, though it's right on the edge of OOM. I can add a compass.heading() call to trigger calibration. If I reset before connecting via BLE, all the services work OK, but I suspect adding much more code would cause OOM as does calling the compass calibration characteristic.

This version seems to be worse than 2.1.0 if it can't include the mag service.

microbit-sam commented 5 years ago

This is to replace the all samples hex that currently exists the docs, which is built using DAL 1.4.8 and doesn't work on micro:bit 1.5 boards

I don't think any existing DAL 2.1.0/2.1.1 all services/Out Of Box hex actually include the magnetometer service, but reducing the stack size to 1280 sounds like the solution

Did you also try that with DAL 2.1.1? Is it that close too OOM that the 2.1.1 hits the limit? I'll try some builds this afternoon

microbit-sam commented 5 years ago

Building on 2.1.1 with stack size set to 1260 (1280 made my 1.5 crash with 020 OOM when calibrating the mag) works if I include these services. Note this does not include the DFU service

    // EventService in config.json
    new MicroBitAccelerometerService(*uBit.ble, uBit.accelerometer);
    new MicroBitButtonService(*uBit.ble);
    new MicroBitIOPinService(*uBit.ble, uBit.io);
    new MicroBitLEDService(*uBit.ble, uBit.display);
    new MicroBitMagnetometerService(*uBit.ble, uBit.compass);
    new MicroBitTemperatureService(*uBit.ble, uBit.thermometer);

BLE_All_Services_DAL_2-1-1-NODFU.hex.zip

Edit: To trigger calibration you need to write 0x01 to the calibration characteristic. If the program were to automatically trigger calibration when it starts then micro:bits with broken magnetometers would throw 050 errors immediately and be unusable.

I think as the DAL stands we may have to choose between DFU and Mag, until the BLE stacks memory usage is substantially reduced

Edit 2: @martinwork This build removes the event listeners that display C for onConnected, and D for onDisconnected. This seems to free enough memory to include Event, Accelerometer, Button, IOPin, LEDs, Mag, Temperature, DFU, and Device Information Service. I've tried on a 1.3 and 1.5, tested the characteristics, mag calibration, and getting into DFU mode without getting an 020 error BLE_All_Services_DAL_2-1-1-NOLISTENERS.hex.zip

martinwork commented 5 years ago

A hex aimed at BLE demonstration really needs the DFU service.

The NOLISTENERS build on a v1.3 microbit either OOMs, or crashes when I try to send the calibration trigger using the Calibrate entry on the iOS app Compass panel.

Maybe having to use the BLE calibration trigger isn't ideal for general demo type use? Isn't there another way to work around the broken magnetometer case?

Could the scrolling text come after starting the BLE services? It's confusing (to me at least!) and tedious that you have to wait for the text before connecting.

MakeCode uses stack size 1280. Reducing it further may not be a good idea. I believe even 1280 may be too small to do calibration. See https://github.com/lancaster-university/microbit-dal/issues/390#issuecomment-430959849.

These examples seem to be demonstrating that the RAM shortage issue is still there.

microbit-sam commented 5 years ago

A hex aimed at BLE demonstration really needs the DFU service.

Here's a hex built off a modified DAL that only brings up the DFU service in pairing mode. I've tested this on a v1.3 and v1.5, all characteristics work, I can trigger and complete mag calibration, and complete a DFU from pairing mode using the Android app.

I've not got an iOS device so if you have time to test and provide feedback that would be amazing!

BLE_All_Services_DAL_2-1-1-DFU_Only_Pairing_Mode.hex.zip

The NOLISTENERS build on a v1.3 microbit either OOMs, or crashes when I try to send the calibration trigger using the Calibrate entry on the iOS app Compass panel.

Ok, I'd not managed to do that with my v1.3 and nRF connect

Maybe having to use the BLE calibration trigger isn't ideal for general demo type use? Isn't there another way to work around the broken magnetometer case?

I can't think of an answer to this problem..

Could the scrolling text come after starting the BLE services? It's confusing (to me at least!) and tedious that you have to wait for the text before connecting.

Yep, changed that

MakeCode uses stack size 1280. Reducing it further may not be a good idea. I believe even 1280 may be too small to do calibration. See #390 (comment).

I've currently got the stack size set to 1140, haven't noticed any ill effects so far but there may well be

These examples seem to be demonstrating that the RAM shortage issue is still there.

Yep, RAM shortage is definitely still a problem. This will hopefully be mitigated by a stripped back BLE stack in the future..

martinwork commented 5 years ago

I tried some modifications of the app. If the app connects just to write 0x01 to the calibration characteristic, it works, but if the app triggers calibration while linked to all the services, microbit crashes during the tilting process. If the app only links to the magnetometer, it seems to work once but panic or crash on a second attempt. The app creates a problem for the calibration routine by altering the acc and mag periods, but fixing that doesn't fix the crashing. If I connect to all services without changing periods, disconnect then reconnect just to trigger calibration, it still crashes.

Maybe the best bet for now is not to include the magnetometer service if the existing sample doesn't have it.

Should calibrationUX take control of the update periods as it does for screen brightness?