bluetooth: tester: MICP Client tests #60045

piotrnarajowski · 2023-07-05T13:16:37Z

Add support for MICP Client tests

github-actions

Hello @piotrnarajowski, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

sjanc · 2023-07-12T11:57:31Z

include/zephyr/bluetooth/audio/micp.h

@@ -271,6 +271,36 @@ int bt_micp_mic_ctlr_mute_get(struct bt_micp_mic_ctlr *mic_ctlr);
 * @return 0 if success, errno on failure.
 */
 int bt_micp_mic_ctlr_cb_register(struct bt_micp_mic_ctlr_cb *cb);
+
+/**


I think that those APIs should be under BLUETOOTH_TESTING (those are upper tester interfaces only)

(see include/zephyr/bluetooth/testing.h)

These should not be added at all :)

None of our other tests depend on the GATT handle - Why would these?

sjanc · 2023-07-12T11:59:50Z

subsys/bluetooth/audio/micp_mic_ctlr.c

@@ -19,6 +19,7 @@
 #include <zephyr/bluetooth/conn.h>
 #include <zephyr/bluetooth/gatt.h>
 #include <zephyr/bluetooth/audio/micp.h>
+#include <../../subsys/bluetooth/audio/aics_internal.h>


why is this needed?

sjanc · 2023-07-12T12:03:34Z

tests/bluetooth/tester/src/btp_micp.c

+#include "btp/btp.h"
+
+static struct bt_micp_mic_ctlr *mic_ctlr;
+#if defined(CONFIG_BT_MICP_MIC_CTLR_AICS)


nitpick: empty line before #if, no emptu line before #endif

sjanc · 2023-07-12T12:06:11Z

tests/bluetooth/tester/src/btp_micp.c

+	(void) bt_conn_get_info(conn, &info);
+	bt_addr_le_copy(&ev.address, info.le.dst);
+
+	ev.mute_handle = mute_handle;


sys_cpu_to_le16()

every uint16, uint32 and uint64 needs to be converted to LE when sending over BTP

sjanc · 2023-07-12T12:06:23Z

tests/bluetooth/tester/src/btp_micp.c

+	(void) bt_conn_get_info(conn, &info);
+	bt_addr_le_copy(&ev.address, info.le.dst);
+
+	ev.state_handle = state_handle;


sjanc · 2023-07-12T12:12:49Z

tests/bluetooth/tester/src/btp_micp.c

+	}
+
+	err = bt_micp_mic_ctlr_discover(conn, &mic_ctlr);
+


sjanc · 2023-07-12T12:15:19Z

tests/bluetooth/tester/src/btp_micp.c

+static uint8_t micp_aics_unmute(const void *cmd, uint16_t cmd_len,
+				void *rsp, uint16_t *rsp_len)
+{
+	LOG_DBG("Unmute");


should we verify that expected peer has profile connected? (BTP command has peer address)

The MICP commands will return -ENOTCONN if not connected (or more like -EINVAL since the GATT handles would not have been set yet if discovery hasn't been done, so it will still fail gracefully if not connected

sjanc · 2023-07-12T12:15:24Z

tests/bluetooth/tester/src/btp_micp.c

+
+static uint8_t micp_aics_mute(const void *cmd, uint16_t cmd_len,
+			      void *rsp, uint16_t *rsp_len)
+{


ditto and others command too

sjanc · 2023-07-12T12:16:49Z

tests/bluetooth/tester/src/btp_micp.c

+
+	err = bt_micp_mic_ctlr_cb_register(&micp_cbs);
+	if (err != 0) {
+		LOG_DBG("Failed to register callbacks: %d", err);


return BTP_STATUS_FAILED;

we don't want to initialize service if those failed

sjanc · 2023-07-12T12:18:29Z

tests/bluetooth/tester/src/btp_micp.c

+
+uint8_t tester_unregister_micp(void)
+{
+	return BTP_STATUS_SUCCESS;


maybe call bt_micp_mic_ctlr_cb_register(NULL); here to "unregister" callbacks?

Thalley

Mostly code style and minor comment. The big comment is that these tests should not depend on the GATT handle if it can be avoided.

Thalley · 2023-07-12T13:35:20Z

include/zephyr/bluetooth/audio/micp.h

@@ -271,6 +271,36 @@ int bt_micp_mic_ctlr_mute_get(struct bt_micp_mic_ctlr *mic_ctlr);
 * @return 0 if success, errno on failure.
 */
 int bt_micp_mic_ctlr_cb_register(struct bt_micp_mic_ctlr_cb *cb);
+
+/**


These should not be added at all :)

None of our other tests depend on the GATT handle - Why would these?

tests/bluetooth/tester/overlay-le-audio.conf

tests/bluetooth/tester/src/btp_micp.c

Thalley · 2023-07-12T13:43:28Z

tests/bluetooth/tester/src/btp_micp.c

+static uint8_t micp_aics_unmute(const void *cmd, uint16_t cmd_len,
+				void *rsp, uint16_t *rsp_len)
+{
+	LOG_DBG("Unmute");


The MICP commands will return -ENOTCONN if not connected (or more like -EINVAL since the GATT handles would not have been set yet if discovery hasn't been done, so it will still fail gracefully if not connected

tests/bluetooth/tester/src/btp_micp.c

include/zephyr/bluetooth/audio/micp.h

subsys/bluetooth/audio/micp_internal.h

subsys/bluetooth/audio/micp_mic_ctlr.c

tests/bluetooth/tester/CMakeLists.txt

tests/bluetooth/tester/src/btp/btp.h

tests/bluetooth/tester/src/btp/btp_aics.h

tests/bluetooth/tester/src/btp/btp_micp.h

mkasenberg · 2023-08-09T09:29:13Z

tests/bluetooth/tester/src/btp_micp.c

+
+	if (err != 0) {
+		LOG_DBG("Discovery failed (%d)", err);
+		return BTP_MICP_DISCOVERY_STATUS_FAILED;


Build warning: 'return' with a value, in function returning void

Suggested change

return BTP_MICP_DISCOVERY_STATUS_FAILED;

return;

mkasenberg · 2023-08-09T09:41:11Z

tests/bluetooth/tester/src/btp_micp.c

+
+	err = bt_micp_mic_ctlr_mute_get(mic_ctlr);
+	if (err != 0) {
+		BTP_STATUS_FAILED;


Build warning: statement with no effect

Suggested change

BTP_STATUS_FAILED;

return BTP_STATUS_FAILED;

szymon-czapracki · 2023-08-18T14:04:52Z

tests/bluetooth/tester/src/btp_aics.c

+
+#define LOG_MODULE_NAME bttester_aics
+LOG_MODULE_REGISTER(LOG_MODULE_NAME, CONFIG_BTTESTER_LOG_LEVEL
+);


Move closing bracket into upper line.

tests/bluetooth/tester/src/btp_micp.c

tests/bluetooth/tester/overlay-le-audio.conf

tests/bluetooth/tester/src/btp_aics.c

tests/bluetooth/tester/src/btp_micp.c

Thalley

My comments have been resolved, but I still see a few unresolved comments from other reviews

MariuszSkamra

LGTM

mkasenberg · 2023-08-23T11:46:24Z

include/zephyr/bluetooth/audio/micp.h

@@ -20,6 +20,7 @@
 */

 #include <stdint.h>
+#include <../../subsys/bluetooth/audio/micp_internal.h>


Public header include/zephyr/bluetooth/audio/micp.h shall not include internal header ../../subsys/bluetooth/audio/micp_internal.h

This is needed for upper tester. Signed-off-by: Piotr Narajowski <piotr.narajowski@codecoup.pl>

Add support for MICP Client tests. Signed-off-by: Piotr Narajowski <piotr.narajowski@codecoup.pl>

Thalley · 2023-08-28T08:44:40Z

@carlescufi I think this can be merged without @sjanc approval (he is on OOO for 2 weeks it seems)

github-actions

Hi @piotrnarajowski!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

piotrnarajowski requested review from alwa-nordic, jhedberg, Vudentz, sjanc and Thalley as code owners July 5, 2023 13:16

zephyrbot added area: Bluetooth area: Bluetooth Audio labels Jul 5, 2023

zephyrbot requested review from asbjornsabo, Casper-Bonde-Bose, fredrikdanebjer, hermabe, kruithofa, larsgk, MariuszSkamra and szymon-czapracki July 5, 2023 13:17

zephyrbot assigned jhedberg Jul 5, 2023

github-actions bot reviewed Jul 5, 2023

View reviewed changes

Thalley assigned Thalley and unassigned jhedberg Jul 5, 2023

piotrnarajowski force-pushed the micp branch 2 times, most recently from 4fec3cc to 36efa0d Compare July 6, 2023 13:27

sjanc requested changes Jul 12, 2023

View reviewed changes

Thalley requested changes Jul 12, 2023

View reviewed changes

piotrnarajowski force-pushed the micp branch 4 times, most recently from 9cba7b3 to ea16167 Compare July 26, 2023 09:50

MariuszSkamra requested changes Aug 3, 2023

View reviewed changes

mkasenberg reviewed Aug 9, 2023

View reviewed changes

piotrnarajowski force-pushed the micp branch from ea16167 to 2917a40 Compare August 17, 2023 13:52

szymon-czapracki reviewed Aug 18, 2023

View reviewed changes

tests/bluetooth/tester/src/btp_micp.c Outdated Show resolved Hide resolved

piotrnarajowski force-pushed the micp branch 4 times, most recently from 22bca3f to cda4ccf Compare August 21, 2023 09:39

Thalley requested changes Aug 22, 2023

View reviewed changes

piotrnarajowski force-pushed the micp branch 2 times, most recently from 8827a07 to 9580ea6 Compare August 22, 2023 21:21

Thalley reviewed Aug 22, 2023

View reviewed changes

piotrnarajowski force-pushed the micp branch from 9580ea6 to 6aaaf1e Compare August 23, 2023 07:40

MariuszSkamra previously approved these changes Aug 23, 2023

View reviewed changes

Thalley previously approved these changes Aug 23, 2023

View reviewed changes

MariuszSkamra requested review from sjanc, mkasenberg and szymon-czapracki August 23, 2023 11:23

mkasenberg suggested changes Aug 23, 2023

View reviewed changes

piotrnarajowski added 2 commits August 23, 2023 13:51

bluetooth: audio: Add API to get MICP service ATT handles

a452486

This is needed for upper tester. Signed-off-by: Piotr Narajowski <piotr.narajowski@codecoup.pl>

bluetooth: tester: MICP Client tests

f8e0162

Add support for MICP Client tests. Signed-off-by: Piotr Narajowski <piotr.narajowski@codecoup.pl>

piotrnarajowski dismissed stale reviews from Thalley and MariuszSkamra via f8e0162 August 23, 2023 11:52

piotrnarajowski force-pushed the micp branch from 6aaaf1e to f8e0162 Compare August 23, 2023 11:52

mkasenberg approved these changes Aug 23, 2023

View reviewed changes

MariuszSkamra approved these changes Aug 23, 2023

View reviewed changes

Thalley approved these changes Aug 23, 2023

View reviewed changes

sjanc approved these changes Sep 8, 2023

View reviewed changes

carlescufi merged commit 925a8b6 into zephyrproject-rtos:main Sep 8, 2023
19 checks passed

github-actions bot reviewed Sep 8, 2023

View reviewed changes

piotrnarajowski deleted the micp branch September 8, 2023 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bluetooth: tester: MICP Client tests #60045

bluetooth: tester: MICP Client tests #60045

piotrnarajowski commented Jul 5, 2023

github-actions bot left a comment

sjanc Jul 12, 2023

Thalley Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

Thalley Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

sjanc Jul 12, 2023

Thalley left a comment

Thalley Jul 12, 2023

Thalley Jul 12, 2023

mkasenberg Aug 9, 2023

mkasenberg Aug 9, 2023

szymon-czapracki Aug 18, 2023

Thalley left a comment

MariuszSkamra left a comment

mkasenberg Aug 23, 2023

Thalley commented Aug 28, 2023

github-actions bot left a comment

bluetooth: tester: MICP Client tests #60045

bluetooth: tester: MICP Client tests #60045

Conversation

piotrnarajowski commented Jul 5, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a comment

Choose a reason for hiding this comment

MariuszSkamra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley commented Aug 28, 2023

github-actions bot left a comment

Choose a reason for hiding this comment