Add LE Advertising and GATT APIs
ClosedPublic

Authored by mweichselbaumer on Jun 4 2019, 7:25 PM.

Diff Detail

Repository
R269 BluezQt
Branch
ble_gatt
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12531
Build 12549: arc lint + arc unit
mweichselbaumer created this revision.Jun 4 2019, 7:25 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 4 2019, 7:25 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mweichselbaumer requested review of this revision.Jun 4 2019, 7:25 PM
drosca requested changes to this revision.Jun 5 2019, 7:47 AM

Looks really good!

src/gattapplication.cpp
39

Coding style:

​GattApplication::GattApplication(QObject *parent)
    : ObjectManager(parent)
    , d(new GattApplicationPrivate())
{
}
54

const

65

GattService *service

src/gattapplication.h
67

Should this be made private (preferrably in GattApplicationPrivate)?

src/gattapplication_p.cpp
32

Shouldn't the caller be made responsible for choosing object path?
If we force a path then we should use "our" namespace - /org/kde/bluez-qt/.

src/gattcharacteristic.cpp
51

const QByteArray &value)

src/gattcharacteristic_p.cpp
34

Any reason to use uint8?

src/gattcharacteristic_p.h
40

No additional spaces please.

src/gattcharacteristicadaptor.cpp
58

newline

src/gattcharacteristicadaptor.h
59

GattCharacteristic *m_gattCharacteristic

src/gattmanager.cpp
60
const auto children =  application->children();
for (const auto &child : children()

Also you can use findChildren<GattService*> as above.

src/gattmanager.h
104

This should have d-ptr as it's exported class.

src/gattservice.h
51

Probably would be better to add setters for uuid/primary (and note that it can only be set during creation), as if we need in future more properties we will need to add new constructors.
Or make it virtual?

src/leadvertisement.h
53

Same as above, add setters?

src/objectmanager.h
47 ↗(On Diff #59151)

Will this be used for more classes? Right now only GattApplication inherits it.

This revision now requires changes to proceed.Jun 5 2019, 7:47 AM
mweichselbaumer marked 11 inline comments as done.Jun 5 2019, 8:43 AM
mweichselbaumer added inline comments.
src/gattapplication_p.cpp
32

Ok, will add possibility for custom prefix.

src/gattcharacteristic_p.cpp
34

I tried to figure out the maximum number of charcs (per service). I do not know exact numbers, but uint8 would intrinsically add limits.

src/gattservice.h
51

GATT is well defined and i do not expect any changes (regarding additional properties).
This also follows the RAII idiom and is less error prone.
Could be made virtual though, however i believe adding further constructors won't harm.
Or if further properties are coming, we add setters for them, since they are then expected not to be mandatory.

src/leadvertisement.h
53

see above

src/objectmanager.h
47 ↗(On Diff #59151)

QDbusAdaptors can only realize one single DBUS interface.
This is the base class the org.freedesktop.DBus.ObjectManager adaptor is handling.
Sure, this could (and should) be reused if other DBUS/BlueZ objects shall realize this interface.
GattApplication is meant to realize the org.freedesktop.DBus.ObjectManager interface (see gatt-api.txt).

mweichselbaumer marked an inline comment as done.

Fixed according to comments

Fixed according to comments

Fixed according to comments

drosca requested changes to this revision.Jun 6 2019, 6:18 AM

It seems you uploaded only diff with latest changes, you need to upload the entire diff against master.

src/gattapplication.h
47

Maybe it would be best to add constructor that uses default object path. Also please add better documentation as to what object path prefix is.

explicit GattApplication(QObject *parent = nullptr);

This revision now requires changes to proceed.Jun 6 2019, 6:18 AM

Fixed according to comments

mweichselbaumer added inline comments.Jun 6 2019, 6:03 PM
src/gattapplication.h
67

I override this in test class, so kept as protected.

drosca requested changes to this revision.Jun 7 2019, 8:10 AM

After this, it should be good to go.

src/gattapplication.h
47

Here I meant adding new constructor, because as you did it now, if you want to set parent from constructor you would need to copy the default value for the objectPathPrefix.

GattApplication(QObject *parent = nullptr);
GattApplication(const QString &objectPathPrefix, QObject *parent = nullptr);
src/objectmanager.h
47 ↗(On Diff #59151)

I don't really like this class at all. This is implementation detail, and is of no use for the users of the library, so it shouldn't be exported. On top of it, there is actually no code, it is just interface class.

Another issue here, although minor, is that the bluezqt_dbustypes.h is not installed, so you can't use it. Yes, you could install it, but we don't need it.

As I can see, the only reason for this class is so you can pass it to ObjectManagerAdaptor, but there are other ways to achieve the same thing without adding ObjectManager and deriving from it in GattApplication. If we really need it in generic way, we can do it later.

Something like this would work:

ObjectManagerAdaptor(QObject *parent)
{
    GattApplication *app = qobject_cast<GattApplication*>(parent);
}

DBusManagerStruct GetManagedObjects()
{
    return app->d->getManagedObjects();
}

Or for now just make it work only with GattApplication, as this is the only one we have now.

Yes, it will not work in the autotest, since you won't have the hook anymore, but you can just remove that test.

This revision now requires changes to proceed.Jun 7 2019, 8:10 AM
mweichselbaumer marked an inline comment as done.Jun 7 2019, 10:04 AM
mweichselbaumer added inline comments.
src/objectmanager.h
47 ↗(On Diff #59151)

I see your point and agree, that ObjectManager is solely used in GattApplication.
Just letting you know, that we would need an ObjectManager for the mesh-api as well.

Shall we then add constructors for appropriate types to ObjectManagerAdaptor (instead of exporting and inheriting from ObjectManager)?

Fixed according to comments

drosca accepted this revision.Jun 7 2019, 2:42 PM
drosca added inline comments.
src/gattapplication.h
86

This should be protected

97

This doesn't need doc anymore.

src/gattcharacteristicadaptor.cpp
63

newline

src/gattservice.h
51

explicit

This revision is now accepted and ready to land.Jun 7 2019, 2:42 PM

Fixed according to comments

ngraham added a subscriber: ngraham.Jun 7 2019, 4:17 PM

What is the net effect of this patch? What does it improve?

drosca added a comment.Jun 7 2019, 4:19 PM

What is the net effect of this patch? What does it improve?

It adds initial support for Bluetooth Low Energy, it's not used anywhere in Plasma.