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

Standardizing arguments passed to callback functions #334

Merged
merged 6 commits into from
Dec 7, 2020

Conversation

Bernstern
Copy link
Contributor

Implementation for standardizing callback arguments passed to callback functions to make use easier across platforms (resolves #301).

This implements the feature for all platforms and include doc updates and an example.

@hbldh hbldh self-assigned this Oct 20, 2020
@hbldh hbldh added Backend: pythonnet Issues or PRs relating to the .NET/pythonnet backend Backend: BlueZ Issues and PRs relating to the BlueZ backend Backend: Core Bluetooth Issues and PRs relating to the Core Bluetooth backend enhancement New feature or request labels Oct 20, 2020
@hbldh
Copy link
Owner

hbldh commented Oct 20, 2020

I like the idea, but I have not got the time to review this right now; it is a lot of changes...

@Bernstern Could you fix the conflicts and squash it down to one commit and I will look this over next week?

Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

Thanks for taking time to work on this. I've made a bunch of comments, but looking at this from a higher level, we are already parsing advertising data in 2 places (implementations of BLEDevice and BaseBleakScanner abstract classes) and this adds a 3rd. Reusing this existing code would fix many of the bugs I pointed out in this PR.

I think we could create an AdvertisementData class based on the dictionary given in this PR and utility functions in each backend to convert the platform-specific data into the common AdvertisementData type. Then this common type could be passed around instead of the platform-specific data.

@Bernstern
Copy link
Contributor Author

I agree, having it as a separate class will be really handy, I'll implement those changes (and remove that debug print 😃 ) over the next few days.

@Bernstern Bernstern force-pushed the standardizing-callback-args branch 3 times, most recently from 8523d3a to 250b267 Compare November 2, 2020 23:29
@Bernstern Bernstern force-pushed the standardizing-callback-args branch from 250b267 to 52016fe Compare November 2, 2020 23:32
@Bernstern
Copy link
Contributor Author

I implemented the requested changes to use an AdvertisementData class and updated the documentation and the example I wrote. I didn't quite follow @dlech's suggestion to include the other advertisement sections for the dotnet backend so I'm open to suggestions on how to implement that 😃.

Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

This looks good. Just a few more questions and suggestions.

We can look at reusing this to replace the duplicated code later.

@Bernstern
Copy link
Contributor Author

Let me know if there are any other changes you would like to see.

@dlech
Copy link
Collaborator

dlech commented Nov 11, 2020

I was hoping to have to have some time to test this before merging, but I haven't had time yet.

Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

Finally had some time to get back to this. It is looking really good.

Although I did find some problems on Windows and Linux - see inline comments.

FWIW, you can find my working branch at https://github.com/dlech/bleak/commits/pr/Bernstern/334 which contains the suggested fixes and some other unrelated fixes

Finally, it would also be helpful to add a changelog entry for this.

self.platform_data = kwargs.get("platform_data", ())

def __str__(self):
return "Advertisement data for device at addr: {0}".format(self.address)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a __repr__() method here for debugging:

    def __repr__(self) -> str:
        kwargs = ""
        if self.local_name:
            kwargs += f", local_name={repr(self.local_name)}"
        if self.rssi:
            kwargs += f", rssi={repr(self.rssi)}"
        if self.manufacturer_data:
            kwargs += f", manufacturer_data={repr(self.manufacturer_data)}"
        if self.service_data:
            kwargs += f", service_data={repr(self.service_data)}"
        if self.service_uuids:
            kwargs += f", service_uuids={repr(self.service_uuids)}"
        if self.platform_data:
            kwargs += f", platform_data={repr(self.platform_data)}"
        return f"AdvertisementData({repr(self.address)}{kwargs})"

@dlech
Copy link
Collaborator

dlech commented Dec 7, 2020

@hbldh do you have an opinion on where the AdvertisementData class should live? Currently it is at bleak.backends.scanner.AdvertisementData.

@Bernstern
Copy link
Contributor Author

@dlech Thank you very much for your suggestions and your working branch made it very apply your requested changes and test them locally, much appreciated!

I added all of the changes on the working branch and merged develop back into it since there were merge conflicts but they should be resolved.

@dlech
Copy link
Collaborator

dlech commented Dec 7, 2020

Awesome. Let's call this good enough and get it merged.

@dlech dlech merged commit 77be08c into hbldh:develop Dec 7, 2020
dlech added a commit that referenced this pull request Dec 7, 2020
…ck().

Since #334, all backends have the same implementation of `BleakScanner.register_detection_callback()` and the documentation was out of date.

This consolidates the implementation in the abstract base class and updates the type hints and documentation.
dlech added a commit that referenced this pull request Dec 7, 2020
…ck().

Since #334, all backends have the same implementation of `BleakScanner.register_detection_callback()` and the documentation was out of date.

This consolidates the implementation in the abstract base class and updates the type hints and documentation.
dlech added a commit that referenced this pull request Dec 7, 2020
…ck().

Since #334, all backends have the same implementation of `BleakScanner.register_detection_callback()` and the documentation was out of date.

This consolidates the implementation in the abstract base class and updates the type hints and documentation.
This was referenced Dec 10, 2020
@dlech dlech mentioned this pull request Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend: BlueZ Issues and PRs relating to the BlueZ backend Backend: Core Bluetooth Issues and PRs relating to the Core Bluetooth backend Backend: pythonnet Issues or PRs relating to the .NET/pythonnet backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using a standard format for args passed to callback functions
3 participants