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: rfcomm: Support dynamic channel allocation #83785

Conversation

lylezhu2012
Copy link
Contributor

In the function bt_rfcomm_server_register(), the channel cannot be dynamic allocated. And only pre-set channel is supported.

Improve the function bt_rfcomm_server_register() to support the dynamic channel allocation if the passed channel is zero.

jhedberg
jhedberg previously approved these changes Jan 10, 2025
} else {
/* Check if given channel is already in use */
if (rfcomm_server_lookup_channel(server->channel)) {
LOG_DBG("Channel already registered");
Copy link
Contributor

Choose a reason for hiding this comment

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

better use LOG_WRN()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep the code block without any changes. I do not notice this.

I think it can be changed to LOG_WRN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@lylezhu2012 lylezhu2012 force-pushed the rfcomm_support_dynamic_channel_allocation branch from 22d63bd to b58abe9 Compare January 14, 2025 06:03
Comment on lines +209 to +216
for (; chan <= RFCOMM_CHANNEL_END; chan++) {
/* Check if given channel is already in use */
if (!rfcomm_server_lookup_channel(chan)) {
server->channel = chan;
LOG_DBG("Allocated channel 0x%02x for new server", chan);
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to add one lock to avoid that the same channel is allocated to two servers because higher priority task may preempt after the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the style of the Bleutooth stack. For example, the l2cap psm allocation of BLE does not have such protection. And servers is not protected for each updating.

So I think the case your description is not including in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Maybe it can be improved later.

@@ -199,15 +199,32 @@ rfcomm_sessions_lookup_bt_conn(struct bt_conn *conn)

int bt_rfcomm_server_register(struct bt_rfcomm_server *server)
{
if (server->channel < RFCOMM_CHANNEL_START ||
Copy link
Contributor

Choose a reason for hiding this comment

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

`RFCOMM_CHANNEL_START' definition can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to support the dynamic channel allocation if the passed channel is zero.

In the function `bt_rfcomm_server_register()`, the channel cannot be
dynamic allocated. And only pre-set channel is supported.

Improve the function `bt_rfcomm_server_register()` to support the
dynamic channel allocation if the passed channel is zero.

Signed-off-by: Lyle Zhu <[email protected]>
@lylezhu2012 lylezhu2012 force-pushed the rfcomm_support_dynamic_channel_allocation branch from b58abe9 to fa87918 Compare January 16, 2025 07:35
@kartben kartben merged commit 87b2a30 into zephyrproject-rtos:main Jan 17, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants