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

bluetooth: BAS: add battery critical status char to bas service #79966

Conversation

niym-ot
Copy link
Contributor

@niym-ot niym-ot commented Oct 17, 2024

Added the battery critical status char to bas service as per bas_1.1 spec.
Updated BSIM test for BAS service to test the INDs of BAS critical characteristic.

Comment on lines 33 to 34
help
Enable this option to include Battery Critical Status Characteristic.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is an issue for this entire Kconfig file, however the help text should be indented with 1 tab + 2 spaces (maybe add a second prior commit to this PR to fix that for the other options in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done Below PR fixes the indentation issue of BAS Kconfig
#79986

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. You could have included that as the first commit in this PR as well. Anyway, I marked the other PR as Trivial, so it should hopefully go in still today, and then you can rebase this one.

@niym-ot niym-ot force-pushed the Add_battery_critical_status_char_to_battery_service branch from 8498061 to dd9f664 Compare October 17, 2024 11:20
subsys/bluetooth/services/bas/bas.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas.c Outdated Show resolved Hide resolved
@niym-ot niym-ot force-pushed the Add_battery_critical_status_char_to_battery_service branch 2 times, most recently from 59e5101 to a10ea39 Compare October 18, 2024 07:19
#include <zephyr/logging/log.h>
LOG_MODULE_DECLARE(bas, CONFIG_BT_BAS_LOG_LEVEL);

#define BATTERY_CRITICAL_STATUS_CHAR_IDX 9
Copy link
Collaborator

Choose a reason for hiding this comment

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

For now this is fine, but if you start adding the other optional characteristics, then we can no longer rely on a static index.

An alternative is to use the UUID when calling bt_gatt_indicate which will be futureproof

subsys/bluetooth/services/bas/bas_bcs.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas_bcs.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas_bcs.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas_bcs.c Outdated Show resolved Hide resolved
Comment on lines 212 to 213

bt_bas_bcs_set_battery_critical_state(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If value == BT_BAS_BLS_CHARGE_LEVEL_UNKNOWN should we really modify the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, if battery is set to critical, then there shouldn't be an option to reset.
But in the case of testing , this reset will be handy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are correct, if battery is set to critical, then there shouldn't be an option to reset.

That is not what I meant :)

And also incorrect; it should definitely be possible to reset the battery state from critical to non-critical, e.g. if changing the battery.

@niym-ot niym-ot force-pushed the Add_battery_critical_status_char_to_battery_service branch from a10ea39 to f22209d Compare October 21, 2024 07:11
Copy link
Collaborator

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

Looks pretty good now. A few more comments

subsys/bluetooth/services/bas/bas_bls.c Outdated Show resolved Hide resolved
subsys/bluetooth/services/bas/bas_bls.c Outdated Show resolved Hide resolved
@niym-ot niym-ot force-pushed the Add_battery_critical_status_char_to_battery_service branch 2 times, most recently from 0f46f8f to e499d8b Compare October 21, 2024 12:18
@niym-ot niym-ot requested a review from Thalley October 21, 2024 12:18
Added the battery critical status char to bas service
as per bas_1.1 spec. Updated BSIM test for BAS service
to test the INDs of BAS critical characteristic.

Signed-off-by: Nithin Ramesh Myliattil <niym@demant.com>
@aescolar aescolar merged commit b717488 into zephyrproject-rtos:main Oct 22, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants