GeezerVison

Unless you've benchmarked this and it makes a noticeable difference, it's probably best to let the compiler decide (drop the expect)

Idea was to have minimal performance impact during actual file transfer and firmware update (by instructing compiler that branching should be optimized for the scenario when the condition is false).
However I tested the performance and in both cases it is around 6.65 KB/s from around 1 m distance.
__builtin_expect removed.

I totally agree with you technically (makes sense to optimise the enabled case), the only reason I pointed this out is that the number of packets per second is never high enough for it to make a big difference (it is easy to forget how fast the CPU is - I'm pretty sure it'd be bottlenecked by the max bandwidth of BLE at about 700kbps) and therefore IMO the "less code wins" rule takes precedence

Could very well be that CPU is never the bottleneck in this case (I haven't thought much about this).
Maybe the flash memory speed is the bottleneck (as the average firmware transfer speed is around 50-55 kbps - at least for me).

The small packet size used for DFU (20 bytes) is probably the biggest factor. Due to the somewhat fragile way DFU is currently implemented, any size that evenly divides 200 bytes is actually acceptable. It would be interesting to see what throughput with 200 byte packets would be, but unfortunately due to a bug with the driver for my laptop's BT chipset I can't test it currently! I need to debug the driver at some point but my kernel skills aren't too sharp...

The flash memory is really fast (>100mbit/s when used properly), it's bottlenecked by the SPI bus for sure (8mbit/s)

Although it doesn't make too much of a difference, maybe apply this to bufferSettings inside the if clause. That way it's clear that work is being done only when settings are successfully loaded, and it means that settings only gets updated once

@mark9064 I would prefer to leave this as it is now. Currently it will fail-secure (set settings.dfuAndFsMode to DfuAndFsMode::Disabled) if something very unexpected happens.

@NeroBurner This is the reason why DfuAndFsMode::EnabledTillReboot should never get restored from the saved settings on reboot.

Sorry if it was unclear, I'm suggesting you do this

  if (bufferSettings.version == settingsVersion) {
    if (bufferSettings.dfuAndFsMode == DfuAndFsMode::EnabledTillReboot) {
      bufferSettings.dfuAndFsMode = DfuAndFsMode::Disabled;
    }
    settings = bufferSettings;
  }

I believe this does not change how the code behaves at all?

This is exactly the thing I would like to keep as is (unless requested by the maintainers to change it).
If possible I would like to keep this code as it is and keep it at the very end of Settings::LoadSettingsFromFile().

The change you suggest would not change how the code behaves.
However in case of refactoring it would increase the chances of the if (bufferSettings.dfuAndFsMode == DfuAndFsMode::EnabledTillReboot) { to be forgotten or moved to a code path that is not always executed.

Maybe I should add a code comment above if (settings.dfuAndFsMode == DfuAndFsMode::EnabledTillReboot) { telling that this (when needed) switches off DFU and FS access after reboot?
Or, should I make the requested code change + a code comment?

I think I'd personally prefer the suggested change with a comment explaining that this is needed for firmware+files security to work properly.

Maybe it'd be better to fix this in the data format itself though. What about introducing a new boolean member of Settings.h that's not inside the Settings struct that stores whether DFU+FS is temporarily enabled.
Then GetDfuAndFsMode can compute the correct enum value using the setting+the member, and SetDfuAndFsMode can unpack the state into Disabled,Disabled+member set,Enabled. That way it will be impossible to break when refactoring and we don't need to mess with saving/loading settings

@mark9064 Thanks! Two bool variables (one in settings and the other one outside) sounds like a really good idea, I have implemented it (see the recent push).

Is this change related to this patchset? Or is it an unrelated bug fix

This is unrelated fix.

Could you open a separate PR for the fix with a nice explanation what is fixed here please 🙏

I think it's to do with the fact that we don't use the notification buffer optimally (or even correctly really). I've proposed fixes for this in #1694 and #1695, but I haven't followed up on them.

@NeroBurner Sure! I will make a separate PR for this.
@FintasticMan I think when copying a string using memcpy() the size always needs to be strlen() of the string plus 1 for the trailing zero-byte string terminator (unless the destination memory is guaranteed to be prefilled with zero-bytes).

@NeroBurner Created the PR #2159