Add support for AIDL binder interface by mlehtima · Pull Request #31 · sailfishos/sensorfw

mlehtima · 2025-07-06T19:31:46Z

No description provided.

mlehtima · 2026-03-04T22:32:49Z

Reworked the PR to split the backends into separate classes to make the code more readable.

Don't wait for hwbinder servicemanager when checking if hidl backend is supported. One new devices hwbinder servicemanager is not present anymore and waiting for servicemanager would cause the service to wait forever.

abranson · 2026-04-18T16:55:43Z

-    }
-#endif
-
    delete[] m_sensorState;


Is this stuff still done when the binderDieds happen in the backends? It seems like the rest of this is copied there, but this stuff isn't done anymore. Is that an issue?

abranson · 2026-04-18T17:26:55Z

+        g_free((void*)m_sensorArray[i].requiredPermission.data.str);
+    }
+    delete[] m_sensorArray;
+    m_sensorArray = NULL;


Should m_sensorCount should be set to 0 here?

Yea that would be symmetric. Now it's initialized from initialize(), cleanup() could then deinit. Though the init is in derived classes which makes this spread out on multiple files. Got myself confused for a moment as nothing in this class assigns to m_sensorCount but reads the value.

Could be slightly simpler to follow with some actual container type rather than plain array.

pvuorela

Some quick comments, didn't have time to go through all yet.

pvuorela · 2026-04-20T12:25:00Z

+****************************************************************************/
+
+#ifndef HybrisBackendHal_H
+#define HybrisBackendHal_H


Minor detail but the CamelCased if guard is slightly unusual. Suspect it might have been some search & replace thing here. Some other ones also in the PR.

pvuorela · 2026-04-20T12:25:37Z

+public:
+    HybrisBackendHal(HybrisManager *manager, QObject *parent = 0);
+    ~HybrisBackendHal();
+    void initialize();


Bonus points for marking override on the reimplemented methods.

pvuorela · 2026-04-20T12:35:58Z

+    virtual float resolution(int index) = 0;
+    virtual int setActive(int handle, bool active) = 0;
+    virtual int setDelay(int handle, int64_t delay_ns) = 0;
+    virtual void eventReaderThreadImpl() = 0;


I can underdstand Impl in e.g. class names in comparison to abstract ones, but on one method it looks strange. All the methods are "Impl" in the subclasses?

The name neither tells what it does and not sure even the thread part.

pvuorela · 2026-04-20T12:42:13Z

+**
+****************************************************************************/
+
+#ifdef USE_BINDER


Should anything_binder.cpp be even ever compiled without USE_BINDER?

pvuorela · 2026-04-20T12:44:53Z

+    HybrisBackend *backend = NULL;
+    for (int i = 0; i < 5; ++i) {
+        if (HybrisBackendBinderAidl::isSupported()) {
+            backend = qobject_cast<HybrisBackend *>(new HybrisBackendBinderAidl(manager));


The backends should all inherit HybrisBackend, thus not need any casting? Applies to all getBackend().

pvuorela · 2026-04-20T12:45:11Z

+
+HybrisBackend *getBackend(HybrisManager *manager)
+{
+    HybrisBackend *backend = NULL;


Bonus points for nullptr.

pvuorela

Bunch of more comments.

pvuorela · 2026-04-21T06:15:54Z

-        gbinder_remote_reply_unref(reply);
-        gbinder_local_request_unref(req);
+    if (m_backend) {
+        m_backend->cleanup();


This seems to be the only external use for the backend cleanup(). Since it's followed by deletion, could just make the deletion do cleanup and have one less method in public api? Internally the backends can keep such if they need to clean up state in the middle.

Then again now it's symmetric with separate initialize() but that also seems to be called right after getBackend() so the ctors could just do that?

pvuorela · 2026-04-21T06:17:22Z

+    int m_sensorCount;
+};
+
+HybrisBackend *getBackend(HybrisManager *manager);


Could this be a static method in HybrisBackend? Seeing the call in HybrisManager I first thought it was a method in that class and was confused for a while where it's defined.

pvuorela · 2026-04-21T06:21:59Z

+    HybrisBackend(HybrisManager *manager, QObject *parent = 0);
+    virtual void initialize() = 0;
+    virtual void cleanup() = 0;
+    virtual int sensorCount();


A single non-abstract virtual method is slightly peculiar. Does the trick with protected m_sensorCount but leaves me stll wondering would it be simpler if the backends just implemented keeping the count themselves. Or alternatively should this be non-virtual.

No biggie though.

pvuorela · 2026-04-21T06:33:19Z

+    virtual int minDelay(int index) = 0;
+    virtual float maxRange(int index) = 0;
+    virtual float resolution(int index) = 0;
+    virtual int setActive(int handle, bool active) = 0;


The api with almost everything fetched as index might be cleaner with some sensor type returned by index and that having this sort of methods.

For another detail almost all is having index parameter but then couple things are based on handles. Then internally the code does some indexForType() -> handle() resolving. Didn't go through all but just wondering if there needs to be a handle exposed. Index is sort of handle too.

This more as suggestion rather than requirement to change.

pvuorela · 2026-04-21T06:50:37Z

+        g_free((void*)m_sensorArray[i].requiredPermission.data.str);
+    }
+    delete[] m_sensorArray;
+    m_sensorArray = NULL;


Yea that would be symmetric. Now it's initialized from initialize(), cleanup() could then deinit. Though the init is in derived classes which makes this spread out on multiple files. Got myself confused for a moment as nothing in this class assigns to m_sensorCount but reads the value.

Could be slightly simpler to follow with some actual container type rather than plain array.

pvuorela · 2026-04-21T06:52:22Z

+    int index = m_manager->indexForHandle(eve->sensor);
+    const struct sensor_t *sensor = &m_sensorArray[index];
+    if (sensor->flags & SENSOR_FLAG_WAKE_UP) {
+        return true;


Minor thing but this is classic 'if (isTrue()) return true; else return false;'

Could be just "return (sensor->flags & SENSOR_FLAG_WAKE_UP);"

pvuorela · 2026-04-21T06:56:27Z

+    float maxRange(int index);
+    float resolution(int index);
+    virtual int setActive(int handle, bool active) = 0;
+    virtual int setDelay(int handle, int64_t delay_ns) = 0;


No point in redeclaring abstract methods in parent class as still abstract? Same for initialize() and needsReaderThread().

pvuorela · 2026-04-21T07:00:23Z

+    Q_OBJECT
+public:
+    HybrisBackendBinder(HybrisManager *manager, QObject *parent = 0);
+    virtual ~HybrisBackendBinder() = 0;


This exists? And dtors as = 0 in general would be strange.

pvuorela · 2026-04-21T07:08:37Z

+
+class HybrisManager;
+
+class HybrisBackend : public QObject


Is there actually any use for the QObject inheritance? Not spotting signals nor slots, and the instance is explicitly deleted, not needing parenting.

mlehtima mentioned this pull request Jul 6, 2025

Add support for writing AIDL FMQs mer-hybris/libgbinder#144

Merged

spiiroin approved these changes Jul 7, 2025

View reviewed changes

Comment thread core/hybrisadaptor.cpp Outdated

Comment thread core/hybrisadaptor.cpp Outdated

Comment thread core/hybrisadaptor.cpp Outdated

Comment thread core/hybrisadaptor.cpp Outdated

mlehtima force-pushed the jb61406 branch from fce8a7b to 3f03761 Compare July 7, 2025 12:40

mlehtima force-pushed the jb61406 branch from 3f03761 to 396e44c Compare March 4, 2026 22:23

monich reviewed Mar 5, 2026

View reviewed changes

Comment thread core/hybrisbackend.h

monich reviewed Mar 5, 2026

View reviewed changes

Comment thread core/hybrisbackend.h Outdated

mlehtima added 2 commits March 5, 2026 15:25

[hybrisadaptor] Split hybris backends to separate classes. JB#61406

16b56bc

Don't wait for hwbinder servicemanager when checking if hidl backend is supported. One new devices hwbinder servicemanager is not present anymore and waiting for servicemanager would cause the service to wait forever.

[hybrisadaptor] Add support for AIDL binder interface. JB#61406

fabd2ac

mlehtima force-pushed the jb61406 branch from 396e44c to fabd2ac Compare March 5, 2026 13:26

abranson reviewed Apr 18, 2026

View reviewed changes

pvuorela reviewed Apr 20, 2026

View reviewed changes

pvuorela reviewed Apr 21, 2026

View reviewed changes

Conversation

mlehtima commented Jul 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlehtima commented Mar 4, 2026

Uh oh!

Uh oh!

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!

pvuorela 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!

pvuorela 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants