InfiniTimeOrg / InfiniTime

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

1.11 - Chimes setting makes the font in the G7710 Watchface disappear #1411

Closed inhji closed 8 months ago

inhji commented 2 years ago

Verification

What happened?

After the watch woke up due to the chimes setting, the font disappeared

What should happen instead?

Waking up from chimes should show the normal complete watch face

Reproduction steps

This is what triggers the bug for me:

The watchface can be restored by going into the settings and then returning back to the watchface.

More details?

No response

Version

1.11

Companion app

Flashed with itd

minacode commented 2 years ago

I searched for similar bug reports and found none was relevant.

This is #1376, right? 😉

Edit: ok, maybe not exactly..

inhji commented 2 years ago

This is what it looks like (sorry for the blurry picture). If this behaviours stems from the same source, I'll happily close this issue :)

PXL_20221101_203009809

minacode commented 2 years ago

This looks different, so it's fine. I think you are right and it stems from the same underlying issue, but keep it open, because it is relevant and no duplication.

HugoMskn commented 2 years ago

I have the exact same problem, but you found the cause IMG_20221102_000023

ITCactus commented 2 years ago

indeed, it's a different issue. looks like in this case external resources (fonts/images) are not displayed (tried with Infineat face #1024, Casio G7710 #1122, and Digistyle #1116).

note: while the issue claims it's for "G7710", the issue itself looks like system-wide. anyway, i will not be able to fix or debug the issue. so, anyone skilled for this case, feel free to bash this bug.

mark9064 commented 1 year ago

I can also reproduce this. When it wakes for the chime, the numbers are visible for an instant (can't tell if they are correct) before being replaced with the blank boxes as above. I tried the simulator (InfiniSim) and could not reproduce the issue there. This is on the 1.11 release

nand11 commented 1 year ago

I can reproduce this issue on 1.12. Scrolling up and down makes the font come back. But next time the watch wakes up, the font is gone again.

JF002 commented 1 year ago

I've never been able to reproduce such issue. My best guess is that sometimes LVGL cannot allocate a chunk of memory big enough to hold the whole font. This will hopefully be fixed with #1709 since more memory will be available to the watchface (I ran this branch for several days with chimes enabled and never noticed such issue).

However, there seems to be 2 behaviors : the font is just not displayed or it's corrupted. And I'm not sure if both behaviors are caused by this issue.

FintasticMan commented 1 year ago

I've been able to reproduce the issue of the font not being displayed at all with a build that has the unified heap. The min free value in SystemInfo reads 3024 after it happened, which leads me to believe that the issue isn't that there isn't enough free memory to allocate the font. I can't remember what the min free was before the chimes activated, so I'm not sure if it was higher before.

minacode commented 1 year ago

It could be a data race. Here is my theory without testing. Tldr: We need better syncronization of our concurrenct tasks when they use hardware components.

In SystemTask.cpp there is

case Messages::OnNewHour:
  // ...
  GoToRunning();
  displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);

The SystemTask and the DisplayApp run concurrently, so there could be a data race.

GoToRunning sets the SystemTask to WakingUp and queues the message GoToRunning:

void SystemTask::GoToRunning() {
  if (state == SystemTaskState::Sleeping) {
    state = SystemTaskState::WakingUp;
    PushMessage(Messages::GoToRunning);
  }
}

Once that message gets handled

case Messages::GoToRunning:
  // ...
  spiNorFlash.Wakeup();
  // ...
  displayApp.PushMessage(Pinetime::Applications::Display::Messages::GoToRunning);

is called.

Meanwhile in the DisplayApp there is:

case Messages::Chime:
  LoadNewScreen(Apps::Clock, DisplayApp::FullRefreshDirections::None);

which creates a watch face that (in the case of the casio watch face) calls

if (filesystem.FileOpen(&f, "/fonts/lv_font_dots_40.bin", LFS_O_RDONLY) >= 0) {
  filesystem.FileClose(&f);
  font_dot40 = lv_font_load("F:/fonts/lv_font_dots_40.bin");
}

and the filesystem is implemented as

int FS::FileOpen(lfs_file_t* file_p, const char* fileName, const int flags) {
  return lfs_file_open(&lfs, file_p, fileName, flags);
}

I am not sure about the internals of littlefs, but does this check if the memory is actually available?

What if the DisplayApp handles the Chimes message and creates the watch face before the SystemTask handles the GoToRunning message? It seems like in that case the FileSystem would read from memory that was not woken up yet. And if that read does not crash (which I don't know :grin:) then whatever gets loaded is null or gibberish.

mark9064 commented 1 year ago

Seems like you are right on the money minacode. I tried a small patch as so Messages::GoToRunning

           state = SystemTaskState::Running;
+          if (sendChimeAfterWake) {
+            displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);
+            sendChimeAfterWake = false;
+          }
           break;

Messages::OnNewHour/OnNewHalfHour

           if (state == SystemTaskState::Sleeping) {
+            sendChimeAfterWake = true;
             GoToRunning();
-            displayApp.PushMessage(Pinetime::Applications::Display::Messages::Chime);
            }

and the issue has been resolved. This change probably isn't sound (I haven't verified), but I think your identification of the root cause is correct

minacode commented 1 year ago

Nice work! 😊 I think we need a more general approach that ensures that hardware is available when being accessed.

nickalcock commented 1 year ago

Aside: I see this with Infineat as well as of 1.13.

JF002 commented 8 months ago

@minacode, @mark9064 thanks for this analysis of the issue and for the fix in #2019!

Tldr: We need better syncronization of our concurrenct tasks when they use hardware components.

Yes, I totally agree with you! This part of the code evolved organically, and probably deserve a nice refactoring :-)