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

Fix initialization #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

amclain
Copy link
Contributor

@amclain amclain commented Mar 15, 2022

This PR fixes a couple issues I ran into when trying to run the scanner.ex example with a Realtek rtl8761b USB dongle (VID=0x0BDA, PID=0x8771). On initialization of the Bluetooth dongle the GenServer would timeout:

iex(1)> {:ok, pid} = BlueHeronScan.start_link(:usb, %{pid: 0x8771})

15:27:30.955 [warning] BlueHeron(USB): Using BT device at bus 3, device 8: ID 0bda:8771
{:ok, #PID<0.284.0>}
iex(2)> 
15:27:35.952 [error] Timeout executing Init commands
 
15:27:40.953 [error] Timeout executing Init commands
 
15:27:45.955 [error] Timeout executing Init commands
** (EXIT from #PID<0.282.0>) shell process exited with reason: :reached_max_error

The first place seems to be from {:next_event, :internal, :init} not getting emitted after each prepare() completed (26eaf58). The second looks like it was from the send_command() call not receiving a reply (ea4a51c).

Versions:

elixir          1.13.3-otp-24
erlang          24.3.1

Kernel:

Linux ubuntu 5.13.0-35-generic

@trarbr
Copy link
Collaborator

trarbr commented Mar 21, 2022

Hi @amclain, thank you for submitting the PR 💜 I've just taken a brief look at it.

The reason why you're seeing timeouts is that BlueHeron.Transport will wait to receive acknowledgements from the controller on HCI packets, before replying to the caller or continuing with the list of init commands. This is by design, so that the Transport can return any values that might be carried in the acknowledgement to the caller. And also to avoid flooding the controller with too many messages (which might just begin throwing packets away or resetting itself).

Now, the controller usually replies immediately, so it's strange that the BlueHeron.Transport is not receiving those acknowledgements in your case. I suspect it is a bug in the USB transport code. There might be other bugs lurking in the USB transport as well (see #42). Unfortunately I don't have a Bluetooth adapter to test with at the moment, nor do I really have the skills to debug the C code, so I'll ping @fhunleth and @ConnorRigby and see if they've got something to offer.

@amclain
Copy link
Contributor Author

amclain commented Mar 21, 2022

Thanks for the info and for passing this on!

FWIW I had originally tried this with a Kinovo dongle that uses the Broadcom BCM20702 chipset (VID 0x0A5C, PID 0x21E8) but experienced the same issue so I got a Realtek dongle since it's officially supported.

@ConnorRigby
Copy link
Collaborator

Taking a look at this right now. as long as it doesn't break existing devices, i think it looks good to me.

@@ -211,7 +211,7 @@ defmodule BlueHeron.HCI.Transport do
case module.send_command(pid, bin) do
true ->
Logger.hci_packet(:HCI_COMMAND_DATA_PACKET, :out, bin)
{:keep_state, add_call(data, {from, opcode})}
{:keep_state, add_call(data, {from, opcode}), {:reply, from, :ok}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this change? it breaks the contract of the send_command function.

Copy link
Contributor Author

@amclain amclain Sep 29, 2022

Choose a reason for hiding this comment

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

I'll have to refresh my memory on this, but I recall having to make this change because the prior code caused a hang or crash and was a blocker. I'm sure Blue Heron has evolved since I opened this PR, so I'll take a look at this again with a fresh set of eyes.

Copy link

Choose a reason for hiding this comment

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

@amclain I tested scanner.ex from this repo and the gatt server example from nerves_livebook using your change on line 190 while excluding this change on line 214. It resolves the issues with initialization timeout.

However, including this :ok reply from line 214, produces issues with modules that expect a tagged tuple response like {:ok, %CommandComplete{}}. In particular, peripheral.ex produces this error when attempting to set advertising parameters for the gatt server example:

I think removing the change on line 214 from this PR would get the desired results and maintain the contract of the send_command function.

Copy link
Contributor Author

@amclain amclain Dec 19, 2022

Choose a reason for hiding this comment

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

👍 I've backed out line 214. Updated in 26eaf58.

Copy link
Collaborator

@ConnorRigby ConnorRigby left a comment

Choose a reason for hiding this comment

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

Sorry it took so long on this @amclain , but i've tested it and found an issue with that reply added in the second commit.

@ConnorRigby ConnorRigby force-pushed the main branch 2 times, most recently from dbd01b5 to 67a245b Compare September 21, 2023 15:06
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.

4 participants