NordicSemiconductor / pc-ble-driver-py

Python bindings for the ble-driver library
Other
126 stars 115 forks source link

Changing PHY from BLE peripheral results in error in pc-ble-driver-py #159

Closed Krakonos closed 4 years ago

Krakonos commented 4 years ago

If the peripheral attempts to change PHY to 2MHz, I get a logger message:

ERROR Invalid received BLE event id: 0x21

I think this is BLE_GAP_EVT_PHY_UPDATE_REQUEST which seems to be not implemented in pc-ble-driver-py. Am I correct? Is it fixable?

Thanks, Ladislav

bihanssen commented 4 years ago

PHY update is supported in the lower layers (ble-driver), but functions and events have not been added to ble_driver.py.

Krakonos commented 4 years ago

Does that mean the pc-ble-driver should accept it automatically? Or do I need to implement the handler and respond myself? Because we have sniffed out the connection the the pc-ble-driver does not respond to the request.

bihanssen commented 4 years ago

The phy update procedure needs to be handled, it doesn't happen automatically in ble-driver. Specifically the phy_update function and phy_update_request event is needed. To follow the existing pattern, it needs to be implemented in ble_driver.py file.

Krakonos commented 4 years ago

I see. For future reference, this is actually a pretty big problem: if the peripheral device requests 2MHz PHY, it expects some response. The current implementation sends none, and so the peripheral disconnects after some time (in my case I observed around 40s, but I'm not sure what defines this interval).

I'll look into implementing the call.

bihanssen commented 4 years ago

That is correct, it will lead to a disconnect if the request is not handled. The timeout is 30 seconds and is specified in the Bluetooth specification, same as for all similar requests. A PR on the feature would be welcome.

jangalda-nsc commented 4 years ago

Hi, is it any plan to implement it? We encountered the same problem.

Krakonos commented 4 years ago

Hi! I never really finished due to different priorities. However, I have started. I'll try to upload the progress tomorrow and check how much is remaining (feel free to bug me if I forget).

bihanssen commented 4 years ago

An implementation of phy update has also been started in branch ble_gap_phy by @halkver.

jangalda-nsc commented 4 years ago

Hi, thanks for answers. I used @halkver's branch and it worked. The only change I needed to do is adding on_gap_evt_phy_update_request and on_gap_evt_phy_update empty methods to BLEDriverObserver class.

Krakonos commented 4 years ago

Great, that's good news. can you share your modifications? I'd like to test those. Also, @halkver might be able to use those to include in his code?

jangalda-nsc commented 4 years ago

You can find my fork here: https://github.com/jangalda-nsc/pc-ble-driver-py/tree/ble_gap_phy

halkver commented 4 years ago

Hi, I have added it to the observer class now. I originally only added it to the observers in my test and forgot the parent class, sorry! I did however encounter some problems with my implementation, it didnt matter which values I requested, the request always got received as tx_phy = 3 and rx_phy = 3. @jangalda-nsc you did not encounter any similar problems?

jangalda-nsc commented 4 years ago

@halkver I have not encountered such problem. In my case, the slave (based on ncs) sends LL_PHY_REQ with TX&RX = 2M, and a master (based on pc-ble-driver-py) responds with LL_PHY_UPDATE_IND with TX&RX = 2M. So, I request is not sent by pc-ble-driver-py based device

jangalda-nsc commented 4 years ago

@halkver Have you figured it out? Do you have any plans to push it to master and release new version of pc-ble-driver-py? If not, could you please prepare private build for me (I have problems with vcpkg and cannot build package from source code on my own)?

halkver commented 4 years ago

@jangalda-nsc We havent been able to fix it yet, but as for the time being it is merged in and will have to be looked into later. Looks like the problem only lies with app initiated phy update requests. There will be released a new version soon, @bihanssen might be able to follow you up on that.