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

nRF5x: add BluetoothRemoteGATTCharacteristic.startNotifications #959

Closed
gfwilliams opened this issue Oct 25, 2016 · 17 comments
Closed

nRF5x: add BluetoothRemoteGATTCharacteristic.startNotifications #959

gfwilliams opened this issue Oct 25, 2016 · 17 comments
Labels

Comments

@gfwilliams
Copy link
Member

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

@Whizzard
Copy link

Whizzard commented Feb 6, 2017

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

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

Whizzard commented Feb 12, 2017

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

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' :)

wilberforce pushed a commit to wilberforce/Espruino that referenced this issue Feb 16, 2017
@Whizzard
Copy link

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

Whizzard commented Feb 19, 2017

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:

  • switching ble tasks to migrate from descriptor discovery to actually start notifications without returning the promise inbetween
  • the global for holding the characteristic during discovery for adding the cccd-handle later

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

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

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

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

Whizzard commented Feb 27, 2017

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

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

Whizzard commented Feb 28, 2017

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

gfwilliams commented Feb 28, 2017

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

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

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

billsalt commented Mar 22, 2017 via email

@gfwilliams
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants