InfiniTimeOrg / InfiniTime

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

Moving class constructors to SystemTask #1493

Open Riksu9000 opened 1 year ago

Riksu9000 commented 1 year ago

Currently everything is constructed in main and passed to both DisplayApp and SystemTask. We could move almost everything, including DisplayApp to SystemTask, to limit their scope. I want your feedback on this before I create a PR, so I won't have to maintain the PR for long.

This relates to #1468, as it shows how the scope of MotorController can be limited for example.

minacode commented 1 year ago

Limiting scope is a good idea.

Do you know if this is compatible with the RTOS tasks? There is one for SystemTask and DisplayApp each, right?

Also the objects were created like this to compile them to the image and avoid putting them in the RAM at runtime if I remember right. But I guess that would still be fine.

A graph showing what uses what would be nice to get the overall structure.

JF002 commented 1 year ago

Originally, I decided to declare all the objects (driver, components,...) in the global scope, in main.cpp. The idea was that those objects should always exist (we won't remove the touchpanel at runtime) and there's no need to instantiate them dynamically (we have only 1 display and we won't add one at runtime either).

Looking backwards, this was not the best idea :

So yes, moving the declarations of these objects in SystemTask and DisplayApp definitely makes sense and will make the ownership of those objects more explicit. :+1:

Note that if you declare them as class members, you'll probably have to increase the stack size of the tasks.

Also the objects were created like this to compile them to the image and avoid putting them in the RAM at runtime if I remember right. But I guess that would still be fine.

@minacode Those objects already live in RAM since they are not const.

Riksu9000 commented 1 year ago

I was thinking of leaving SystemTask in the global scope, so I believe the rest of the classes would still be statically initialized.