WIP: bleio rewrite #1289

arturo182 · 2018-10-21T14:18:17Z

Here is the WIP of the bleio rewrite, it was done in July and August, but I rebased it on the current master, so not 100% everything will work out of the box, but at least this can give a good starting point for the ble work.

I tried to make the API as simple as possible, while still being robust and not limiting more advanced users. I think we could make a separate python API based on this one, like @tannewt suggested way back, so the users can easily create services alike Battery Service, etc.

I also tried to document as much as possible, so the generated docs should give some insight to the API and usage. See the Device class for usage:

import bleio

# Peripheral
periph = bleio.Device()

serv = bleio.Service(bleio.UUID(0x180f))
periph.add_service(serv)

chara = bleio.Characteristic(bleio.UUID(0x2919))
chara.read = True
chara.notify = True
serv.add_characteristic(chara)

periph.start_advertising()

# Central
scanner = bleio.Scanner()
entries = scanner.scan(2500)

my_entry = None
for entry in entries:
    if entry.name is not None and entry.name == 'MyDevice':
        my_entry = entry
        break

central = bleio.Device(my_entry.address)
central.connect()

Also added a UUIDType enum-like class for determining UUID type.

Use the new in the Adapter singleton.

…a class

Damn copy-paste!

…odule

This was the last class from ubluepy and so that module is now gone. The Device class offers both Peripheral and Central functionality. See the inline docs for more info.

Moved the functions to classes that they belong to.

Because of the very specific way nRF requires service registration (characteristics can be added only to last added service), we would have to write the Python code in a specific way. With this patch the user has more freedom.

dhalbert · 2018-10-21T14:48:49Z

Thanks! Could you say a little something about how it differs from ubluepy and your thinking when making the changes?

dhalbert · 2018-10-30T23:01:09Z

Thanks for all this work! I'm starting a review with comments after educating myself on BLE, going over bluepy (not specifically ubluepy) and then your code.

dhalbert

I really like how you have cleaned up the non-Pythonic parts of ubluepy, creating classes where necessary, and using properties where appropriate.

I haven't reviewed in any detail common-hal code at all, because I'm completely unfamiliar with the Softdevice API. I'm willing to merge it in and start testing it.

We still should create wrappers on top of this for various Profiles and maybe some common Central use cases.

What are your thoughts about handling things that DefaultDelegate does in ubluepy?

dhalbert · 2018-10-30T23:02:26Z

ports/nrf/bluetooth/ble_drv.h

+#include "ble.h"
+
+#if (BLUETOOTH_SD == 132) && (BLE_API_VERSION == 2)
+#define NRF52


Does this cover SD140 as well?

I believe that for SD140 this define is not needed as it uses the NRF528xx defines. This is just for legacy reasons

dhalbert · 2018-10-30T23:04:41Z

ports/nrf/bluetooth/ble_uart.c

+
+#if (MICROPY_PY_BLE_NUS == 1)
+
+static const char default_name[] = "CP-REPL"; // max 8 chars or uuid won't fit in adv data


"CIRCPY", maybe? Is this max 7 + 1 null or max 8?
"CIRCUITP" is 8

dhalbert · 2018-10-30T23:09:33Z

shared-bindings/bleio/Characteristic.c

+
+//|   .. attribute:: write
+//|
+//|     A `bool` specifying if the characteristic allows writting to its value.


"writting" -> "writing" here and several other places.

dhalbert · 2018-10-30T23:14:36Z

shared-bindings/bleio/Device.c

+//| =========================================================
+//|
+//| Provides access a to BLE device, either in a Peripheral or Central role.
+//| When a device is created without any parameter passed to the constructor,


Device now does both Peripheral and Central, but this seems like two classes mostly stuck together. They share .name and .services, but even those mean somewhat different things. I would be inclined to go back to two different classes, so that they really do different things and you don't have to keep checking whether you're one kind of Device or the other.

I think my idea was that we want to make the API as simple as possible, so we don't want to bother the user with terms like Peripheral and Central and having them try to figure out which one they need to use, it's easier to just have one.

dhalbert · 2018-10-30T23:18:00Z

shared-bindings/bleio/UUID.c

+
+//|   .. attribute:: type
+//|
+//|     The UUID type. One of:


I think it would be better to have UUID16 and UUID128, possibly as subclasses of an abstract UUID, if there is really code to be shared. We can use the Python type system. We can do isinstance() to figure out the type, instead of checking a field.

Yes, that might be better.

dhalbert · 2018-10-30T23:18:38Z

shared-bindings/bleio/UUIDType.c

+
+#include "shared-bindings/bleio/UUIDType.h"
+
+//| .. currentmodule:: bleio


Class would go away if we have UUID16 and UUID128.

dhalbert · 2018-11-05T03:39:39Z

@arturo182 I know you're working on other projects. Let me know if you have time to respond to this, or if I should just proceed. Even a quick response to some of the queries would be helpful. Tnx.

arturo182 · 2018-11-05T07:56:56Z

I'm so sorry! I'll respond later today, but in general I think you can proceed with what you think is best for the project.

arturo182 · 2018-11-05T20:58:30Z

Thanks! Could you say a little something about how it differs from ubluepy and your thinking when making the changes?

It differs quite a lot since I made many breaking changes. I wanted the API to be something that I would find intuitive and easy to use, to be like other CP APIs, easy for beginners.

Truth be told, it's been some time since I wrote this and I should've pushed it earlier, because now I have a hard time remembering all the details of my decisions.

Also, as you say, I've been quite busy working on other projects and I'm not sure how much time I will be able to spend on this, so if you feel it's going to be easier for you to polish this PR up before it's merged or even abandon it and only pick pieces to use as a base for a different API, I'll understand.

@arturo182

Resubmit PR #1289, "WIP: bleio rewrite" by @arturo182

dhalbert · 2018-11-07T20:32:52Z

Wow, merging #1319 merged #1289 automatically, because #1319 included the commits.

arturo182 added 30 commits October 21, 2018 15:43

nrf: Move the UUID class from ubluepy to the shared bleio module

f4940c9

Also added a UUIDType enum-like class for determining UUID type.

nrf: Move the Descriptor class from ubluepy to the shared bleio module

20b8d51

bleio: Add a AddressType enum-like class

d5f942a

bleio: Add a new Address class

345334a

Use the new in the Adapter singleton.

bleio: Move ScanEntry to shared module and add a new AdvertisementDat…

7390dc7

…a class

bleio: Move the Scanner class to a shared module

1c6bf9a

nrf: Remove unused ubluepy classes

61bf4a1

bleio: Remove SAMD mention in include guard

fb422cc

Damn copy-paste!

nrf: Move the Characteristic class from ubluepy to the shared bleio m…

cc78249

…odule

nrf: Move the Service class from ubluepy to the shared bleio module

bda7342

nrf: Move the Peripheral class to bleio as Device

3bd65fb

This was the last class from ubluepy and so that module is now gone. The Device class offers both Peripheral and Central functionality. See the inline docs for more info.

bleio: Improve type documentation

5412bf6

nrf: Remove the ble drv specific address struct

4b34481

nrf: Remove the ble drv specific advertisement data struct

3df7dea

nrf: Remove the ble drv specific service struct

98aa8c5

nrf: Remove the ble drv specific characteristic struct

6545aa9

bleio: Fix incorrect role detection

a126897

nrf: BLE driver cleanup

77eeecb

nrf: Cleanup of the ble driver

17f13ec

Moved the functions to classes that they belong to.

nrf: Move bluetooth driver to the 'bluetooth' folder

d5a71a4

nrf: Fix ble uart using the new API

cf79316

bleio: A bit of cleanup

c7b42d8

bleio: Remove redundant struct field

b5e5805

bleio: Remove deep copy constructor for UUID

19fab4a

bleio: Remove unneeded TODO

684f267

bleio: Add scan_entry as an param for the Device constructor

beee58a

bleio: Let Characteristic inherit the Services UUID length

ad466b3

bleio: Don't register the services until needed

eceb21a

Because of the very specific way nRF requires service registration (characteristics can be added only to last added service), we would have to write the Python code in a specific way. With this patch the user has more freedom.

bleio: Remove UUID static variables

13dd27a

bleio: Allow using len() on UUID

5354aea

bleio: Fix docs error

c62b708

arturo182 requested review from tannewt, microbuilder and dhalbert October 21, 2018 14:18

arturo182 force-pushed the bleio branch 2 times, most recently from 8e959e6 to 01a6bbc Compare October 21, 2018 14:39

bleio: Fix errors after rebase

4bc24c4

arturo182 force-pushed the bleio branch from 01a6bbc to 4bc24c4 Compare October 21, 2018 15:15

dhalbert requested changes Oct 30, 2018

View reviewed changes

dhalbert mentioned this pull request Nov 7, 2018

Resubmit PR #1289, "WIP: bleio rewrite" by @arturo182 #1319

Merged

tannewt merged commit 4bc24c4 into adafruit:master Nov 7, 2018

tannewt added a commit that referenced this pull request Nov 7, 2018

Merge pull request #1319 from dhalbert/origin/arturo182_bleio_pr_1289

d08747d

Resubmit PR #1289, "WIP: bleio rewrite" by @arturo182

arturo182 deleted the bleio branch November 7, 2018 20:17

dhalbert mentioned this pull request Nov 17, 2018

Refactor and move BLE stuff to shared-bindings #995

Closed


		#if (MICROPY_PY_BLE_NUS == 1)

		static const char default_name[] = "CP-REPL"; // max 8 chars or uuid won't fit in adv data


		#include "shared-bindings/bleio/UUIDType.h"

		//\| .. currentmodule:: bleio

WIP: bleio rewrite #1289

WIP: bleio rewrite #1289

Uh oh!

Conversation

arturo182 commented Oct 21, 2018 • edited by dhalbert Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhalbert commented Oct 21, 2018

Uh oh!

dhalbert commented Oct 30, 2018

Uh oh!

dhalbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhalbert commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturo182 commented Nov 5, 2018

Uh oh!

arturo182 commented Nov 5, 2018

Uh oh!

dhalbert commented Nov 7, 2018

Uh oh!

Uh oh!

arturo182 commented Oct 21, 2018 •

edited by dhalbert

Loading

dhalbert commented Nov 5, 2018 •

edited

Loading