@NeroBurner

I like the usage of the impl reference, as I was wary of the compile time overhead if so much code must be in the headers as templates require. But with the impl reference this should be ok as the heavy lifting is in the impl.cpp file

In theory (and according to my experiments with compiler explorer), those calls to impl.xxx() should be optimized by the compiler, since the type of impl is known at compile time.

Looks like the new cmake Unity build feature does not like some of those changes...

I think this PR is ready for review.

In this PR, I added a new concept and proxy object for the following low-level drivers:

• Display (St7789)
• HeartRateSensor (HRS3300)
• MotionSensor (BMA42x)
• SpiMaster
• FlashMemory
• TouchPanel
• TwiMaster
• Watchdog

The goals of these changes are:

• They allow to easily customize the implementation of a low-level driver. This will be useful to support variants of the PineTime (Colmi P8, for example).
• They also allow a cleaner implementation in InfiniSim (corresponding PR).
• This is a first step to a complete hardware abstraction that will allow to port InfiniTime on other devices and implement unit tests, for example.
• This is a cleaner design that is more expressive and that explicitly describes the interfaces that must be implemented to support new hardware without making use of runtime polymorphism.
• I personally think this is a great use of this new feature (concepts) of modern C++(20).

The current naming of the classes / namespaces are:

namespace Pinetime {
  namespace Drivers  {
    template <typename DeviceImpl>
    concept IsDevice = { /* ... */ };
    
    namespace Interface {
      template <class T>
        requires IsDevice<T>
      class Device {
      public:
         explicit Device(T& impl) : impl {impl} {}
      /* ... */
      private:
        T& impl;
      };
    }

    using Device = Interface::Device<ActualDevice>;
  }
}

Basically:

• the concept that describes the API of the classes are defined in Pinetime::Drivers. They are prefixed with Is to avoid name collision. This has already been discussed here and I haven't fond a better naming convention yet.
• The 'proxy object' that will forward the calls from the application to the actual implementation of the driver (needed to avoid runtime polymorphism) are declared in Pinetime::Drivers::Interface. We could maybe renamed this namespace as Proxy since that's what they are : proxy objects.
• The selection at build time of the actual interface is done in header files in port/. This is the only place where you'll see #ifdefs. The classes in Pinetime::Drivers::Interface are aliased in Pinetime::Drivers so that this new design causes minimal changes to the rest of the application.

TODO:

• Review the naming convention
• Write design documentation
• Update GCC -> Update GCC (in doc and docker) to GCC12 #1512
• Merge :)

It seems to me like there's no framework to choose the drivers yet, unless I'm missing something. How would you make two drivers selectable with CMake?

It seems to me like there's no framework to choose the drivers yet, unless I'm missing something.

The selection of the driver can be done by editing files in src/port (should I name this folder ports?). Currently, InfiniTime only supports the PineTime, so this is the only option available. There are a few variant that are supported (MOY-TFK5 MOY-TIN5 MOY-TON5 MOY-UNK) but they only need different clock settings, which was not in the scope of this PR.

I implemented this mecanism in InfiniSim in this PR. See this file, for example

I think, for example, that some Colmi P8 are using another touch panel. The file src/port/TouchPanel.h could be edited this way:

#pragma once
#include "drivers/TouchPanel.h"

#ifdef TARGET_DEVICE_PINETIME
  #include <drivers/touchpanels/Cst816s.h>
#endif

namespace Pinetime {
  namespace Drivers {
#ifdef TARGET_DEVICE_PINETIME
    using TouchPanel = Interface::Touchpanel<Pinetime::Drivers::TouchPanels::Cst816S>;
#elif TARGET_DEVICE_COLMI_P8
    using TouchPanel = Interface::Touchpanel<Pinetime::Drivers::TouchPanels::TheOtherTouchPanelDriver>;
#else
  #error "No target device specified!"
#endif
  }
}

How would you make two drivers selectable with CMake?

This is done using the variable TARGET_DEVICE, which is set to PINETIME by default.
We could probably avoid using #ifdefs here and include only the "port" file specific to the hardware target (by splitting port file by target).

In the future, I think we could also move the initialization of the objects (that are currently created and initialized in the global scope in main.cpp) so that we can apply different parameter values to the constructors of the driver (ex : to apply a different display mirroring). But I figured that this could be done in a future PR, once this design is approved.

What about main.cpp, where the touchPanel is hard coded to be constructed with the Cst816S implementation?

What about main.cpp, where the touchPanel is hard coded to be constructed with the Cst816S implementation?

Yes, right. I didn't encounter this issue in InfiniSim since it has its own main.cpp file, that is distinct from the one in InfiniTime.
To solve this in InfiniTime, we could move the initialization of the driver objects into another file, specific to the target, or even provide 1 main.cpp file per target. This would give more freedom to make custom initialization on a target basis.

@Riksu9000
May I suggest that we limit the scope of this PR to the re-design of the low-level hardware drivers while keeping exactly the same functionalities that the current project? I think we can work on actually choosing the drivers and implement support for other variants based on this design in a future PR (which I plan to work on, obviously) ?

Unless there's any doubt that this design is correct and that we'll be able to actually support multiple hardware with it ?

https://stackoverflow.com/questions/21152018/is-it-worth-it-to-avoid-polymorphism-in-order-to-gain-performance

Reading the linked discussion it seems like avoiding runtime polymorphism might not make a big difference, but it makes the architecture more confusing. I don't really understand why it wouldn't get optimized without the proxy, but I think it could be worth the tradeoff to make the code significantly simpler.

This abstraction scheme as-is is unable to handle multiple differing implementations of the same concept being needed at runtime, for example if trying to port to a device that has two different SPI flashes on board, or (if this is extended to GPIOs), if some things are wired up to GPIOs from the SoC and some are going through an I2C GPIO expander chip. Handling these cases would require either explicitly templating the user on the peripheral type, or introducing virtual functions.

@Riksu9000

Reading the linked discussion it seems like avoiding runtime polymorphism might not make a big difference, but it makes the architecture more confusing

This is an interesting discussion! Let me (try to) explain the goals of this new design.

The first goal of this design is not optimizations. Instead, it's more about "semantics" : I want the design to be as close as the hardware as possible.

InfiniTime is a firmware for smartwatches. To the InfiniTime point of view, a smartwatch is a device with a display, a touchpanel, a motion sensor, a heart rate sensor, a button, an external memory,... This list of peripherals is know and does not change : every PineTime has the exact same hardware.
Also, the kind of smartwatch targeted by InfiniTime is based on a very memory and resource constraint hardware : low power cpu, low RAM, low FLASH.

So the idea is to provide a design that explicitly builds one and only one instance of each driver : the one that will be needed at runtime for a specific hardware.

Also, the concept brings a nice way to describe the interface of a driver, in my opinion.

In the end, I wanted this design to be as close as what we currently have in InfiniTime : everything is statically resolved and allocated.

BUT... you said that this new implementation is confusion, and that is certainly something I would like to avoid so I guess I still need to work a bit on that.

Using dynamic polymorphism and trust the compiler to optimize everything nicely might actually work! This is something I need to think about and experiment (compare).

@qookei

This abstraction scheme as-is is unable to handle multiple differing implementations of the same concept being needed at runtime

Is this really an issue? Up to now, I've I've never seen a smartwatch with multiple memory cheap (let alone 2 different ones), or multiple displays, multiple HR sensors,... Do you think there are reasonable reason to need to instantiate 2 different implementations of the same "category" ?

Is this really an issue? Up to now, I've I've never seen a smartwatch with multiple memory cheap (let alone 2 different ones), or multiple displays, multiple HR sensors,... Do you think there are reasonable reason to need to instantiate 2 different implementations of the same "category" ?

To be fair, in most cases I can't think of any applicable to InfiniTime, apart from potentially the aforementioned GPIO expanders vs SoC GPIO banks. I thought it still might be worth mentioning.

Is this really an issue? Up to [...]

I tend to agree that it's unlikely to be an issue for InfiniTime any time soon.

Thanks everyone for your feedback on this PR. I'll think a bit more about this new hardware abstraction since the additional complexity brought by the new design might not be needed after all.