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

Added phy_update method to Connection #203

Merged
merged 2 commits into from
Nov 8, 2023

Conversation

kext
Copy link
Contributor

@kext kext commented Nov 8, 2023

This calls sd_ble_gap_phy_update if the ScanConfig includes the 2M PHY in phys.

The change requires no API changes and should have no downsides.
If you are fine with extended advertisements on the 2M PHY, you should be fine using it for the connection as well.

@Dirbaio
Copy link
Member

Dirbaio commented Nov 8, 2023

why not add support for sd_ble_gap_phy_update, with a method in Connection instead, so the user can do it themselves?

IMO this lib should be a "thin" wrapper as possible on top of the softdevice, we should try to avoid adding our own "helpful" logic if we can instead make more underlying functionality available.

@kext
Copy link
Contributor Author

kext commented Nov 8, 2023

To be honest: Because this is the easiest solution I could think of. 🤣

I like the idea of adding a function to Connection. However I'm not sure how to implement that. Optimally the method would be async and return after the BLE_GAP_EVTS_BLE_GAP_EVT_PHY_UPDATE event and give the actual PHY that has been switched to.

Something like async fn phy_update(&mut self, tx_phy: PhySet, rx_phy: PhySet) -> Result<(Phy, Phy), SomeError>.

My question is: How do I get the return value from the on_evt handler back to the function?

@kext
Copy link
Contributor Author

kext commented Nov 8, 2023

I have now implemented this as a method on Connection. I still think it would probably be cooler if it was async but all the other methods aren't async either.

Another pain point that remains is that I basically copied the error type. Now there are SetConnParamsError, IgnoreSlaveLatencyError and PhyUpdateError which are all exactly the same. Maybe it would be better to replace them all by the same error type. That would be a breaking change however.

@kext kext changed the title Upgrade to 2M PHY if it is included in the ScanConfig Added phy_update method to Connection Nov 8, 2023
Copy link
Member

@Dirbaio Dirbaio 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 it's fine that it's not async, you don't always want to wait. set_conn_params doesn't wait either, I use it in my project in a setting where I don't want it to wait.

We can always add a pub fn wait_for_phy_update_done() later if someone wants it.

Thanks for the contribution! :)

@Dirbaio Dirbaio added this pull request to the merge queue Nov 8, 2023
Merged via the queue into embassy-rs:master with commit f82d0b7 Nov 8, 2023
2 checks passed
@kext kext mentioned this pull request Nov 8, 2023
@kext kext deleted the ble-phy-upgrade branch November 8, 2023 22:59
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.

2 participants