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

Disconnect device connected with autoconnect but has lost connection #751

Open
midoragh opened this issue Oct 3, 2023 · 12 comments
Open

Comments

@midoragh
Copy link
Contributor

midoragh commented Oct 3, 2023

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. Connect a device with autoconnect flag

2.Move the device out of range so that there is a DeviceConnectionLost event

3.DisconnectDeviceAsync the device.

4.Move the device into the range

  1. The device connects and the DeviceConnected event will be triggered

Expected behavior

The device should no longer reconnect after a call to DisconnectDeviceAsync .
CloseGattInstances could be called to remove auto connection from the ble stack

Actual behavior

Disconnect just returns. The trace tells "Device not in the list of connected devices" which is correct

Crashlog

None

Configuration

3.0 r1

Android 10, Android 13 Api 33

Wiko Y51, Nokia C32

@smsissuechecker
Copy link

Hi @midoragh,

I'm the friendly issue checker.
It seems like (37.50 %) you haven't used our issue template 😢 I think it is very frustrating for the repository owners, if you ignore them.

If you think it's fine to make an exception, just ignore this message.
But if you think it was a mistake to delete the template, please close the issue and create a new one.

Thanks!

@janusw
Copy link
Member

janusw commented Oct 4, 2023

Thanks for the report. Is this a new problem, or does it occur already with previous versions? There were disconnect fixes for Android in beta.3 and beta.5, which might possibly influence the behavior in this case.

Do you see the same behavior with 3.0.0-beta.2? How about 2.1.3?

@midoragh
Copy link
Contributor Author

midoragh commented Oct 4, 2023

In 2.1.3 the issue #519 has been solved but introduced the mentioned issue:

                // Close GATT if autoConnect is disabled, else we can accumulate zombie gatts.
                if (!_device.ConnectParameters.AutoConnect)
                {
                    CloseGattInstances(gatt);
                }

because DisconnectDeviceAsync doesn't remove the autoconnect zombie.

    public Task DisconnectDeviceAsync(IDevice device)
    {
        if (!ConnectedDevices.Contains(device))
        {
            Trace.Message("Device is {0} not in the list of connected devices.", device.Name);
            return Task.FromResult(false);
        }

I did a fix but I'm not sure that it removes everything like done in CloseGattInstances(gatt);

        if (!ConnectedDevices.Contains(device))
        {
            Trace.Message("Device is {0} not in the list of connected devices.", device.Name);
            if (RemoveAutoConnectNative(device)) return Task.FromResult(true);
            return Task.FromResult(false);
        }
        
    protected override bool RemoveAutoConnectNative(IDevice device)
    {
        var d = (Device)device;
        if (!d.ConnectParameters.AutoConnect) return false;
        Trace.Message("Remove GATT because of autoconnect");
        d.CloseGatt();
        return true;
    }

@janusw
Copy link
Member

janusw commented Oct 4, 2023

In 2.1.3 the issue #519 has been solved but introduced the mentioned issue:

Thanks for the context!

I did a fix but I'm not sure that it removes everything like done in CloseGattInstances(gatt);

If you find a proper way to fix this, please open a PR. In case the fix is reasonably simple and low-risk, we can still include it in the final 3.0.0, which I would really like to release this week.

@midoragh
Copy link
Contributor Author

midoragh commented Oct 5, 2023

There is also an issue in the case that the device is in the ConnectedDevices list. I'm looking to fix this in 2.1.3 and PR it later to 3.0.
Doing the same for bonding and the PairingRequest broadcast.

@janusw
Copy link
Member

janusw commented Oct 6, 2023

There is also an issue in the case that the device is in the ConnectedDevices list. I'm looking to fix this in 2.1.3 and PR it later to 3.0.

I'm not sure if it's worth fixing this on 2.x. I'm not really planning on making another 2.x release (unless there's a strong reason for it). I'd recommend fixing this directly on master / 3.0.0-rc.1, in particular if you still want this to make it into 3.0.0.

@midoragh
Copy link
Contributor Author

midoragh commented Oct 7, 2023

This may be a misunderstanding. I do not expect that you fix something in 2.x, everybody should switch to 3 if possible.
But I have to continue support for 2.x in the moment. Because Xamarin is in the end of life state. Everybody has. Some are waiting for .NET 8 because of LongTimeSupport.

@janusw
Copy link
Member

janusw commented Oct 7, 2023

This may be a misunderstanding. I do not expect that you fix something in 2.x, everybody should switch to 3 if possible. But I have to continue support for 2.x in the moment. Because Xamarin is in the end of life state.

v3.0 still supports Xamarin in addition to MAUI, so you don't have to stick to v2 because of Xamarin. I strongly recommend that you switch to 3.0.

@midoragh
Copy link
Contributor Author

midoragh commented Oct 7, 2023

I'm not able to repeat this discussion with my colleagues. Not every month ,-)
There are improvements in 3.0 I really like to use (like the improved scan filters), but no chance in the moment.

@janusw
Copy link
Member

janusw commented Oct 7, 2023

I'm not able to repeat this discussion with my colleagues. Not every month ,-) There are improvements in 3.0 I really like to use (like the improved scan filters), but no chance in the moment.

No chance, why?

If it's only the fear of beta releases: I'm about to tag the final 3.0.0 this weekend ...

(It doesn't sound like your life depends on having a fix for this issue in 3.0.0, so it will need to go into a future 3.0.x bugfix release.)

@midoragh
Copy link
Contributor Author

midoragh commented Oct 7, 2023

We are not even allowed to change apps used for production tests. You are not allowed to shift any dates of approval. You can only cancel and restart the process which takes months and is quite expensive.
I like to support, but I can only do tests locally and this is just not enough to provide serious feedback.

I'm making some progress with PairingRequested broadcast but it is still not working in all cases.
And it's the same for autoConnect. There the KnownServices should be removed on disconnecting an auto connected device but this somehow destroy something else which I do not understand in the moment. And the final disconnect does not remove the zombie gatts so I guess it needs a flag for autoConnect and not the ConnectionParameters.

BTW: A DeviceStateChanged event would be nice :-) But this should be a different issue.

@janusw
Copy link
Member

janusw commented Oct 8, 2023

We are not even allowed to change apps used for production tests. You are not allowed to shift any dates of approval. You can only cancel and restart the process which takes months and is quite expensive. I like to support, but I can only do tests locally and this is just not enough to provide serious feedback.

Well, ok, I really don't understand the details of your company policies (and I don't need to).

In any case: If you have a hard requirement for sticking to 2.1.x for now, we could probably do a 2.1.4 bugfix release (if you are willing to help with this). But it would certainly only include minor bugfixes and no new features etc.

All the new pairing/bonding interfaces are only available on 3.x, and I don't think we'll be backporting these to 2.1.x. Also new events will only be accepted for 3.x.

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

No branches or pull requests

3 participants