BLE/HRM : Fix #4661

[Nodraak]

• Contributer agreement: Signed
• Description: Fix issue [BLE] HeartRateService/Heart Rate Measurement should not be Read #4661
• Status: Ready

[Nodraak]

Maybe you don't want to merge this right now, as I might add other fixes related to this Heart Rate Service.

Let me explain:

I am currently comparing Cypress's autogenerated configuration with mbed's, and there is a difference in the Heart Rate Measurement characteristic configuration (I have removed unnecessary code that only adds noise):

Cypress:

// handle, id, permissions, last_handle, {attr_type, pointer}
{ xx, 0x2803u, 0x00100001u, xx, {{0x2A37u, NULL}} },    // characteristic
{ xx, 0x2A37u, 0x01100000u, xx, {{0x0002u, xx}} },      // measurement
{ xx, 0x2902u, 0x030A0101u, xx, {{0x0002u, xx}} },      // Client Characteristic Configuration

Mbed:

// handle, id, permissions, last_handle, {attr_type, pointer}
{ xx, 0x2803u, 0x00100001u, xx, {{0x2A37u, NULL}} },    // characteristic
{ xx, 0x2A37u, 0x01100000u, xx, {{0x0003u, xx}} },      // measurement

The BLE spec specifies that the Client Characteristic Configuration is Mandatory. Therefore, Mbed is not conforming to the spec.

I am not sure about the attr_type field, I'll have a more in depth look tomorrow.

[Nodraak]

My second commit add the missing descriptor to the Heart Rate Mesurement characteristic (source).

I spotted another issue, but it might not be worth fixing it, and I think I will be too lazy to fix it:
The Heart Rate Measurement Value depends on Flags: if Flags's bit 0 (LSbit) is 0, the value is uint8, the bit is 1, it is uint16. In mbed implementation, it is assummed that it is a uint16_t anyway (although there is code for uin8_t).

On my side, it is ready for merge (ping @pan-), you may replace the need:work by the needs:review tag

[Nodraak]

Hi ! Just doing a gentle up : my PR is ready :)

[pan-]

This PR is not necessary, CCCD should be automatically created and added to the GATT server whenever a characteristic with the notification and/or the indication flag is added to the GATT server.

If you doesn't observe this behavior then the port of BLE API that you are using have a defect. What version of the port are you using ?

Edit: The valuable part of this PR which can stay is the fix of the measurement characteristic properties:
GattCharacteristic::BLE_GATT_CHAR_PROPERTIES_READ | GattCharacteristic::BLE_GATT_CHAR_PROPERTIES_NOTIFY => GattCharacteristic::BLE_GATT_CHAR_PROPERTIES_NOTIFY.

[pan-]

static variables shouldn't be used, it prevent the instantiation of multiple, distinct heart rate services.

[pan-]

same as above

[Nodraak]

Hum, ok I got it, I though it was more logical for a service / characteristic to add its own CCCD if needed instead of relying on the underlying implementation. Also, the constructor of a GattCharacteristic accept a GattAttribute *descriptors[] argument.

Although the second commit of this PR is therefore not relevant, the first one still holds: what do you think? It fixes issue #4661.

[pan-]

@Nodraak I would approve the PR if it was just the first commit.

Hum, ok I got it, I though it was more logical for a service / characteristic to add its own CCCD if needed instead of relying on the underlying implementation. Also, the constructor of a GattCharacteristic accept a GattAttribute *descriptors[] argument.

Agreed that the documentation of the GattCharacteristic constructor should state explicitly that it is not required to allocate CCCD if either the notify or indicate flag is set.

[Nodraak]

Great, thanks a lot for the cristal clear feedback !

A rebased my branch against master, and integrated your comments, should be better now.

[pan-]

Thanks for the changes.

[Nodraak]

What is the status here ? CI is ok

[theotherjimmy]

We need to run the Austin CI

/morph test

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 741

Test failed!

[Nodraak]

So, two targets failed during the test tests-mbedmicro-rtos-mbed-basic: MAX32625MBED and NRF51_DK.

For MAX:

[1499362624.24][HTST][INF] Start: 1499362613.556000
[1499362624.24][HTST][INF] Finish: 1499362624.232000
[1499362624.24][HTST][INF] Total time taken: 10.676000
[1499362624.24][HTST][INF] Total drift/Max total drift: 0.676000/0.500000
[1499362624.24][HTST][INF] Average drift/Max average drift: 0.067600/0.050000
[1499362624.24][HTST][INF] FAIL: Total drift exceeded max total drift

And NRF:

[1499362405.43][HTST][INF] Start: 1499362396.290000
[1499362405.43][HTST][INF] Finish: 1499362405.245000
[1499362405.43][HTST][INF] Total time taken: 8.955000
[1499362405.43][HTST][INF] Total drift/Max total drift: -1.045000/0.500000
[1499362405.43][HTST][INF] Average drift/Max average drift: -0.104500/0.050000
[1499362405.43][HTST][INF] FAIL: Total drift exceeded max total drift

This does not seems related to my PR

[pan-]

It isn't related to the PR. Can we relaunch the tests ?

[Nodraak]

I don't know how this works, but let's try (I'm pretty sure one needs some sort of accreditation to start them, but let's try anyway):

/morph test

[0xc0170]

/morph test

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 762

Build Prep failed!

[pan-]

@0xc0170 I've looked at the failure, it is an internal CI error, could you relaunch the tests ?

[0xc0170]

/morph test

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 780

Example Build failed!

[0xc0170]

/morph test

[theotherjimmy]

/morph test

[mbed-bot]

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 826

All builds and test passed!