InfiniTime icon indicating copy to clipboard operation
InfiniTime copied to clipboard

P8: Mirror display for P8b variant

Open StarGate01 opened this issue 2 years ago • 3 comments

This PR has been broken out of https://github.com/InfiniTimeOrg/InfiniTime/pull/1050, and depends on / includes https://github.com/InfiniTimeOrg/InfiniTime/pull/1128 .

A new P8b hardware variant requires the display to be mirrored, and the colors to be inverted. Patch provided by @izzeho .

Related: https://github.com/StarGate01/p8b-infinitime/issues/3

StarGate01 avatar Jun 25 '22 18:06 StarGate01

@JF002 I like the idea of reducing #ifdefs. I used them because they require minimal changes to the code and classes, but I agree that they kind of break the OOP model.

How would you go about writing these variant-specific header files? A file (e.g. src/variants.h), which essentially does a similar selection as the CMake config (https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/src/CMakeLists.txt#L789), i.e. check TARGET_DEVICE, and then define the needed constexpressions, similar to the Pins in drivers/PinMap.h?

I dont think we can get rid of the CMake config completely, because it sets definitions for e.g. the NRF SDK as well.

This could also be used for control of the touch driver behaviour (https://github.com/InfiniTimeOrg/InfiniTime/pull/1130#discussion_r910661435), where @Riksu9000 was rightfully hesitant on littering the code with #ifdefs as well.

The acceleration driver (https://github.com/InfiniTimeOrg/InfiniTime/pull/1132) is already abstracted and split into multiple classes, so a simple flag can be used to select which implementation to use. I believe link-time-optimization will then remove the unneded code.

StarGate01 avatar Jul 07 '22 14:07 StarGate01

@StarGate01 Sorry for the late answer! I have already investigated some ways to support multiple hardware using a more OOP approach, but I haven't finished those researches yet. The idea was to find a way to correctly encapsulate device drivers so that it's easy to add alternative implementations and support multiple hardware (simulator, other smartwatches like the P8, experiments with the PineDio STACK,...).

I'll try to remember what I've already done (I mean... find the git stash with the code :D) and so we can move forward on that topic!

JF002 avatar Jul 21 '22 19:07 JF002

@JF002 regarding encapsulation, what do you think of the approach of #1132 ? There I split the implementations of the device-specific drivers into two specifications of an abstract class. That would be quite a bit of overkill for this PR imho.

I will prepare a variants.h approach as mentioned above, then we can discuss that ?

StarGate01 avatar Jul 24 '22 16:07 StarGate01

I think altering a single line with #ifdefs is simple and acceptable. In the case of functions, two or more functions can be defined and #ifdefs only choose which one gets called, which is what I would do in #1130 for example.

Riksu9000 avatar Aug 11 '22 08:08 Riksu9000

Yes, you are absolutely right @Riksu9000, those changes are pretty limited and harmless for the global architecture, and I think we can merge them.

I'm pretty sure we can find a better design than this to support multiple hardware variants, but I unfortunately don't have the time to work on that right now.

So, yeah, let's not block time any further. We'll still be able to improve the design in the future :)

JF002 avatar Aug 15 '22 11:08 JF002