Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting to disable DFU and FS access #1891

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DavisNT
Copy link

@DavisNT DavisNT commented Oct 20, 2023

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

@github-actions
Copy link

github-actions bot commented Oct 20, 2023

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

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

@DavisNT
Copy link
Author

DavisNT commented Oct 21, 2023

Both issues discovered by the workflows should now be fixed.

@DavisNT
Copy link
Author

DavisNT commented Oct 22, 2023

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

@DavisNT
Copy link
Author

DavisNT commented Oct 27, 2023

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
Copy link
Author

DavisNT commented Oct 27, 2023

Rebased as well.

@DavisNT
Copy link
Author

DavisNT commented Jun 24, 2024

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.

@DavisNT
Copy link
Author

DavisNT commented Jun 30, 2024

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).

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/components/ble/DfuService.cpp Outdated Show resolved Hide resolved
@@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/components/ble/DfuService.h Outdated Show resolved Hide resolved
src/components/ble/FSService.h Outdated Show resolved Hide resolved
src/displayapp/screens/settings/Settings.h Show resolved Hide resolved
src/components/settings/Settings.h Outdated Show resolved Hide resolved
src/components/ble/FSService.cpp Outdated Show resolved Hide resolved
@mark9064
Copy link
Member

mark9064 commented Jul 2, 2024

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

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

DavisNT commented Oct 19, 2024

@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!

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@mark9064 mark9064 Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NeroBurner Created the PR #2159

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

Controllers::BrightnessController::Levels brightLevel = Controllers::BrightnessController::Levels::Medium;

DfuAndFsMode dfuAndFsMode = DfuAndFsMode::Disabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Author

DavisNT commented Nov 9, 2024

@mark9064 Building the simulator fails on main branch with exactly the same error messages.

@mark9064, @FintasticMan, @NeroBurner, @JF002, @Avamander the updated PR:

  • is rebased
  • the ImmediateAlertService fix has been moved to a separate PR ImmediateAlertService: fix latent bug #2159
  • changing of setting value on loading has been replaced with two separate bool variables - one volatile and one persisted in the settings object (code from Settings::LoadSettingsFromFile() has been removed)

@DavisNT
Copy link
Author

DavisNT commented Nov 15, 2024

@FintasticMan, @NeroBurner could it be possible to review this PR (and merge it, if it is OK)?

@NeroBurner NeroBurner added the enhancement Enhancement to an existing app/feature label Nov 16, 2024
@NeroBurner NeroBurner added this to the 1.16.0 milestone Nov 16, 2024
@NeroBurner
Copy link
Contributor

We are currently in feature freeze for 1.15. I've scheduled your PR to 1.16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants