Details
- Reviewers
drosca - Commits
- R269:1a5660ab1547: Add LE Advertising and GATT APIs
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
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? | |
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. | |
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. |
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). | |
src/leadvertisement.h | ||
53 | see above | |
src/objectmanager.h | ||
47 ↗ | (On Diff #59151) | QDbusAdaptors can only realize one single DBUS interface. |
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); |
src/gattapplication.h | ||
---|---|---|
67 | I override this in test class, so kept as protected. |
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. |
src/objectmanager.h | ||
---|---|---|
47 ↗ | (On Diff #59151) | I see your point and agree, that ObjectManager is solely used in GattApplication. Shall we then add constructors for appropriate types to ObjectManagerAdaptor (instead of exporting and inheriting from ObjectManager)? |