Setting to disable DFU and FS access #1891

DavisNT · 2023-10-20T22:28:14Z

This PR adds setting to disable firmware update and Bluetooth file transfer.
This is 2. part for #1814 (comment)

github-actions · 2023-10-20T22:38:33Z

Build checks have not completed. Possible reasons for this are:

The checks need to be approved by a maintainer
The branch has conflicts
The firmware build has failed

DavisNT · 2023-10-21T21:51:39Z

Both issues discovered by the workflows should now be fixed.

I have force-pushed fix for the formatting error in SettingOTA.cpp.
For InfiniSim I have created Adjust NimbleController for changes in InfiniTime InfiniSim#125 Building this PR with InfiniSim PR should fix the InfiniSim build issue.

DavisNT · 2023-10-22T14:37:26Z

The build-simulator step is expected to fail, because it needs InfiniTimeOrg/InfiniSim#125

DavisNT · 2023-10-27T22:14:01Z

Made two improvements:

Added branch prediction hints, to minimize performance impact during real data transfer
Return BLE_ATT_ERR_INSUFFICIENT_RES when access is not allowed

Could it be possible to review this PR?

DavisNT · 2023-10-27T22:30:37Z

Rebased as well.

DavisNT · 2024-06-24T17:24:24Z

Is this PR (and the other PR described in #1814 (comment) ) welcome?
If I will rebase and adjust this PR to work with the latest changes, will it be merged?
@JF002, @FintasticMan, @Avamander Any feedback from project maintainers would be appreciated.

Expose NotificationManager and Settings for use by the feature in next commit. This is a memory efficient way for accessing SystemTask dependencies from controllers that have SystemTask injected as a dependency. Looks like each direct dependency injection uses 4 bytes RAM. As InfiniTime is close to running out of RAM (using 16 more bytes causes build to fail with "ld: region RAM overflowed with stack") it might be helpful to use this approach more.

DavisNT · 2024-06-30T18:13:31Z

Rebased (against main) and adjusted this PR. And done a few tests on my PineTime.

If there are questions about b2e2690 please see branch https://github.com/DavisNT/InfiniTime/tree/otaset-oom - this branch has the memory saving as the last commit (trying to build otaset-oom without last commit should cause the build to fail with /opt/gcc-arm-none-eabi-10.3-2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: region RAM overflowed with stack).

mark9064

Generally looking good to me! Works well on hardware
Few suggestions, feel free to challenge any of them

src/components/ble/DfuService.cpp

mark9064 · 2024-07-01T15:09:45Z

src/components/ble/DfuService.cpp

@@ -78,6 +80,18 @@ void DfuService::Init() {
 }

 int DfuService::OnServiceData(uint16_t connectionHandle, uint16_t attributeHandle, ble_gatt_access_ctxt* context) {
+#ifndef PINETIME_IS_RECOVERY
+  if (__builtin_expect(systemTask.GetSettings().GetDfuAndFsMode() == Pinetime::Controllers::Settings::DfuAndFsMode::Disabled, 0)) {


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)

src/components/ble/DfuService.h

src/components/ble/FSService.h

src/displayapp/screens/settings/Settings.h

src/components/settings/Settings.h

src/components/ble/FSService.cpp

mark9064 · 2024-07-02T22:50:59Z

Regarding b2e2690
So one option is to decrease the heap size a notch so more RAM is free for static alloc. This is going to happen soon anyway as there are very few bytes free
I'm not 100% sure why the original approach was taken, really @JF002 would have to chime in on this. I suspect that it's to ensure that classes only have access to certain controllers etc? The memory savings look good though

mark9064

LGTM code wise :)

I haven't had the time to properly consider the alternative ideas put forward in #1814 yet so that might end up being a better user interface, but in terms of implementation this looks solid (aside from discussion about the way of accessing controllers)

DavisNT · 2024-10-19T08:40:08Z

@JF002, @NeroBurner, @FintasticMan, @Avamander Could it be possible to review this PR?
CC: @mark9064

P.S. If I need to rebase this, please let me know!

mark9064

Spotted a couple more things on re-review

I think the only questionable thing at the moment is b2e2690 and whether this architecture is a good idea

Other than those bits this is ready to go IMO

mark9064 · 2024-10-19T10:32:03Z

src/components/settings/Settings.cpp

@@ -34,6 +34,9 @@ void Settings::LoadSettingsFromFile() {
  if (bufferSettings.version == settingsVersion) {
    settings = bufferSettings;
  }
+  if (settings.dfuAndFsMode == DfuAndFsMode::EnabledTillReboot) {


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 · 2024-10-19T10:32:06Z

src/components/ble/ImmediateAlertService.cpp

@@ -63,7 +63,8 @@ int ImmediateAlertService::OnAlertLevelChanged(uint16_t attributeHandle, ble_gat
      auto* alertString = ToString(alertLevel);

      NotificationManager::Notification notif;
-      std::memcpy(notif.message.data(), alertString, strlen(alertString));
+      std::memcpy(notif.message.data(), alertString, strlen(alertString) + 1);


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 · 2024-10-19T18:23:29Z

src/components/settings/Settings.h

@@ -308,6 +320,8 @@ namespace Pinetime {
        uint16_t shakeWakeThreshold = 150;

        Controllers::BrightnessController::Levels brightLevel = Controllers::BrightnessController::Levels::Medium;
+
+        DfuAndFsMode dfuAndFsMode = DfuAndFsMode::Disabled;


If the mode EnabledTillReboot is saved into the settings, and restored on reboot then it is effectively always enabled. Please check my suspicion

See https://github.com/InfiniTimeOrg/InfiniTime/pull/1891/files#r1807283475

@NeroBurner The DfuAndFsMode::EnabledTillReboot should never be restored on reboot. Please check the link from @mark9064's comment above.

DavisNT force-pushed the otaset branch from 46f4e19 to 82464ad Compare October 21, 2023 21:29

DavisNT mentioned this pull request Oct 21, 2023

Adjust NimbleController for changes in InfiniTime InfiniTimeOrg/InfiniSim#125

Closed

DavisNT force-pushed the otaset branch from 82464ad to 813488a Compare October 27, 2023 22:08

DavisNT force-pushed the otaset branch from 813488a to ee8b35b Compare October 27, 2023 22:15

DavisNT mentioned this pull request May 22, 2024

No pairing is needed to re-flash watch #1814

Open

1 task

DavisNT force-pushed the otaset branch from ee8b35b to 62874d3 Compare June 30, 2024 17:45

mark9064 reviewed Jul 1, 2024

View reviewed changes

DavisNT added 2 commits July 3, 2024 00:51

Add setting to disable DFU and FS access

eeb0d23

ImmediateAlertService: fix possible bug

565ed00

DavisNT force-pushed the otaset branch from 62874d3 to 565ed00 Compare July 2, 2024 22:12

mark9064 approved these changes Jul 2, 2024

View reviewed changes

mark9064 reviewed Oct 19, 2024

View reviewed changes

NeroBurner reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting to disable DFU and FS access #1891

Setting to disable DFU and FS access #1891

DavisNT commented Oct 20, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023 •

edited

Loading

DavisNT commented Oct 21, 2023

DavisNT commented Oct 22, 2023

DavisNT commented Oct 27, 2023

DavisNT commented Oct 27, 2023

DavisNT commented Jun 24, 2024

DavisNT commented Jun 30, 2024 •

edited

Loading

mark9064 left a comment

mark9064 Jul 1, 2024

DavisNT Jul 2, 2024

mark9064 Jul 2, 2024

DavisNT Jul 3, 2024

mark9064 Jul 3, 2024

mark9064 commented Jul 2, 2024

mark9064 left a comment

DavisNT commented Oct 19, 2024

mark9064 left a comment

mark9064 Oct 19, 2024

DavisNT Oct 22, 2024

mark9064 Oct 23, 2024

DavisNT Oct 24, 2024

mark9064 Oct 26, 2024 •

edited

Loading

mark9064 Oct 19, 2024

DavisNT Oct 19, 2024

NeroBurner Oct 19, 2024

FintasticMan Oct 19, 2024

DavisNT Oct 22, 2024

NeroBurner Oct 19, 2024

mark9064 Oct 19, 2024

DavisNT Oct 22, 2024

Setting to disable DFU and FS access #1891

Are you sure you want to change the base?

Setting to disable DFU and FS access #1891

Conversation

DavisNT commented Oct 20, 2023 • edited Loading

github-actions bot commented Oct 20, 2023 • edited Loading

DavisNT commented Oct 21, 2023

DavisNT commented Oct 22, 2023

DavisNT commented Oct 27, 2023

DavisNT commented Oct 27, 2023

DavisNT commented Jun 24, 2024

DavisNT commented Jun 30, 2024 • edited Loading

mark9064 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark9064 commented Jul 2, 2024

mark9064 left a comment

Choose a reason for hiding this comment

DavisNT commented Oct 19, 2024

mark9064 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark9064 Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisNT commented Oct 20, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023 •

edited

Loading

DavisNT commented Jun 30, 2024 •

edited

Loading

mark9064 Oct 26, 2024 •

edited

Loading