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

Windows: More than one lost connection does not work correctly #826

Closed
CodeMusher opened this issue Mar 8, 2024 · 17 comments · Fixed by #849
Closed

Windows: More than one lost connection does not work correctly #826

CodeMusher opened this issue Mar 8, 2024 · 17 comments · Fixed by #849

Comments

@CodeMusher
Copy link

To help us fix your issue, please provide the information in the below template. If something causes a crash, provide as much information as you can gather.
Just imagine: we do not know what you are doing!

Note: There is often little we can do without a minimal reproducible sample of the issue, so please provide that in a standalone git repository and link it here.

Steps to reproduce

  1. I run the WinConsole application. Press D1 to listen for devices, Select my Device and press D9 to run Connect -> Loop: ConnectionLost -> Connect

  2. The first time I power cycle my device it clearly state : DisconnectedPeripheral by lost signal:

  3. Power up the device => Normal behavior

  4. Power cycle the device again. Now it states: DisconnectedPeripheral by user:

Expected behavior

It should not matter now many times I power cycle the device. It should always be DisconnectedPeripheral by lost signal

Actual behavior

It works one time! The first power cycle operation causes the device to be put in the disconnectingRegistry

Crashlog

If something causes an exception paste full stack trace + Exception here

Configuration

Version of the Plugin:
3..0-beta-2.

Platform: e.g. iOS 10.1 / Android 4.4 / ... (including version!!! e.g. Android 5.1 / i0S 10)
Windows
Device: e.g. HTC Sensation /i Phone 7 ...
Laptop

@smsissuechecker
Copy link

Hi @CodeMusher,

I'm the friendly issue checker.
Thanks for using the issue template 🌟
I appreciate it very much. I'm sure, the maintainers of this repository will answer, soon.

@janusw
Copy link
Member

janusw commented Mar 16, 2024

Version of the Plugin: 3..0-beta-2.

This is supposed to be 3.1.0-beta.2, I assume?

@CodeMusher
Copy link
Author

Sorry, my bad! 3.1.0-beta.2 is correct

@CodeMusher
Copy link
Author

This could be solved by adding this line
disconnectingRegistry.Remove(id);
in the Device_ConnectionStatusChanged method of the Adapter class

@janusw
Copy link
Member

janusw commented May 4, 2024

This could be solved by adding this line disconnectingRegistry.Remove(id); in the Device_ConnectionStatusChanged method of the Adapter class

Hm, looks like that line is there already ...

if (nativeDevice.ConnectionStatus == BluetoothConnectionStatus.Disconnected
&& ConnectedDeviceRegistry.TryRemove(id, out var disconnectedDevice))
{
bool disconnectRequested = disconnectingRegistry.Remove(id);

... but maybe it's not always called?

@AskBojesen This seems to be your code (see #809). Care to have a look?

@janusw
Copy link
Member

janusw commented May 4, 2024

@CodeMusher So this fails with 3.1.0-beta.2. Does it work with any prior version, like 3.1.0-beta.1 or 3.0.0? (Then it would be regression.) Or a later version like 3.1.0-rc.1? (Probably not?)

@AskBojesen
Copy link
Contributor

I will try to reproduce with latest later today or tomorrow

@CodeMusher
Copy link
Author

@janusw, @AskBojesen
Sounds good.
One more thing I need is the possibility to register for updates on a characteristic that is only INDICATE, not NOTIFY.
That would be nice.

@AskBojesen
Copy link
Contributor

@janusw, @AskBojesen Sounds good. One more thing I need is the possibility to register for updates on a characteristic that is only INDICATE, not NOTIFY. That would be nice.

@CodeMusher: Please register a new issue, so we don't get more confused...
I will ignore your comment in this issue

@CodeMusher
Copy link
Author

@AskBojesen Sorry, I'll do that of course.

@AskBojesen
Copy link
Contributor

AskBojesen commented May 6, 2024

IMO this is not a bug in the plugin, but wrong usage of the API in the example code, which I have made myself to test something....
Take a look here: https://github.com/dotnet-bluetooth-le/dotnet-bluetooth-le/blob/master/Source/BLE.Client/BLE.Client.WinConsole/PluginDemos.cs line 166

This method does in fact not test connection lost > reconnect... correctly as I believe the API should be used

In our application, where we use the plugin in both Android + iOS + Windows. We listen for connection lost and handles connection lost -> reconnect,.. and it works on all three platforms :-)
BUT my colleague has spend quite some work to make it work ...

If this is to be "fixed" the WinConsole demo method must be fixed to show how it can work. But it is not related to the API and the functionality itself of the plugin - I can fix the demo method within a week - but remember it is only the winconsole demo...

The WinConsole demo is now used - and I appreciate that :-), but I also see it has consequences: All test methods must be valid with respect to how how a client must use the API....

@janusw: IMO the other client implementations are not even close to the winconsole client with respect to correct usage and functionality... I vote fixing up the clients with functionality and correct usage (whatever that is) for some next release.

What do you say @janusw : Should I use the effort to show how this can work with the existing API for connection lost -> reconnect now (within a week)?

image

@CodeMusher
Copy link
Author

@AskBojesen Does the WinConsole demo work the same when you tried it or have I missed something?
I think it would be a great thing to describe how the connection lost/reconnect should be implemented for this to work as designed.

@CodeMusher
Copy link
Author

@AskBojesen, @janusw In my application I also listen for lost connection event and start a reconnection process. Since I only managed to get the lost connection event once I tried to find any errors.
I used your WinConsole Demo try to locate the error. I came to the conclusion that the first lost connection invoked the lost connection event but it also put the device in the disconnectingRegistry. That caused the next lost connection to be handled as if the user disconnected and no lost connection event was invoked. Added a line in the source code, removing the device from the disconnectingRegistry every time we got connected kind of solved it.

If this is not a bug and it works for you on all platforms, then something must be missing in my use of the API.
Maybe you could provide some pseudocode to to do this properly?

@AskBojesen
Copy link
Contributor

AskBojesen commented May 8, 2024

Well now I have improved the demo in the winconsole.
PR: #849
And I actually see that there is a bug!
The second time I disconnect the plugin states: DisconnectedPeripheral by user, and I get no connectionlost event
I will investigate.

@AskBojesen
Copy link
Contributor

I found the bug and fixed the issue - sorry for my stubbornness from believing there was a bug.
Fixed now in PR: #849

@CodeMusher
Copy link
Author

@AskBojesen good to hear that I wasn’t wrong about this😉
I checked your log file and noticed that the second lost connection differs from the others. There is no gatt closed messages. Should the disconnect not be identical?

@AskBojesen
Copy link
Contributor

@AskBojesen good to hear that I wasn’t wrong about this😉 I checked your log file and noticed that the second lost connection differs from the others. There is no gatt closed messages. Should the disconnect not be identical?

No I don't see it as an error with the GattSession_SessionStatusChanged - we could leave it out of the log.
I guess it is some timeng when the GattSession is disposed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants