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

nimble: add sysconfig for enabling additional PHYs #1646

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

KKopyscinski
Copy link
Contributor

Three new configs are introduced: BLE_ADDITIONAL_PHY that enables additional PHY and two configs that specify what additional PHY shall be enabled (2M or CODED)

@@ -130,6 +130,25 @@ syscfg.defs:
This enables LE Connection Subrating feature
value: 0

BLE_ADDITIONAL_PHY:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name it simply 'BLE_PHY'

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need separate syscfg? just enable 2m or coded or both.

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's because it may be simplier to cut down code if neither is supported. So when you check something besides 1M is supported you do

#if MYNEWT_VAL(BLE_PHY)

instead of

#if MYNEWT_VAL(BLE_PHY_2M) || MYNEWT_VAL(BLE_PHY_CODED)

Copy link
Contributor

Choose a reason for hiding this comment

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

#define BLE_PHY_ENABLED (MYNEWT_VAL(BLE_PHY_2M) || MYNEWT_VAL(BLE_PHY_CODED))

...

#if BLE_PHY_ENABLED
#endif

alternatively smth like this should work

syscfg.defs.'BLE_PHY_2M || BLE_PHY_CODED':
    BLE_PHY: 1

we should avoid syscfg vals that do nothing except they have to be set because another syscfg is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me, I updated PR with second option. Config generates OK

This enables support for addtitional PHY (2M or CODED)
value: 0

BLE_2M_PHY:
Copy link
Contributor

Choose a reason for hiding this comment

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

BLE_PHY_2M

restrictions:
- 'BLE_ADDITIONAL_PHY if 1'

BLE_CODED_PHY:
Copy link
Contributor

Choose a reason for hiding this comment

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

BLE_PHY_CODED

Copy link
Contributor

@sjanc sjanc left a comment

Choose a reason for hiding this comment

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

and we should have same for host, ie
BLE_HS_PHY
BLE_HS_PHY_2M etc

Three new configs are introduced: BLE_ADDITIONAL_PHY that enables
additional PHY and two configs that specify what additional PHY shall be
enabled (2M or CODED)
Copy link
Contributor

@andrzej-kaczmarek andrzej-kaczmarek left a comment

Choose a reason for hiding this comment

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

if you have some time to spare, you can also add BLE_LL_PHY and change ll to use it consistently instead of BLE_LL_BT5_PHY_SUPPORTED

@KKopyscinski
Copy link
Contributor Author

if you have some time to spare, you can also add BLE_LL_PHY and change ll to use it consistently instead of BLE_LL_BT5_PHY_SUPPORTED

Added second commit

@@ -233,7 +233,7 @@ struct ble_ll_conn_sm
uint16_t host_req_max_rx_time;
#endif

#if (BLE_LL_BT5_PHY_SUPPORTED == 1)
#if (MYNEWT_VAL(BLE_LL_PHY))
Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

This config infers value from PHY flags and replaces BLE_LL_BT5_PHY_SUPPORTED
define.
@KKopyscinski KKopyscinski merged commit ba1a746 into apache:master Oct 23, 2023
14 checks passed
@KKopyscinski KKopyscinski deleted the phy_config branch October 23, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants