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: Mesh: Rename adv relay to adv simultaneous #48903

Closed
wants to merge 2 commits into from

Conversation

LingaoM
Copy link
Collaborator

@LingaoM LingaoM commented Aug 11, 2022

Since notice that simultaneous advertising is not only used
by relay message, provision over pb-adv can also be used.
so it was changed to a more general name.

Signed-off-by: Lingao Meng menglingao@xiaomi.com

Since notice that simult advertising is not only used
by relay message, provision over pb-adv can also be used.
so it was changed to a more general name.

Signed-off-by: Lingao Meng <menglingao@xiaomi.com>
@LingaoM LingaoM changed the title Bluetooth: Mesh: Rename adv relay to adv parallel Bluetooth: Mesh: Rename adv relay to adv simultaneous Aug 11, 2022
@LingaoM LingaoM force-pushed the rename_adv_relay branch 3 times, most recently from 8c03d31 to 13925f8 Compare August 11, 2022 04:33
Add config for pb-adv retransmit

Signed-off-by: Lingao Meng <menglingao@xiaomi.com>
@@ -245,7 +245,7 @@ static struct net_buf *adv_buf_create(uint8_t retransmits)
{
struct net_buf *buf;

buf = bt_mesh_adv_create(BT_MESH_ADV_PROV, BT_MESH_LOCAL_ADV,
buf = bt_mesh_adv_create(BT_MESH_ADV_PROV, BT_MESH_SIMUL_ADV,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate on this change? What does it improve or fix? Also, what if CONFIG_BT_MESH_SIMULT_ADV_SETS > 1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, the same question from me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can try for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm upload ellisys capture for this change, please take a look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With such configuration, would it make more sense to stop advertising if an ack is received? Then only one adv set is needed.

Copy link
Collaborator Author

@LingaoM LingaoM Aug 22, 2022

Choose a reason for hiding this comment

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

That true, but actually we need at least two advertisers sets, since ack pdu not ack by peer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the idea is cool, but stopping the advertiser would be preferable as it will not generate so much traffic. I may guess the drawback of using 1 adv set is that and advertisement may already be scheduled by the time stop is called thus delaying the next message, but this I think will be an issue anyway even if multiple sets are used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... How about the scenario with high-density traffic? Won't compete relayed traffic and provisioning for the same source? How about the reliability of the provisioning process? Seems that even if the provisioner isn't targeted for any model communication it becomes less responsible as a provisioner due to high-density traffic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The balance between high throughput and performance needs to be determined by the user.

@@ -52,13 +52,41 @@ config BT_MESH_UNPROV_BEACON_INT
This option specifies the interval (in seconds) at which the
device sends unprovisioned beacon.

if BT_MESH_PB_ADV
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to hide PB-ADV parameters under menuconfig?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this parameters only valid for PB-ADV.

Comment on lines +65 to +69
int "Provision pdu retransmit count"
default 0
range 0 7
help
Controls the number of retransmissions of original transaction pdu,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion to add "Link Open" to the description:

Suggested change
int "Provision pdu retransmit count"
default 0
range 0 7
help
Controls the number of retransmissions of original transaction pdu,
int "Link Open and Transaction PDU retransmit count"
default 0
range 0 7
help
Controls the number of retransmissions of original Link Open and Transaction PDU,

Comment on lines +73 to +77
int "Provision transaction ack retransmit count"
default 2
range 0 7
help
Controls the number of retransmissions of original transaction ack,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion to add "Link Ack" to the description:

Suggested change
int "Provision transaction ack retransmit count"
default 2
range 0 7
help
Controls the number of retransmissions of original transaction ack,
int "Link Ack and Transaction Ack retransmit count"
default 2
range 0 7
help
Controls the number of retransmissions of original Link Open and Transaction Acknowledgment PDU,

Comment on lines +411 to +412
config BT_MESH_SIMULT_ADV_SETS
int "Maximum of simultaneous message support"
Copy link
Collaborator

@omkar3141 omkar3141 Aug 22, 2022

Choose a reason for hiding this comment

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

Suggestion.

Suggested change
config BT_MESH_SIMULT_ADV_SETS
int "Maximum of simultaneous message support"
config BT_MESH_MULTI_ADV_SETS
int "Maximum number of parallel advertising sets that can be used by the Bluetooth Mesh stack"

Copy link
Collaborator

@PavelVPV PavelVPV left a comment

Choose a reason for hiding this comment

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

As discussed here #48903 (comment) this approach generates a lot of unnecessary traffic. Probably, the correct way would be to find how to stop provisioning PDU advertisements.

@PavelVPV
Copy link
Collaborator

PavelVPV commented Sep 6, 2022

@LingaoM are you planning to do anything with this PR?

@PavelVPV
Copy link
Collaborator

@LingaoM are you planning to do anything with this PR?

@LingaoM any updates on this?

@zephyrproject-rtos zephyrproject-rtos locked and limited conversation to collaborators Sep 30, 2022
@LingaoM LingaoM closed this Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants