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: Controller: Simplified logic in dle_max_time_get #23370

Closed
wants to merge 2 commits into from

Conversation

dagbja
Copy link
Collaborator

@dagbja dagbja commented Mar 10, 2020

Combined RX and TX time calculation for different PHY cases

Signed-off-by: Dag Bjarvin Dag.Bjarvin@nordicsemi.no

Combined RX and TX time calculation for different PHY cases

Signed-off-by: Dag Bjarvin <Dag.Bjarvin@nordicsemi.no>
@dagbja dagbja requested review from cvinayak and kruithofa March 10, 2020 11:12
@dagbja dagbja self-assigned this Mar 10, 2020
u16_t rx_time = 0;
u16_t tx_time = 0;
u16_t bit = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

u8_t phy = 0;

Use phy elsewhere too

Copy link
Collaborator

Choose a reason for hiding this comment

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

or active_phy

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 mean use phy instead of bit?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. phy or phy_bitmask

#endif
#endif /* CONFIG_BT_CTLR_PHY_CODED */

if (!conn->common.fex_valid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this above the if at line 3157 and use else if to reduce the number of compare-and-branch instructions.

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

#else
feature_coded_phy = 0;
feature_phy_2m = 0;
if (conn->llcp_feature.features & BIT(BT_LE_FEAT_BIT_PHY_2M)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

else if

local supports Coded PHY, but peer may not support Coded PHY and supports 2M.

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'll let Andries comment on this


if (!conn->common.fex_valid ||
(!feature_coded_phy && !feature_phy_2m)) {
if (bit == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!phy

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the if (bit==0), and replace the 0 in the formulas for rx_time and tx_time with phy (or actual_phy)
Note that phy can only be non-zero if CONFIG_BT_CTLR_PHY is enabled, so the calculation for tx_time will still be correct

Fixed according to code review comments
Signed-off-by: Bjarvin <dag.bjarvin@nordicsemi.no>
u16_t rx_time = 0;
u16_t tx_time = 0;
u16_t bit = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

or active_phy


if (!conn->common.fex_valid ||
(!feature_coded_phy && !feature_phy_2m)) {
if (bit == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the if (bit==0), and replace the 0 in the formulas for rx_time and tx_time with phy (or actual_phy)
Note that phy can only be non-zero if CONFIG_BT_CTLR_PHY is enabled, so the calculation for tx_time will still be correct

}

if (bit == 0) {
if (phy_bitmask == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need the if-statement

u8_t phy_bitmask = 0;

if (!conn->common.fex_valid) {
phy_bitmask = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There were some other changes in the master branch
ref #23482 and #23557
In #23557 there are defines for the BIT(x) statements; rebase on master and use those, and replace 0 with the appropriate define as well (PHY_1M)

@kruithofa
Copy link
Collaborator

Please squash the commits into one

@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 2, 2020
@nashif nashif added the Stale label Sep 4, 2020
@github-actions github-actions bot closed this Sep 19, 2020
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.

4 participants