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

Feature/blecsc app: Cycling Speed and Cadence app #819

Merged
merged 9 commits into from Feb 20, 2018

Conversation

mjurczak
Copy link
Contributor

Added a new example BLE app implementing Cycling Speed and Cadence service.

The app was build from blehr app and may have inherited some issues. I consider implementation of Cycling Power sample app as well after this.

Tested with Polar V650.

@mjurczak
Copy link
Contributor Author

One comment. I did not figure out how to implement one SC control point requirement:

"(...)Collector writes an Op Code to the SC Control Point while the SC Control Point
characteristic is not configured for indication (i.e. via the Client Characteristic
Configuration descriptor), it will receive an ATT Error Response with the Application
error code set to Client Characteristic Configuration Descriptor Improperly Configured"

It is fairly easy to check if indication is configured. But I could not find appropriate error code and I'm not sure what is the right way to pass ATT error response in such case.

Sensor location opcodes handled by SC control point only if multiple sensor locations feature is enabled.
Copy link
Contributor

@andrzej-kaczmarek andrzej-kaczmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks! I have just few comments.

As for ATT error, this one is not defined in any header since it's specific for CSCP profile - see section 1.6. Just make local #define with proper numeric value and pass it to API call.

- net/nimble/host/services/gap
- net/nimble/host/services/gatt
- net/nimble/host/store/config
- net/nimble/transport/ram
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use net/nimble/transport package instead - this way other transport can be configured via syscfg setting without need to change app code (useful for sample apps since they may be run on different targets)

(see 2e59449 for reference)

};

static void
store_le32_as_u8_arr(uint32_t val, uint8_t * arr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os/endian.h already defines get/put for le16, le32 and le64

@@ -37,6 +37,8 @@ struct ble_hs_cfg;

const char *ble_svc_gap_device_name(void);
int ble_svc_gap_device_name_set(const char *name);
int ble_svc_gap_device_appearance(void);
int ble_svc_gap_device_appearance_set(const uint16_t appearance);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually make this a syscfg value (BLE_SVC_GAP_APPEARANCE)
the reason for this is that appearance is read-only so it cannot (and should not) be changed in runtime.
getter is ok though.


case BLE_GAP_EVENT_ADV_COMPLETE:
BLECSC_LOG(INFO, "adv complete\n");
blecsc_advertise();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSCP does not allow concurrent connections to single sensor so we should not restart advertising here - only after disconnection and failed connection

Copy link
Contributor Author

@mjurczak mjurczak Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.
The spec is a bit ambiguous: "2.3 (...) There are no concurrency limitations or restrictions for the Collector and CSC Sensor roles imposed by this profile."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means a device can implement different roles concurrently so it ca be both a sensor and a collector, which probably does not make sense, but iirc only a handful of profiles impose any concurrency restrictions only if necessary (like HoGP).

CSC profile specific ATT error added.
Custom endianness handling functions replaced with system functions.
Device appearance moved to syscfg.
@andrzej-kaczmarek
Copy link
Contributor

Looks good to me after changes, thanks!

@sjanc sjanc merged commit 200cd39 into apache:master Feb 20, 2018
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.

None yet

3 participants