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

Add Link Loss Service #14

Merged
merged 2 commits into from
Dec 10, 2020
Merged

Add Link Loss Service #14

merged 2 commits into from
Dec 10, 2020

Conversation

noonfom
Copy link

@noonfom noonfom commented Nov 26, 2020

The Link Loss Service uses the Alert Level characteristic, as defined in the Bluetooth SIG Assigned Numbers, to cause an
alert in the device when the link is lost.

Reviewers

@pan-
@paul-szczepanek-arm

Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

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

Good work, please fill out the doxy.

services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/src/LinkLossService/LinkLossService.cpp Outdated Show resolved Hide resolved
services/src/LinkLossService/LinkLossService.cpp Outdated Show resolved Hide resolved
services/src/LinkLossService/LinkLossService.cpp Outdated Show resolved Hide resolved
@paul-szczepanek-arm
Copy link
Member

We need to decide on some common folder structure for the service files. @pan- @AGlass0fMilk ?

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

Thanks for the submission @noonfom .

One thing: as a general rule of thumb consider doing in class initialization for class variables that do not depend on the constructor, in this case _event_queue_handle, _alert_timeout, ...

services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
services/src/LinkLossService/LinkLossService.cpp Outdated Show resolved Hide resolved
services/src/LinkLossService/LinkLossService.cpp Outdated Show resolved Hide resolved
services/inc/LinkLossService.h Outdated Show resolved Hide resolved
@noonfom noonfom force-pushed the experimental-services branch from fb1f2c9 to ddc20f6 Compare December 1, 2020 15:43
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

We're almost there, we just need to cleanup the value of _event_queue_handle when it is cancelled (or triggered).

services/src/LinkLossService/LinkLossService.cpp Outdated Show resolved Hide resolved
@noonfom noonfom force-pushed the experimental-services branch from ddc20f6 to 6eac2a4 Compare December 2, 2020 18:44
Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

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

Just a couple more issues and please add an mbed_lib.json, see:
#15

* The on_alert_requested() and on_alert_end() event handlers should be overridden
* by your application. For example, in the former case, you could alert the user by
* flashing lights, making noises, moving, etc.
*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*
* This service requires access to gap events. Please register a ChainableGapEventHandler with Gap and pass it to this service.

void LinkLossService::onDisconnectionComplete(const ble::DisconnectionCompleteEvent &event)
{
AlertLevel level = get_alert_level();
if (_alert_handler != nullptr && event.getReason() == ble::disconnection_reason_t::CONNECTION_TIMEOUT && level != AlertLevel::NO_ALERT) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (_alert_handler != nullptr && event.getReason() == ble::disconnection_reason_t::CONNECTION_TIMEOUT && level != AlertLevel::NO_ALERT) {
if (_alert_handler != nullptr && event.getReason() == ble::disconnection_reason_t::CONNECTION_TIMEOUT && level != AlertLevel::NO_ALERT && !_in_alert) {

{
const uint8_t level = *write_request->data;

if (level <= *reinterpret_cast<uint8_t *>(AlertLevel::HIGH_ALERT)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (level <= *reinterpret_cast<uint8_t *>(AlertLevel::HIGH_ALERT)) {
if (level <= static_cast<uint8_t>(AlertLevel::HIGH_ALERT)) {

@noonfom noonfom force-pushed the experimental-services branch 2 times, most recently from abe8dc4 to 4c3494f Compare December 3, 2020 11:03
@noonfom noonfom force-pushed the experimental-services branch from e474742 to 8ee4f22 Compare December 3, 2020 16:34
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

Minor change request when invalid data are written from a peer.
After this it should be good to go.


if (level <= (uint8_t)(AlertLevel::HIGH_ALERT)) {
set_alert_level((AlertLevel) level);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you return an error if the alert is out of range ?

@pan- pan- merged commit 70bda92 into ARMmbed:main Dec 10, 2020
noonfom pushed a commit to noonfom/mbed-os-experimental-ble-services that referenced this pull request Dec 16, 2020
* Add Link Loss Service

* Set auth reply if alert level is out of range
@pan- pan- mentioned this pull request Jan 20, 2021
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.

3 participants