Error when compiling new iptsd & tools on Android-x86 (BlissOS)

[hmtheboy154]

Hi, I've merged the latest iptsd and trying to build them on Bliss OS. I've followed the new requirement to add things like Eigen3 (it's already available on AOSP), fmtlib 9.x & INIReader and currently stuck at these error

https://gist.github.com/hmtheboy154/9f7a23a2f919cdab15fcaf155da49baf

If you search error: it will give about 20 results. I don't know what to do with these.

[StollD]

Either include Eigen with -isystem, this will disable all compiler warnings. Or remove -Wconversion and maybe -Wdouble-promotion.

Sorry for missing this in the announcement post about the Eigen changes.

[hmtheboy154]

It still have the same error, and pass -isystem make -fexceptions useless. Because we are trying to build with spdlog, we need -fexceptions

[StollD]

This is how the compiler command line looks on my device using meson:

c++ -Isrc/iptsd-check-device.p -Isrc -I../src -I/usr/include -flto=auto -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Wpedantic -Werror -std=c++17 -O3 -g -Wlogical-op -Wmissing-include-dirs -Winit-self -Wimplicit-fallthrough -Wdouble-promotion -Wconversion -D_GLIBCXX_ASSERTIONS -march=x86-64-v3 -DSPDLOG_SHARED_LIB -DSPDLOG_COMPILED_LIB -DSPDLOG_FMT_EXTERNAL -isystem/usr/include/eigen3 -MD -MQ src/iptsd-check-device.p/apps_check-device_main.cpp.o -MF src/iptsd-check-device.p/apps_check-device_main.cpp.o.d -o src/iptsd-check-device.p/apps_check-device_main.cpp.o -c ../src/apps/check-device/main.cpp

Using -isystem <eigen> instead of -I <eigen> just makes the compiler treat it as vendor code, where it would be useless to generate warnings, as you cant modify it. The same is done by default for stuff like /usr/include/linux.

I haven't heard anything about -isystem disabling exceptions, neither the GCC nor the Clang documentation seem to mention anything like it (but Clangs docs are kinda confusing). And in my builds exceptions work just fine, even though I dont explicitly add -fexceptions.

[StollD]

FWIW, if you cant make it build without warnings, just drop -Werror and live with the warnings I would say.

[hmtheboy154]

it still doesn't help. And I think all the warning that pointed to external/eigen are treated as warning instead of error already. Meanwhile these are the error in the iptsd *.hpp files, like this one.

device/generic/common/iptsd/src/contacts/detection/detector.hpp:151:24: error: no matching constructor for initialization of 'const Matrix2<double>' (aka 'const Matrix<double, 2, 2>')
                        const Matrix2<TFit> prec {{One<TFit>(), Zero<TFit>()},

I remember that when I put -Wlogical-op in, there are more error and they will point to external/eigen, but If I don't put them in it will reduce to only these 20 errors on iptsd.

[StollD]

Ah sorry, I totally missed that. Fixing these shouldnt be too difficult, probably just a case of that specific constructor not being available in your version of eigen. (Could you post which version Android uses? I tried to search it, but either I dont understand Android tags (likely), or they are shipping 3.2.2 which is very old).

Regarding the errors from signal-handler.hpp, I see you already fixed these before, like this: https://github.com/hmtheboy154/iptsd/blob/master/src/common/signal.hpp#L58-L62

[hmtheboy154]

Ah sorry, I totally missed that. Fixing these shouldnt be too difficult, probably just a case of that specific constructor not being available in your version of eigen. (Could you post which version Android uses? I tried to search it, but either I dont understand Android tags (likely), or they are shipping 3.2.2 which is very old).

Currently I'm building on 12L, and my target are 11/12L/13.

Regarding the errors from signal-handler.hpp, I see you already fixed these before, like this: https://github.com/hmtheboy154/iptsd/blob/master/src/common/signal.hpp#L58-L62

Oh..... that was your suggestion btw :D

[hmtheboy154]

Latest 13 QPR3 tag r52 use eigen 3.4.4, is it considered too old ?

[StollD]

Oh..... that was your suggestion btw :D

I totally forgot this. xD And I actually dont know why I didnt commit that fix here back then, gonna do that this time.

Latest 13 QPR3 tag r52 use eigen 3.4.4, is it considered too old ?

Its actually 3.4.0, which is the latest release. So that will work. You can also try this patch for the older versions:

diff --git a/src/contacts/detection/algorithms/gaussian.hpp b/src/contacts/detection/algorithms/gaussian.hpp
index a9bff34..f9b33af 100644
--- a/src/contacts/detection/algorithms/gaussian.hpp
+++ b/src/contacts/detection/algorithms/gaussian.hpp
@@ -146,10 +146,8 @@ void assemble_system(Matrix6<T> &m,
 template <class T>
 bool extract_params(const Vector6<T> &chi, T &scale, Vector2<T> &mean, Matrix2<T> &prec)
 {
-       prec.noalias() = -2 * Matrix2<T> {
-                                     {chi[0], chi[1]},
-                                     {chi[1], chi[2]},
-                             };
+       prec << chi[0], chi[1], chi[1], chi[2];
+       prec *= -2;

        // mu = sigma * b = prec^-1 * B
        const T d = prec.determinant();
diff --git a/src/contacts/detection/detector.hpp b/src/contacts/detection/detector.hpp
index 679f916..b271ba5 100644
--- a/src/contacts/detection/detector.hpp
+++ b/src/contacts/detection/detector.hpp
@@ -150,8 +150,9 @@ public:
                // Prepare clusters for gaussian fitting
                for (const Box &cluster : m_clusters) {
                        const Vector2<TFit> mean = cluster.cast<TFit>().center();
-                       const Matrix2<TFit> prec {{One<TFit>(), Zero<TFit>()},
-                                                 {Zero<TFit>(), One<TFit>()}};
+
+                       Matrix2<TFit> prec {};
+                       prec << One<TFit>(), Zero<TFit>(), Zero<TFit>(), One<TFit>();

                        // min() and max() are inclusive so we need to add one
                        const Vector2<Eigen::Index> size = cluster.sizes() + one;

[hmtheboy154]

Its actually 3.4.0, which is the latest release. So that will work. You can also try this patch for the older versions:

Oh cool.

I'll try the patch now, but with the given information you gave about eigen, I think I should just make another eigen fork and put it in external/eigen3 :p

[hmtheboy154]

It works, thanks

[hmtheboy154]

by the way Eigen define the version number on Eigen/src/Core/util/Macros.h, maybe we can take advantage of it

[StollD]

FYI, i noticed a pretty stupid mistake that I made when changing how matrices are initialized, I used the wrong index in one place. This manages to break literally everything.

This is the commit that fixes this: https://github.com/linux-surface/iptsd/commit/ecb85c7a06d2b0ba7cc3ac31111718981ec06335. Sorry for not testing my change beforehand.

I also had a look at your repository and I would strongly recommend you to always base your builds off of the last iptsd release and not the master branch. The reason is that I don't guarantee that the master branch will always work perfectly. Sometimes I push config options that have 100% guessed defaults because I didnt have time for fine-tuning them yet. Or code where I later discover that it silently breaks something.

[hmtheboy154]

FYI, i noticed a pretty stupid mistake that I made when changing how matrices are initialized, I used the wrong index in one place. This manages to break literally everything.

This is the commit that fixes this: ecb85c7. Sorry for not testing my change beforehand.

I also had a look at your repository and I would strongly recommend you to always base your builds off of the last iptsd release and not the master branch. The reason is that I don't guarantee that the master branch will always work perfectly. Sometimes I push config options that have 100% guessed defaults because I didnt have time for fine-tuning them yet. Or code where I later discover that it silently breaks something.

Well I also can't build 1.2.1, which is why I build master branch and send the issue to you. But after all things sorted out like this, maybe you should start finalizing into 1.2.2, so I'll start going for version instead of always master like this.

[StollD]

yeah, my suggestion would be to do v1.2.1 + cherry picking all the commits that you need for building. But right now, master seems to be fine, so its probably fine to ship it. It's just missing longterm testing.

[hmtheboy154]

yeah, my suggestion would be to do v1.2.1 + cherry picking all the commits that you need for building. But right now, master seems to be fine, so its probably fine to ship it. It's just missing longterm testing.

Oh don't worry. Actually on BlissOS I still haven't shipped builds with linux-surface kernel + iptsd "publicly" yet. We're still providing beta test build through Telegram or Discord