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

BLE/HRM : Fix #4661 #4662

Merged
merged 2 commits into from Jul 24, 2017

Conversation

Projects
None yet
5 participants
@Nodraak
Contributor

Nodraak commented Jun 28, 2017

  • Contributer agreement: Signed
  • Description: Fix issue #4661
  • Status: Ready
@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jun 28, 2017

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.

@0xc0170 0xc0170 requested a review from pan- Jun 29, 2017

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jun 29, 2017

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

This comment has been minimized.

Contributor

Nodraak commented Jul 4, 2017

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

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jul 4, 2017

@pan-

pan- requested changes Jul 4, 2017 edited

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.

features/FEATURE_BLE/ble/services/HeartRateService.h Outdated
controlPoint(GattCharacteristic::UUID_HEART_RATE_CONTROL_POINT_CHAR, &controlPointValue)
{
static uint8_t clientCharConfBytes[2] = {0, 0}; // clientCharacteristicConfiguration - length is 16 bits
static GattAttribute clientCharConf(BLE_UUID_DESCRIPTOR_CLIENT_CHAR_CONFIG, clientCharConfBytes, 2, 2, false); // clientCharacteristicConfiguration

This comment has been minimized.

@pan-

pan- Jul 4, 2017

Member

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

features/FEATURE_BLE/ble/services/HeartRateService.h Outdated
controlPoint(GattCharacteristic::UUID_HEART_RATE_CONTROL_POINT_CHAR, &controlPointValue) {
controlPoint(GattCharacteristic::UUID_HEART_RATE_CONTROL_POINT_CHAR, &controlPointValue)
{
static uint8_t clientCharConfBytes[2] = {0, 0}; // clientCharacteristicConfiguration - length is 16 bits

This comment has been minimized.

@pan-

pan- Jul 4, 2017

Member

same as above

@pan- pan- added the do not merge label Jul 4, 2017

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jul 4, 2017

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-

This comment has been minimized.

Member

pan- commented Jul 4, 2017

@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.

@pan- pan- added needs: work and removed do not merge labels Jul 4, 2017

@Nodraak Nodraak force-pushed the Nodraak:fix/4661 branch to 27901f2 Jul 4, 2017

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jul 4, 2017

Great, thanks a lot for the cristal clear feedback !

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

@pan-

pan- approved these changes Jul 4, 2017

Thanks for the changes.

@pan- pan- removed the needs: work label Jul 4, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jul 4, 2017

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jul 6, 2017

What is the status here ? CI is ok

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 6, 2017

We need to run the Austin CI

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 6, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 741

Test failed!

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jul 7, 2017

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-

This comment has been minimized.

Member

pan- commented Jul 7, 2017

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

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Jul 7, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jul 10, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 11, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 762

Build Prep failed!

@pan-

This comment has been minimized.

Member

pan- commented Jul 11, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 11, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 11, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 780

Example Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 14, 2017

/morph test

1 similar comment
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 17, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 18, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 826

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jul 18, 2017

@theotherjimmy theotherjimmy merged commit 766b240 into ARMmbed:master Jul 24, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@theotherjimmy theotherjimmy changed the title from [BLE/HRM] Fix #4661 to BLE/HRM : Fix #4661 Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment