embassy-rs / nrf-softdevice

Apache License 2.0
256 stars 76 forks source link

Added phy_update method to Connection #203

Closed kext closed 8 months ago

kext commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.