espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.8k stars 749 forks source link

nRF5x: add BluetoothRemoteGATTCharacteristic.startNotifications #959

Closed gfwilliams closed 7 years ago

gfwilliams commented 8 years ago

Currently we can only read and write, but notifications are needed to handle stuff like the Nordic UART nicely

Whizzard commented 7 years ago

Would really like this feature and thought about trying to contribute myself. Have some past C/C++ experience, but feel a bit lost in the whole source right now. Any hints where to start / what to try?

PS: Already found my way to libs/bluetooth/jswrap_bluetooth.c for starters... ;-)

gfwilliams commented 7 years ago

Thanks! targets/nrf5x/bluetooth.c is another good place to look too (that's the more platform-specific side of the code).

The gotcha is that I believe you need to know the handle of the Characteristic's descriptor (cccd_handle), so you can write to it with sd_ble_gattc_write to enable notifications. Most stuff I've seen seems to use the a 'discovery' library, but Puck.js does it directly at the moment - so you'd have to see what discovery is doing to find that handle, and then implement it (roughly where BLE_GATTC_EVT_CHAR_DISC_RSP is used in bluetooth.c)

Hope that's some help! To be honest once there some code to add a handle_cccd to the characteristic (like is done for handle_value) then I could do the rest really quickly.

Whizzard commented 7 years ago

Saw your code from commit f7d4946 and spent some time to dig deeper. Not sure how to approach this. jswrap_BluetoothRemoteGATTService_getCharacteristic() currently just does the base discovery. Should it a) also do the descriptors discovery additionally or better b) in a different task (like BleTask::BLETASK_CHARACTERISTIC_DESC)?

For a) the if(done) part in the on_ble_evt() will have to be somehow postponed till both - characteristic AND descritptor discovery is done, right? For b) I am not sure when and how to start the new task, but at first it sounds like the cleaner approach.

OT: Oh my, this is really complex stuff. Never would have thought that. Chapeau for doing all this with that much speed, precision and enthusiasm.

gfwilliams commented 7 years ago

Thanks - actually that's a really good point - there's not much reason to do it during discovery since it won't get used most of the time and it'll slow down characteristic discovery for everything. Best to do it when startNotifications (and I guess stopNotifications) is called.

We could have a task BLETASK_CHARACTERISTIC_DESC_AND_STARTNOTIFY that gets the descriptor and then starts notifications once it's received, since realistically it wouldn't matter if startNotifications took a little longer.

I'll have a look at doing that - it should make it lot easier as the discovery would be a bit of a nightmare to add it to.

And thanks! I guess it's shows it's working if most people aren't aware how much work has to go on underneath to make it 'just work' :)

Whizzard commented 7 years ago

Sorry for the delay. I have spent a hour here and there to look into this and may be somewhere at 80% of a possible solution. Have my NRF52 DK up and running also to have faster test cycles.

One issue still: If I do startNotifications() and trigger a discovery first with the new task - being in on_ble_evt() I'd like to add the missing cccd property to the current Characteristic object. But how do I get it there? There is no proper context? Keep a reference in a static like bleStatus? Can I return something in an intermediate promise that is caught by the calling function again?

In jswrap_BluetoothRemoteGATTService_getCharacteristics() it's easy because the Characteristic is returned as a newly created object inside the callback.

That's where I am stuck currently.

Maybe you could give me a hint to the right direction?

Whizzard commented 7 years ago

I have managed to put something together, but it does not seem to work yet: https://github.com/Whizzard/Espruino/tree/nrf52-ble-notifications

Some things may be hacky:

This is the code I use for testing:

var count = 0;

NRF.setServices({
  0x4242 : {
    0x4242 : {
      value : count,
      readable : true,
      notify: true,
      indicate: true,
      writable: true,
      onWrite: (evt) => {
        count = evt.data[ 0 ];
        LED3.set();
        setInterval( () => {
          LED3.reset();
        }, 200 );
      }
    }
  }
});

var interval;

NRF.on( 'connect', () => {
  if( interval !== undefined ) {
    clearInterval( interval );
  }

  LED2.set();
  setTimeout( () => {
    LED2.reset();
  }, 100 );

  interval = setInterval( () => {
    NRF.updateServices({
      0x4242 : {
        0x4242 : {
          value : count++,
          notify: true
        }
      }
    });
    analogWrite( LED3, 0.01 );
    setTimeout( () => {
      LED3.reset();
    }, 100 );
  }, 1000 );
});

NRF.on( 'disconnect', () => {
    if( interval !== undefined ) {
      clearInterval( interval );
      interval = undefined;
    }

    LED1.set();
    setTimeout( () => {
      LED1.reset();
    }, 100 );
});

var device;
var characteristic;

function checkButton() {
  setWatch( () => {
    connect();
  }, BTN );
}

function connect() {
  NRF.requestDevice({ filters: [ { name: 'Puck.js 2844' } ] }).then( (d) => {
    console.log( 'found', d.name );
    return d.gatt.connect();
  }).then( (g) => {
    device = g;
    setTimeout( () => {
        console.log( "timeout - disconnected" );
        g.disconnect();
        device = undefined;
        characteristic = undefined;
        checkButton();
    }, 25000 );
    console.log( 'connected', g );
    return g.getPrimaryService( "4242" );
  }).then( (s) => {
    console.log( 'service found', s );
    return s.getCharacteristic( "4242" );
  }).then( (c) => {
    characteristic = c;
    console.log( 'characteristic found', c );
    return c.readValue();
  }).then( (v) => {
    console.log( 'value', v.charCodeAt(0) );
    characteristic.on('characteristicvaluechanged', (e) => {
      console.log( 'notify', e.target.value.charCodeAt(0) );
    });
    return characteristic.startNotifications();
  }).then( i => {
    console.log( "notifications started", i );
  }).catch( e => {
    console.log( "error", e );
    if( device !== undefined ) {
      device.disconnect();
      device = undefined;
    }
  });
}

checkButton();

It runs on a Puck as a test probe. Once a central connects the characteristic is increased every second so there should be a notification every time that happens. Tested to work with the iOS LightBlue app.

When running the new code on a NRF52-DK board it finds the Puck and everything including the cccd-handle but there is no notification: (trigger by button press)

found Puck.js 2844
connected BluetoothRemoteGATTServer {
  "device": BluetoothDevice {
    "id": "dd:16:f9:6a:28:44 random",
    "rssi": -54,
    "services": [  ],
    "data": new ArrayBuffer([2, 1, 5, 13, 9, 80, 117, 99, 107, 46, 106, 115, 32, 50, 56, 52, 52]),
    "name": "Puck.js 2844",
    "gatt":  ...
   },
  "connected": true }
service found BluetoothRemoteGATTService {
  "uuid": "0x4242",
  "isPrimary": true, "start_handle": 17, "end_handle": 65535 }
characteristic found BluetoothRemoteGATTCharacteristic {
  "uuid": "0x4242",
  "handle_value": 19, "handle_decl": 18,
  "properties": { "broadcast": false, "read": true, "writeWithoutResponse": true, "write": true,
    "notify": true, "indicate": true, "authenticatedSignedWrites": false }
 }
value 122
BluetoothRemoteGATTCharacteristic {
  "uuid": "0x4242",
  "handle_value": 19, "handle_decl": 18,
  "properties": { "broadcast": false, "read": true, "writeWithoutResponse": true, "write": true,
    "notify": true, "indicate": true, "authenticatedSignedWrites": false },
  "#oncharacteristicvaluechanged": function (e) {console.log( 'notify', e.target.value.charCodeAt(0) );},
  "handle_cccd": 20 }
found
timeout - disconnected
gfwilliams commented 7 years ago

Thanks! I'm a bit busy catching up for the start of this week, but I'll try and look into this properly later on.

It looks good though - only thing is instead of m_characteristic_desc_discover, how about using bleTaskInfo? It's meant for this kind of thing.

Whizzard commented 7 years ago

Thanks for the feedback. I removed m_characteristic_desc_discover and use bleTaskInfo instead now. Not sure yet if the cleanup and handling is according to the bleTask mechanics. Just pushed the changes: https://github.com/espruino/Espruino/compare/master...Whizzard:nrf52-ble-notifications?expand=1

It works as before: characteristics + descriptors are discovered, cccd_handle found, notifications enabled - theoretically. But the event is still missing.

Could someone give me a brief hint how to get into debugging with the NRF52-DK board? Some simple console logging of variables or checkpoints would be nice.

gfwilliams commented 7 years ago

Hi,

Sorry it's taken a while to look into this... I tried the following, but got CCCD Handle not found on a Puck.js - I'm wondering if maybe the handle range is wrong.

var gatt;
NRF.connect("c6:7e:84:c4:e7:82 random").then(function(g) {
  gatt = g;
  return gatt.getPrimaryService("6e400001-b5a3-f393-e0a9-e50e24dcca9e");
}).then(function(service) {
  return service.getCharacteristic("6e400003-b5a3-f393-e0a9-e50e24dcca9e");
}).then(function(characteristic) {
  console.log("Start notifications ", characteristic);
  characteristic.on('characteristicvaluechanged', function(event) {
    //var value = event.target.value.buffer;
    console.log("RX:"+JSON.stringify(event));
  });
  return characteristic.startNotifications();
}).then(function() {
  console.log("Done!");
});

I'm looking into this now though - it's looking really promising.

wrt debugging: I can't be much help - you can use GDB directly and there's a JLink GDB server. It's not 'fun' though :)

Whizzard commented 7 years ago

Thanks for checking. Strange. I found a handle (see my logs), but got no notification.

Have a look at bluetooth.c - line 1500ff:

void jsble_central_characteristicDescDiscover(JsVar *characteristic) {
...
  // start discovery for our single handle only
  uint16_t handle = (uint16_t)jsvGetIntegerAndUnLock(jsvObjectGetChild(characteristic, "handle_value", 0));

  ble_gattc_handle_range_t range;
  range.start_handle = handle;
  range.end_handle = handle + 1;

I was not sure about that. Maybe we'd have to scan the whole handle range but you would need the service (not characteristic) to get that... Also I thought that the handle should be bound to the characteristic - not the whole service. What if you have different characteristics - I suppose each would have an individual handle, or not?

Sorry I did not spend more time - have some home improvement project going on currently also (bathroom renovations).

About debugging: What about simple outputs to the console? How do you check things like internal variables?

gfwilliams commented 7 years ago

Sorry I did not spend more time

Wow, don't apologise - what you've done has been a huge help. I didn't even reply to you for a week :)

And I know about how much time home improvement stuff can take...

I think the handle should be handle+1 -> handle+1 - at least that's what works for me. It seems that when Nordic do it they pass in value+1 and then the handle before the decl of the next characteristic - but I think it only needs one handle in the range so we might be safe.

However then there were other bugs in my code ;) I just got it working - I'll post up in a few minutes and you can see what I've done :)

What about simple outputs to the console?

Ahh, yes - I add jsiConsolePrintf calls.

I also tend to use the JS a lot - using global["\xff"] which gets at the hidden object with internal variables in, as well as trace(..) which dumps the internal datastructure of a variable.

Whizzard commented 7 years ago

Great. Just compiled + tested and it works! Still not sure what I missed on my attempts...

One thing: characteristic.readValue() returns a promise with the value as string. while event.characteristicvaluechanged returns the whole characteristic with target.value as Uint8Array(). Was this done intentionally? I would have expected the same datatype for both calls...

But actually great that it's working now. I can't believe it... :)

gfwilliams commented 7 years ago

Well, as usual with these things it was a few problems all at once. For debugging BLE stuff I also uncomment a jsiConsolePrintf right at the start of the IRQ handler - it gives me a better idea what's going on.

Good point about readValue - I'm trying to implement it to be like the Web Bluetooth spec, so readValue should ideally return the same as characteristicvaluechanged. There are no DataViews in Espruino so I've just been using Uint8Array.

I've just filed a bug for this: https://github.com/espruino/Espruino/issues/1091

edit: I should probably just add a DataView. It doesn't look that hard and it'd be a bit more spec compliant.

billsalt commented 7 years ago

Hi @gfwilliams, Is it to the state that I can/should test it out?

I'm starting out to implement a relay -- connect to a peripheral, receive notifications and data and act as a peripheral (simultaneously) to relay that data.

I'll be testing the whole thing on an nRF52832 dev board (PCA10040).

Thanks! Bill

gfwilliams commented 7 years ago

Hi. You should be able to flash the nrf52832 hex files from here: http://www.espruino.com/binaries/travis/master/

The usage is just as it is with Web Bluetooth.

However obviously I can't afford to actively support you if you hit problems on the nrf52832dk

billsalt commented 7 years ago

Thanks Gordon! Support issues totally understood; I have already downloaded that and installed it and it works just fine. Since it is designed to run on the puck-js, I would expect that.

I’m also running it on the u-blox NINA-B1 module (another 52832-based product). Just lacking the documentation to give it a try, so I’ll take a look. Do you have a good reference for Web Bluetooth?

Bill

-- Bill Saltzstein Code Blue Communications, Inc.

billsalt@consultcodeblue.com www.consultcodeblue.com http://www.linkedin.com/in/billsaltzstein (LinkedIn)

+1 425-442-5854 Skype: billsaltz

On Mar 22, 2017, at 1:30 AM, Gordon Williams notifications@github.com wrote:

Hi. You should be able to flash the nrf52832 hex files from here: http://www.espruino.com/binaries/travis/master/ http://www.espruino.com/binaries/travis/master/ The usage is just as it is with Web Bluetooth.

However obviously I can't afford to actively support you if you hit problems on the nrf52832dk

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/espruino/Espruino/issues/959#issuecomment-288330280, or mute the thread https://github.com/notifications/unsubscribe-auth/ABXIyQAHj6Ivm3vrnBZ2ZRd3MvRCpcbhks5roNwegaJpZM4KgPm_.

gfwilliams commented 7 years ago

Realistically this is it: https://webbluetoothcg.github.io/web-bluetooth/

Or use scripts/build_docs to make your own Espruino reference - but when 1v92 is released the docs for startNotifications will be in there