Skip to content

Conversation

iliar
Copy link
Contributor

@iliar iliar commented Sep 4, 2025

This commit adds macros for packing and extracting PAST service data for BAP.
Fixes #95337

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.

Thankss for your contribution. Some formatting comments, but mostly requests to add users of this new macro so that we can verify that it works :)

@iliar iliar force-pushed the PAST_Service_Data branch from dec4870 to e735249 Compare September 9, 2025 14:00
@zephyrbot zephyrbot added the area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests label Sep 9, 2025
@iliar iliar force-pushed the PAST_Service_Data branch 2 times, most recently from 16db4b0 to c3052f1 Compare September 9, 2025 16:01
@kartben
Copy link
Contributor

kartben commented Sep 22, 2025

@iliar please rebase on main

@iliar iliar requested a review from Thalley September 24, 2025 20:28
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.

I think there are a few mistakes that needs to be fixed

@iliar iliar force-pushed the PAST_Service_Data branch from 02a6b62 to 2f9748d Compare October 9, 2025 19:33
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Oct 9, 2025
@zephyrbot zephyrbot requested a review from nashif October 9, 2025 19:35
@iliar iliar force-pushed the PAST_Service_Data branch from 2f9748d to 2e15b49 Compare October 9, 2025 21:11
@iliar iliar force-pushed the PAST_Service_Data branch 2 times, most recently from 884d00c to 359eaf7 Compare October 10, 2025 22:28
This commit adds macros for packing PAST service data for BAP.

Signed-off-by: Iliar Rabet <[email protected]>
@iliar iliar force-pushed the PAST_Service_Data branch from 359eaf7 to 670795e Compare October 12, 2025 13:31
Copy link

Comment on lines +186 to +187
((uint16_t)(_src_id) & 0xFFU) << 8U \
| ((uint16_t)(_past_flags) & 0xFFU)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a 100-line maximum in Zephy, so we could do

Suggested change
((uint16_t)(_src_id) & 0xFFU) << 8U \
| ((uint16_t)(_past_flags) & 0xFFU)
((uint16_t)(_src_id) & 0xFFU) << 8U | ((uint16_t)(_past_flags) & 0xFFU)

else I would put the | on the first line

Suggested change
((uint16_t)(_src_id) & 0xFFU) << 8U \
| ((uint16_t)(_past_flags) & 0xFFU)
((uint16_t)(_src_id) & 0xFFU) << 8U | \
((uint16_t)(_past_flags) & 0xFFU)

IS_ENABLED(CONFIG_BT_PER_ADV_SYNC_TRANSFER_SENDER)) {
uint8_t past_flags = BT_BAP_PAST_FLAG_NO_MATCH_ADV_EXT_IND;

bt_le_ext_adv_get_info(ext_adv, &info);
Copy link
Contributor

Choose a reason for hiding this comment

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

Our coding guidelines require that all functions that may return an error, shall have the return value checked, so this must be

Suggested change
bt_le_ext_adv_get_info(ext_adv, &info);
err = bt_le_ext_adv_get_info(ext_adv, &info);

In cases like this where it should never fail unless something terribly has happened, I'd test this with an ASSERT

Suggested change
bt_le_ext_adv_get_info(ext_adv, &info);
err = bt_le_ext_adv_get_info(ext_adv, &info);
__ASSERT(err == 0, "Failed to get adv info: %d", err);

This also applies to the other cases in this PR

Comment on lines +186 to +191
uint8_t src_id = info.sid;


err = bt_le_per_adv_sync_transfer(g_pa_sync,
conn,
BT_BAP_PAST_SERVICE_DATA(past_flags, src_id));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint8_t src_id = info.sid;
err = bt_le_per_adv_sync_transfer(g_pa_sync,
conn,
BT_BAP_PAST_SERVICE_DATA(past_flags, src_id));
err = bt_le_per_adv_sync_transfer(g_pa_sync,
conn,
BT_BAP_PAST_SERVICE_DATA(past_flags, info.src_id));

Comment on lines +541 to +546
bt_le_per_adv_sync_get_info(g_pa_sync, &info);
uint8_t src_id = info.sid;

err = bt_le_per_adv_sync_transfer(g_pa_sync,
conn,
BT_BAP_PAST_SERVICE_DATA(past_flags, src_id));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bt_le_per_adv_sync_get_info(g_pa_sync, &info);
uint8_t src_id = info.sid;
err = bt_le_per_adv_sync_transfer(g_pa_sync,
conn,
BT_BAP_PAST_SERVICE_DATA(past_flags, src_id));
bt_le_per_adv_sync_get_info(g_pa_sync, &info);
err = bt_le_per_adv_sync_transfer(g_pa_sync,
conn,
BT_BAP_PAST_SERVICE_DATA(past_flags, info.src_id));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for feedback.
but I am getting this error:
error: ‘struct bt_le_per_adv_sync_info’ has no member named ‘src_id’

The defintion looks like this:

/** @brief Periodic advertising set info structure. */
struct bt_le_per_adv_sync_info {
	/** Periodic Advertiser Address */
	bt_addr_le_t addr;

	/** Advertising Set Identifier, valid range @ref BT_GAP_SID_MIN to @ref BT_GAP_SID_MAX. */
	uint8_t sid;

	/** Periodic advertising interval (N * 1.25 ms) */
	uint16_t interval;

	/** Advertiser PHY (see @ref bt_gap_le_phy). */
	uint8_t phy;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is on me. It's not the sid that we need, but the src_id of the receive state. bt_le_per_adv_sync_get_info and bt_le_ext_adv_get_info shouldn't be used for this purpose. Sorry for a bad recommendation when I suggested that :D

We need to store the src_id of the receive state somewhere and then use that use. I confused myself with sid, which is not the same as src_id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Audio area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth area: Tests Issues related to a particular existing or missing test

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

Bluetooth: BAP: Add helper macro to set PAST service data

4 participants