InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.65k stars 912 forks source link

Purpose of MotionController #1679

Open Riksu9000 opened 1 year ago

Riksu9000 commented 1 year ago

Verification

Introduce the issue

What is the purpose of MotionController? We just copy data from the driver to it and add a few functions. Why not use the driver directly?

Preferred solution

Remove/split MotionController and use the driver directly. The trip counter could be another class, maybe the step counter as well.

Version

No response

JF002 commented 1 year ago

The idea behind MotionController (like many other Controllers like MotorController, BatteryController, BrightnessController,...) is to decouple the application from the low-level driver and the hardware.

The goal is to implement only the low-level processing in the driver : configure the sensor, read raw data from it, maybe handle IRQ, DMA,... Then, the Controller layer uses this low-level driver to provide a higher level service and API to the application.

Some of the reasons for this design are

This is probably not clear if you only look at MotionController, but look at the HeartRateController. The application cannot directly use the raw data from the sensor. So, instead of converting data in the low level driver of in the GUI, the processing is implemented in the Controller layer.

MotionController adds the ShakeWake and RaiseWake algorithm, so I wouldn't say it's useless.

Also, removing the MotionController and directly using the low level driver breaks the intend of the design (application does not directly talk to the hardware) and is not consistent with other parts of the projects.

Riksu9000 commented 1 year ago

Sorry I wasn't clear enough in the main post. I mainly wanted to bring attention to the fact that the controller was used to access raw sensor information, and hadn't planned out what should be done about it entirely. The PR #1700 should address this appropriately.