-
-
Notifications
You must be signed in to change notification settings - Fork 952
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
base: main
Are you sure you want to change the base?
Conversation
Build checks have not completed. Possible reasons for this are:
|
Both issues discovered by the workflows should now be fixed.
|
The build-simulator step is expected to fail, because it needs InfiniTimeOrg/InfiniSim#125 |
Made two improvements:
Could it be possible to review this PR? |
Rebased as well. |
Is this PR (and the other PR described in #1814 (comment) ) welcome? |
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 |
There was a problem hiding this 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
@@ -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)) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
Regarding b2e2690 |
There was a problem hiding this 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)
@JF002, @NeroBurner, @FintasticMan, @Avamander Could it be possible to review this PR? P.S. If I need to rebase this, please let me know! |
There was a problem hiding this 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
src/components/settings/Settings.cpp
Outdated
@@ -34,6 +34,9 @@ void Settings::LoadSettingsFromFile() { | |||
if (bufferSettings.version == settingsVersion) { | |||
settings = bufferSettings; | |||
} | |||
if (settings.dfuAndFsMode == DfuAndFsMode::EnabledTillReboot) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated fix.
There was a problem hiding this comment.
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 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
src/components/settings/Settings.h
Outdated
@@ -308,6 +320,8 @@ namespace Pinetime { | |||
uint16_t shakeWakeThreshold = 150; | |||
|
|||
Controllers::BrightnessController::Levels brightLevel = Controllers::BrightnessController::Levels::Medium; | |||
|
|||
DfuAndFsMode dfuAndFsMode = DfuAndFsMode::Disabled; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
@mark9064 Building the simulator fails on @mark9064, @FintasticMan, @NeroBurner, @JF002, @Avamander the updated PR:
|
@FintasticMan, @NeroBurner could it be possible to review this PR (and merge it, if it is OK)? |
We are currently in feature freeze for 1.15. I've scheduled your PR to 1.16 |
This PR adds setting to disable firmware update and Bluetooth file transfer.
This is 2. part for #1814 (comment)