Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

microbit-sam
Copy link
Contributor

@microbit-sam microbit-sam commented Nov 28, 2018

Combines existing PRs #394 #340 #395


Changes include:

  • Partial flashing service: dynamically allocate block buffer
  • Device Information Service: "BBC micro:bit" or "BBC micro:bit v1.3" or "BBC micro:bit v1.5"
  • MicroBitEvent to restart into pairing mode
  • Wait for pairing info to be saved before showing the tick

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
Copy link

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

@microbit-sam
Copy link
Contributor Author

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:

  • Inferring Application mode from lack of Partial Flashing Service
  • Requesting the restart in every case, and deciding not to restart if already in pairing mode
  • Add information to Device Information Service (this was discussed in a ticket somewhere but I can't find it right now!)
  • If the event service is a default service, create a call and response event?

@pelikhan
Copy link

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

@microbit-sam
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link

pelikhan commented Dec 3, 2018 via email

@microbit-sam
Copy link
Contributor Author

microbit-sam commented Dec 5, 2018

@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
Copy link
Contributor

smartyw commented Dec 5, 2018

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

@martinwork
Copy link
Contributor

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

@microbit-sam
Copy link
Contributor Author

Sorry, wasn't thorough enough

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

@martinwork
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

smartyw commented Dec 6, 2018

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

@microbit-sam
Copy link
Contributor Author

microbit-sam commented Jan 3, 2019

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

@microbit-sam
Copy link
Contributor Author

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
Copy link

pelikhan commented Jan 7, 2019

What is the status on this?

@microbit-sam
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

microbit-sam commented Jan 8, 2019

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
Copy link
Contributor

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 #390 (comment).

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

@microbit-sam
Copy link
Contributor Author

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
Copy link
Contributor

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?

@microbit-sam microbit-sam requested a review from finneyj May 6, 2020 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants