D21584: Add LE Advertising and GATT APIs

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  Looks really good!

INLINE COMMENTS

> gattapplication.cpp:38
> +
> +GattApplication::GattApplication(QObject *parent) :
> +    ObjectManager(parent),

Coding style:

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

> gattapplication.cpp:53
> +
> +    auto serviceAdaptors = findChildren<GattServiceAdaptor*>();
> +    auto charcAdaptors = findChildren<GattCharacteristicAdaptor*>();

const

> gattapplication.cpp:64
> +
> +        GattService* service = qobject_cast<GattService*>(serviceAdaptor->parent());
> +        if (service) {

`GattService *service`

> gattapplication.h:66
> +
> +    DBusManagerStruct getManagedObjects() const override;
> +

Should this be made private (preferrably in GattApplicationPrivate)?

> gattapplication_p.cpp:31
> +    static uint8_t appNumber = 0;
> +    m_objectPath.setPath(QStringLiteral("/org/bluez/app") + QString::number(appNumber++));
> +}

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/`.

> gattcharacteristic.cpp:50
> +
> +void GattCharacteristic::writeValue(const QByteArray& value)
> +{

`const QByteArray &value)`

> gattcharacteristic_p.cpp:33
> +{
> +    static uint8_t charcNumber = 0;
> +    m_objectPath.setPath(m_service->objectPath().path() + QStringLiteral("/char") + QString::number(charcNumber++));

Any reason to use uint8?

> gattcharacteristic_p.h:39
> +    QDBusObjectPath m_objectPath;
> +    QByteArray      m_value;
> +    GattCharacteristic::ReadCallback m_readCallback = nullptr;

No additional spaces please.

> gattcharacteristicadaptor.cpp:57
> +    return m_gattCharacteristic->readValue();
> +}
> +void GattCharacteristicAdaptor::WriteValue(const QByteArray &value, const QVariantMap &/*options*/)

newline

> gattcharacteristicadaptor.h:58
> +private:
> +    GattCharacteristic* m_gattCharacteristic;
> +};

`GattCharacteristic *m_gattCharacteristic`

> gattmanager.cpp:59
> +
> +    for (auto child : application->children()) {
> +        GattService* service = qobject_cast<GattService*>(child);

const auto children =  application->children();
  for (const auto &child : children()

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

> gattmanager.h:103
> +private:
> +    explicit GattManager(const QString &path, QObject *parent = nullptr);
> +

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

> gattservice.h:50
> +     */
> +    GattService(const QString &uuid, bool isPrimary, GattApplication *parent);
> +

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?

> leadvertisement.h:52
> +     */
> +    explicit LEAdvertisement(const QStringList &serviceUuids, QObject *parent = nullptr);
> +

Same as above, add setters?

> objectmanager.h:47
> + */
> +class BLUEZQT_EXPORT ObjectManager : public QObject
> +{

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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D21584

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190605/ebeb41e6/attachment-0001.html>