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

Added keyword argument option to force using Notifications instead of Indications in WinRT backend #620

Merged
merged 5 commits into from
Oct 6, 2021

Conversation

hbldh
Copy link
Owner

@hbldh hbldh commented Aug 10, 2021

Added force_notify keyword argument for WinRT backend client's start_notify method.

Fixes #526

@hbldh hbldh added the Backend: WinRT Issues or PRs relating to the WinRT backend label Aug 10, 2021
@hbldh hbldh self-assigned this Aug 10, 2021
@dlech
Copy link
Collaborator

dlech commented Aug 10, 2021

I am still of the opinion that Windows should be changed to use notifications instead of indications by default when both are available. This matches the BlueZ backend and the name of the method (and in my limited experience, notifications seem to be used much more frequently than indications).

@dlech
Copy link
Collaborator

dlech commented Aug 10, 2021

Random idea: should we "namespace" platform-specific kwargs like this with the platform name? I.e. win_force_notification or win_force_indication here. This would make it obvious to readers of user code that this is a platform-specific feature without having to consult the Bleak documentation or source code.

@hbldh
Copy link
Owner Author

hbldh commented Aug 10, 2021

I agree with the default. Will change it to use notifications instead of indications.

There are multiple issue where people use some keywords in OSes that do not care for them. It is apparently not enough with documentation, since users look through issues/SO and use anything that is reminiscent of what they want to achieve.

I like the idea, but the prefixing seems horribly ugly to me. I have no better idea though...

How do other cross platform APIs handle this?

@dlech
Copy link
Collaborator

dlech commented Aug 10, 2021

I like the idea, but the prefixing seems horribly ugly to me. I have no better idea though...

What if there is a standard kwarg for each platform that takes a dictionary. Looks like this:

start_notify(..., win={ "force_indication": True })

How do other cross platform APIs handle this?

This is how cross-platform settings (json rather than API) in VS Code work.

The APIs I am familiar with, usually the entire method either works or doesn't on a platform. I haven't really seen anything that just applies to single parameters.

@hbldh
Copy link
Owner Author

hbldh commented Aug 12, 2021

I have given it some thought and I agree with your assessment: we need a clearer separation of OS specific arguments. I do not personally like neither the win={} nor the prefixing, but my reasoning is that a clear and understandable API for users trumps everything else.

Of the two I prefer the win={}, macos={}, linux={} (or bluez? not sure. posix?) I will open a new issue for this, because it will take a lot careful combing through all backends to get a complete coverage for this. It feels like it is a release of its own, like 0.14.0...

Flipped the default for Windows backends here now. Do you think the Changelog entry is enough or do you think it should be even more verbose about the changing of the default? I do not think many will notice it, but for those that do it might be helpful...

@dlech
Copy link
Collaborator

dlech commented Aug 12, 2021

It wouldn't hurt to mention the change in behavior.

@hbldh
Copy link
Owner Author

hbldh commented Aug 12, 2021

Yes, I know and agree. I was hoping on you saying, don't bother and I would magically be free of handling the change...

@dlech
Copy link
Collaborator

dlech commented Aug 12, 2021

Haha, OK, only 3 people will notice. 😁

@hbldh
Copy link
Owner Author

hbldh commented Aug 12, 2021

That is enough. My bar for getting away for something is 0.

It is right to do and I know it. I would have done it anyway before merging. Documentation and transparency is the only way forward in a project like this. Everything else is wrong.

I REALLY need to focus on #266 and get that done soon. It feels like 80% of our issues this spring and summer came from a poor documentation and a lack of including real users' user cases in it. I need to rewrite it.

bleak/backends/winrt/client.py Outdated Show resolved Hide resolved
bleak/backends/winrt/client.py Outdated Show resolved Hide resolved
dlech and others added 2 commits October 5, 2021 14:39
This simplifies the logic of selecting indications or notifications
and raises an error when neither are supported instead of disabling
notifications/indications.
@dlech
Copy link
Collaborator

dlech commented Oct 6, 2021

I added a commit with my suggested changes.

@dlech dlech merged commit 1075f5e into develop Oct 6, 2021
@dlech dlech deleted the feature/issue_526 branch October 6, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend: WinRT Issues or PRs relating to the WinRT backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How can I activate notify if both notify and indicate are in the characteristic at the same time?
2 participants