ubports / unity8

The operating environment for everywhere. Lomiri development has moved to https://gitlab.com/ubports/development/core/lomiri
https://lomiri.com/
GNU General Public License v3.0
722 stars 99 forks source link

Test and use hfd if Android SDK indicates it #414

Closed Flohack74 closed 2 years ago

Flohack74 commented 2 years ago

To get LEDs working again after H9 and most newer devices requiring that API. See also https://gitlab.com/ubports/community-ports/android10/project-management/-/issues/6

peat-psuwit commented 2 years ago

I don't particularly like tack-on override like this. At first, I would asked you to add another class implementing the same interface, and do detection at QML singleton providing time.

Although, there already exists the Hfd implementation in Unity8: https://github.com/ubports/unity8/commit/e236c3f810c844bbe096c24eb8abb22bf595e646

It completely replaces the old backend with Hfd, using Hfd's QML plugin. Alternatively, one could use Hfd's QML plugin in addition to the current plugin. Although, note that Hfd is suppose to work on older devices too, so we might as well consider doing the transition to Hfd now...

@mariogrip, what do you think? What's the current state of Hfd?

Flohack74 commented 2 years ago

@peat-psuwit we have LED regression now ongoing since months if not already for a whole year. I agree that this solution is not optimal, but at least it brings back functionality that is missing from the product. The other PR looks nice, but will it work on old devices? Do we have again something thats on a branch and needs to be separately installed for >= H9 and Pinephone? The magic about continous integration of an ever-changing feature world is to make small, sometimes silly changes that might need refactor later - which can be expensive, but with the beauty of keeping users enabled and functions functioning. I find it really a poor approach that hfd was dropped in, stayed unfinished and the consumers around were partly adjusted to it. In the end Halium 9 was regressing from the state in H7.1 and 5.1 in at least vibrator and LEDs API a lot, and I dont think we are serving the users by keeping the fucntionality broken until we find the perfect long-term solution. Software development in 2021 is no longer a waterfall, but a series of iterations that should ideally never do breaking changes, work a lot with feature flags and parallel implementations, and always strive to prevent long-lived feature branches: Modify, Integrate test, rinse & repeat.

Flohack74 commented 2 years ago

I also can add that what @mardy said in https://github.com/ubports/hfd-service/issues/22 is true and I dont like to have to call 5 times into DBus for enabling a darn LED, and yet I still would use the current hfd API for LEDs to reach my goal. I could also say we need to refactor whole hfd and make better APIs inside, and until we reach that goal nobody should use it, but thats the wrong approach. The right approach is: Think about the usability of the product, and see it from the point of a user. Users will not care for the technical solution so much but want a result. So, give them a result, and then lets make it better. And dont make breaking changes that stay around for months and years.

peat-psuwit commented 2 years ago

If I understand it correctly, the issue affects Android 10 ports and Android 9 ports lacking legacy .so-based Light HAL. Have LED ever worked on those? If it has never it's not actually a regression...

The other PR looks nice, but will it work on old devices?

I believe it should, given that Hfd-service is included in the image.

And, if you prefer to keep the old code, I'm not opposed to that. What I'm asking is a better way to integrate the new code it. Either:

Flohack74 commented 2 years ago

Well it might have never worked for those devices, but as a user I don't care for the low-level peculiarities: If I am a user changing a device from an old Nexus 5 to a newer device then it will feel like a regression. Technically its not a regression of that kind what you think, but its an incomplete implementation of a feature that drags on, and in my opinion that's even worse: Something was started, but then, for whatever reason, energy was lost to complete it. And that happens mostly when one task is too big to be solved in a reasonable time frame, including testing and documentation, if possible. Its about slicing and scoping out until you get an implementable unit of functionality that can be easily integrated. Well with my limited time this is off the plate for me. Someone else need to pick that up then. I have a solution that works for me, to get me LED notifications back, and I can install this branch if I update my device.

amartinz commented 2 years ago

And i agree to Florian's point of view. We should rather unblock users now and be more agile, especially with all the limited resources we have.

End users care if something works and not how technically perfect it is implemented, because most of them are not software architects and bother diving into the code.

From a users point of view all they see "They can not even make a simple light blink, how should i have faith in them being able to manage a whole operating system and trust them with my data when using it as daily driver?" But that is a whole different discussion and off-topic.

The changes itself look fine and leightweight to me and they are not intrusive. If there are bigger plans for a certain direction, they can always be pursued but i would prefer to get things in a working state as soon as possible instead of postponing and hoping that we ever get the resources, time and motivation to work on it.

Flohack74 commented 2 years ago

Alright I went for a subclassed approach as this requires equal work than all the review comments probably ;)

Flohack74 commented 2 years ago

Yeah, the Vollaphone does not have a LED :) I will re-test since I made substantial changes on my SHIFT 6mq

Flohack74 commented 2 years ago

Aaand it still works. nice. Lets do it!

cibersheep commented 2 years ago

Device: Arale Channel: RC 2021-W45 Result: Leds seem to work as previously (on notification), tapping on home button when screen is on and when phone is off and charging

Flohack74 commented 2 years ago

We also need a test from a few devices >= Halium9

amartinz commented 2 years ago

I can confirm that it started working for a Halium 10 device with this change, where it did not work previously (axolotl - SHIFT6mq). But you already know that :smile:

jranaraki commented 2 years ago

LED works on Nexus 6P on RC updated on Nov 17th. If it helps, the LED on Nexus 6P never stopped working.