Skip to content

Bluetooth: Immediate Alert Service#43574

Merged
carlescufi merged 1 commit intozephyrproject-rtos:mainfrom
szymon-czapracki:ias
Apr 12, 2022
Merged

Bluetooth: Immediate Alert Service#43574
carlescufi merged 1 commit intozephyrproject-rtos:mainfrom
szymon-czapracki:ias

Conversation

@szymon-czapracki
Copy link
Contributor

@szymon-czapracki szymon-czapracki commented Mar 9, 2022

Add Immediate Alert Service
@github-actions github-actions bot added area: API Changes to public APIs area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Samples Samples labels Mar 9, 2022
@szymon-czapracki
Copy link
Contributor Author

szymon-czapracki commented Mar 9, 2022

@MariuszSkamra FYI
@szymon-czapracki szymon-czapracki force-pushed the ias branch 4 times, most recently from b2fbbea to a4c1fe0 Compare March 9, 2022 12:20
@zephyrbot zephyrbot requested review from Thalley and asbjornsabo March 9, 2022 12:54
Thalley
Thalley requested changes Mar 9, 2022
View reviewed changes
include/linker/common-rom.ld Outdated
Comment on lines 139 to 142
Copy link
Contributor

@Thalley Thalley Mar 9, 2022

Choose a reason for hiding this comment

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

Not at all opposed to allow statically registered callbacks, but I am not sure how this is properly added.

I see that for e.g. bt_conn_cb, it is added to not only this file, but also some of the ESP32 linker.ld files.

Do you know how this is properly added? If adding it to only here, does it then not work on e.g. the ESP32 devices?
Copy link
Contributor

@sylvioalves sylvioalves Mar 9, 2022

Choose a reason for hiding this comment

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

Unfortunately we have to manually add this in ESP32/S2/C3 as well. We are still to figure out a way to overcome this.
Copy link
Contributor

@sylvioalves sylvioalves Mar 17, 2022

Choose a reason for hiding this comment

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

Just an update. This issue regarding ESP32 linker script would/will be fixed in this #43659.
Thalley
Thalley requested changes Mar 10, 2022
View reviewed changes
Thalley
Thalley requested changes Mar 15, 2022
View reviewed changes
asbjornsabo
asbjornsabo reviewed Mar 17, 2022
View reviewed changes
include/bluetooth/services/ias.h Outdated
Copy link
Contributor

@asbjornsabo asbjornsabo Mar 17, 2022

Choose a reason for hiding this comment

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

What is the advantage of doing it this way?
Copy link
Contributor

@Thalley Thalley Mar 17, 2022

Choose a reason for hiding this comment

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

Callbacks are stored in ROM instead of RAM :)
Copy link
Contributor

@asbjornsabo asbjornsabo Mar 23, 2022

Choose a reason for hiding this comment

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

That much I got ;-)
subsys/bluetooth/services/ias.c Outdated
Copy link
Contributor

@asbjornsabo asbjornsabo Mar 17, 2022

Choose a reason for hiding this comment

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

Why is it called a sample? How about "implementation"?
subsys/bluetooth/services/ias.c Outdated
Copy link
Contributor

@asbjornsabo asbjornsabo Mar 17, 2022

Choose a reason for hiding this comment

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

What if the ongoing alert has been stopped by the user? In that case, this should start a new one, rather than just returning.
@MariuszSkamra MariuszSkamra self-requested a review March 21, 2022 13:24
alwa-nordic
alwa-nordic reviewed Mar 21, 2022
View reviewed changes
subsys/bluetooth/services/ias.c Outdated
Comment on lines 50 to 51
Copy link
Contributor

@alwa-nordic alwa-nordic Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Setting alert_level should be outside the loop.
alwa-nordic
alwa-nordic reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@alwa-nordic alwa-nordic left a comment

Choose a reason for hiding this comment

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

The IAS spec is not clear on behavior when multiple peers are connected. But I think clearing the alert when any peer disconnects is wrong.

The Alert Level characteristic is not readable. This makes me think that each GATT client should have a separate alert level.
Thalley
Thalley requested changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

LGTM on the condition that we mark it as experimental in the Kconfig as well, so that we may revisit the API later more easily
subsys/bluetooth/services/Kconfig.ias Outdated
Comment on lines 6 to 7
Copy link
Contributor

@Thalley Thalley Apr 7, 2022

Choose a reason for hiding this comment

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

Needs a select EXPERIMENTAL
Copy link
Member

@jhedberg jhedberg Apr 7, 2022

Choose a reason for hiding this comment

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

There should also be " [EXPERIMENTAL]" at the end of the description.
Copy link
Contributor Author

@szymon-czapracki szymon-czapracki Apr 7, 2022

Choose a reason for hiding this comment

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

Done
@szymon-czapracki szymon-czapracki force-pushed the ias branch 2 times, most recently from b52abe7 to a601be6 Compare April 7, 2022 12:02
Thalley
Thalley previously approved these changes Apr 7, 2022
View reviewed changes
@MariuszSkamra MariuszSkamra self-requested a review April 8, 2022 09:16
MariuszSkamra
MariuszSkamra previously approved these changes Apr 8, 2022
View reviewed changes
jhedberg
jhedberg previously approved these changes Apr 8, 2022
View reviewed changes
asbjornsabo
asbjornsabo previously approved these changes Apr 8, 2022
View reviewed changes
@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Apr 8, 2022

@szymon-czapracki please rebase
@szymon-czapracki
Bluetooth: Immediate Alert Service
83ec366 
This commits adds IAS for zephyr bluetooth.

Signed-off-by: Szymon Czapracki <szymon.czapracki@codecoup.pl>
@szymon-czapracki
Copy link
Contributor Author

szymon-czapracki commented Apr 11, 2022

@szymon-czapracki please rebase

Done
@carlescufi carlescufi merged commit af2e781 into zephyrproject-rtos:main Apr 12, 2022
@Thalley Thalley mentioned this pull request Aug 25, 2022
Closed
@Thalley Thalley mentioned this pull request Nov 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth area: Samples Samples

10 participants

@szymon-czapracki @Thalley @mbolivar-nordic @jhedberg @rymanluk @MariuszSkamra @sylvioalves @asbjornsabo @alwa-nordic @carlescufi