Skip to content

Bleio attribute api revamp #2092

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Aug 30, 2019
Merged

Bleio attribute api revamp #2092

merged 9 commits into from
Aug 30, 2019

Conversation

dhalbert
Copy link
Collaborator

Service, Characteristic, and Descriptor now are created via a factory classmethod, which takes their parent object:

service = Service.add_to_peripheral(peripheral, uuid, ...)
characteristic = Characteristic.add_to_service(service, uuid, ...)
descriptor = Descriptor.add_to_characteristic(service, uuid, ...)

So there can be no unattached attributes, and they can't be reused by accident.

The C code is cleaner and needs less error checking.

@tannewt I changed from Style B to style D, because it leaked less of the abstraction. In the other style (peripheral.add_service(...)), one class needed to know the args for the other class's constructor. This is more general.

@dhalbert dhalbert requested a review from tannewt August 29, 2019 03:24
@dhalbert dhalbert mentioned this pull request Aug 29, 2019
Merged
tannewt
tannewt requested changes Aug 29, 2019
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for these revisions. It's definitely valuable to ensure that everything is attached as expected.

It's still not clear to me:

  1. How dynamic are the links between Peripheral, Service, Characteristic and Descriptor?
  2. Is it possible for a service to exist on a peripheral more than once?
  3. Can we load knowledge of a service like DeviceInformationService separately from us producing it so that we can use it when we discover a device that has it?

I still feel a declarative approach to defining the tree would be best. It would factor out defining how everything connects with actually binding it to a device, local or remote. It would also force naming and documenting each child. Let's chat later about this.

@dhalbert
Copy link
Collaborator Author

@tannewt and I talked about the structure of the API extensively. I understand his desire to describe a service in a declarative way in a class, with attributes or dictionary lookup for characteristic values. We agreed that is a goal, probably done first in Python and then possibly moved to C.

Because the native module API will continue to evolve, we are renaming bleio to _bleio to discourage its everyday use, and to allow it to change incompatibly between minor versions of CircuitPython. The adafruit_ble library should be used instead. If it's missing some functionality, then it should be added. The library will also evolve in an incompatible way as we head towards a better API.

We would like people to use and test BLE as it stands to help get bugs out and to try new use cases, with the understanding that things may change (for the better).

@tannewt Please add comments or correct as you wish.

@dhalbert dhalbert requested a review from tannewt August 30, 2019 02:38
tannewt
tannewt approved these changes Aug 30, 2019
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good! Excited to start building on this!

@tannewt tannewt merged commit b954b2f into adafruit:master Aug 30, 2019
@dhalbert dhalbert mentioned this pull request Sep 9, 2019
Merged
@dhalbert dhalbert deleted the bleio-api-revamp branch November 22, 2019 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhalbert @tannewt