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

Is there a need for the local cache? #24

Closed
tony-gutierrez opened this issue Oct 28, 2020 · 9 comments
Closed

Is there a need for the local cache? #24

tony-gutierrez opened this issue Oct 28, 2020 · 9 comments

Comments

@tony-gutierrez
Copy link
Contributor

tony-gutierrez commented Oct 28, 2020

Windows has its own cache of services and chars, so do we really need the local cache that winrt currently has?

The local cache is not currently populated by DiscoverServices, so subsequent calls for a single service or char will hit windows again, windows will serve from its ble cache, and then we add to local cache.

It seems like the local cache could be eliminated.

@geovie
Copy link
Contributor

geovie commented Oct 30, 2020

@tony-gutierrez You're right! The way it's currently implemented it will only ask the windows ble cache.

I think we could improve this by first checking the windows ble cache with BluetoothCacheMode::Cached and if that fails let windows query the device with BluetoothCacheMode::Uncached.

One thing I'm not sure anymore but IIRC we need to keep a reference to the characteristic to get notifications for it

@tony-gutierrez
Copy link
Contributor Author

My understanding is that BluetoothCacheMode::Cached will automatically query the device if there is no cached value. So it shouldn't fail.

Deets here: https://docs.microsoft.com/en-us/uwp/api/windows.devices.bluetooth.bluetoothcachemode?view=winrt-19041

@geovie
Copy link
Contributor

geovie commented Oct 30, 2020

Actually the way it's implement currently is correct, as all services need first to be discovered they should already be in cache once we request one.

@tony-gutierrez
Copy link
Contributor Author

tony-gutierrez commented Oct 30, 2020

Yeah, that is what happens. On second discovery, it misses local cache, and hits windows. On 3rd it would probably hit the local cache. I am just thinking we don't need the local cache. We might need to track chars like you said, but I don't think the local cache check before reading a service is needed.

@pursual
Copy link

pursual commented Sep 9, 2021

There is now expanded information on the cache at the MS url linked above.

@tony-gutierrez
Copy link
Contributor Author

Was able to get a good perf improvement in my version by using Cached mode, in addition to not trashing the local cache on disconnect. Have not tested eliminating the local cache and just relying on windows, but seems like a logical next step.

@geovie
Copy link
Contributor

geovie commented Sep 14, 2021

As far as I can see we're already using BluetoothCacheMode::Cached everywhere. If we can rely on the windows caching then we could definitely remove our custom cache, after all it'll be less code and less of a chance to get out of sync with the device.

@tony-gutierrez
Copy link
Contributor Author

Ah, somehow my code started with some Uncached, from a fork I guess. I'll be in touch if I get a chance to remove local cache.

@tony-gutierrez
Copy link
Contributor Author

Removing cachedServices seems to cause 2 problems:

  1. You lose the ability to maintain cachedServices during disconnect, which for my use case allows for rapid retries of a failed procedure.
  2. Notify seems to break, it stops announcing characteristic value changes.

Closing this for now.

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

No branches or pull requests

3 participants