fix: Prevent functional degradation if SD has less than 50MB free space (#3730)

[pfeerick]

cherry-pick of #3730

[philmoz]

There's an issue where the logsClose and loggingTimerStop calls at the end of logsWrite may be called every main loop iteration. You can see this in the trace output of the simulator in 2.9.

They should be inside an if (g_oLogFile.obj.fs) {} block like it was before this PR.

[pfeerick]

Both, or just logsClose? loggingTimerStop looks like it should only really do anything on the first iteration of logWrite after the function is no long active since loggingTimer would then be null.

Just to make things more spicier than usual, logging behaves a little differently on the simulator to the radio hardware... on the simulator, logsWrite is called directly in the main program loop (hence it will probably be spamming the debug/trace logs with file closes), whereas on the radio it runs on an RTOS timer, but only if some initialisation checks are met - i.e. the SD logging function needs to actually be active. Still a bug, but one that wouldn't be visible when debugging on the hardware. 😖

[philmoz]

loggingTimerStop used to be inside logsClose so to keep original functionality both should be in the if block.

[pfeerick]

Need to see why @mha1 moved that, as he separated it when adding the warning popup, and has a comment that implies the timer still running after log close triggers the UI warning message...

[mha1]

you are referring to the else path of the log SF is active if statement, right?

  else {
    error_displayed = nullptr;
    logsClose();
    
    #if !defined(SIMU)
    loggingTimerStop();
    #endif
  }

This piece of code is only active if the logs SF is off and you'd have to distinguish radio firmware and Simu:
radio firmware: after the logs SF is switched off logs are closed and the timer is stopped. The else path will only run once after SF off because the timer that calls logsWrite() is shut down. The timer will be restarted if the logs SF is activated again.
Simu: the else path is run every perMain() cycle without calling loggingTimerStop(). So logsClose() will be called every cycle. _(Simu should have dummies for loggingTimerStart() and loggingTimer(Stop), use them in perMain() and run logsWrite() only if the dummy timer is running to mimic radio firmware behavior)

But you are right for the current implementation, logsClose is called every cycle, but only in Simu. LogsClose will try to close a file that's already closed. No harm done because f_close() takes care of this. I agree it'll be better practice to check for g_oLogFile.obj.fs before trying to close the log file to stop unnecessary f_close() calls in Simu.

But it should be done in logsClose itself:

void logsClose()
{
  if (g_oLogFile.obj.fs && sdMounted()) {
    if (f_close(&g_oLogFile) != FR_OK) {
...

The reasons to move timer stop out of logsClose() was to have a consistent timer to function activated relation. The timer (which calls logsWrite()) is running if logs are activated (SF on) and stopped (SF off) when deactivated. First it's clean and second it enables a use case where SD card got full while writing logs and you'd delete a large file with the logs still enabled. It'll start a new log as soon as there is enough space again. As long as SD full persists it'll issue the popup (only if it's not already up) but keeps trying as long as SF the is activated. No worries about CPU consumption. Retries take much less time than logging itself.

[mha1]

Just tried this on 2.9. Makes Simu quiet again.

void logsClose()
{
  if (g_oLogFile.obj.fs && sdMounted()) {
    if (f_close(&g_oLogFile) != FR_OK) {
...  

[mha1]

• confirmed current 2.9 runs else path only once (after logs SF is turned off)
• confirmed above change works too
• will update PR if there is no objection

[pfeerick]

will update PR if there is no objection

It was your PR, please take up your concerns with the author 🤭

If you can't, patch/diff files are also accepted, I'm good at faking the commit author attribution by now 😇

[mha1]

Asked author. Says don't ask, do it.

Does that work ? logsClose.patch