Setting to disable DFU and FS access #1891
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. Although it doesn't make too much of a difference, maybe apply this to There was a problem hiding this comment. @mark9064 I would prefer to leave this as it is now. Currently it will fail-secure (set @NeroBurner This is the reason why There was a problem hiding this comment. 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. This is exactly the thing I would like to keep as is (unless requested by the maintainers to change it). The change you suggest would not change how the code behaves. Maybe I should add a code comment above There was a problem hiding this 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. |
||
| settings.dfuAndFsMode = DfuAndFsMode::Disabled; | ||
| } | ||
| } | ||
|
|
||
| void Settings::SaveSettingsToFile() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ namespace Pinetime { | |
| }; | ||
| enum class PTSGaugeStyle : uint8_t { Full, Half, Numeric }; | ||
| enum class PTSWeather : uint8_t { On, Off }; | ||
| enum class DfuAndFsMode : uint8_t { Disabled, Enabled, EnabledTillReboot }; | ||
|
|
||
| struct PineTimeStyle { | ||
| Colors ColorTime = Colors::Teal; | ||
|
|
@@ -283,10 +284,21 @@ namespace Pinetime { | |
| return bleRadioEnabled; | ||
| }; | ||
|
|
||
| void SetDfuAndFsMode(DfuAndFsMode mode) { | ||
| if (mode != settings.dfuAndFsMode) { | ||
| settingsChanged = true; | ||
| } | ||
| settings.dfuAndFsMode = mode; | ||
| }; | ||
|
|
||
| DfuAndFsMode GetDfuAndFsMode() const { | ||
| return settings.dfuAndFsMode; | ||
| }; | ||
|
|
||
| private: | ||
| Pinetime::Controllers::FS& fs; | ||
|
|
||
| static constexpr uint32_t settingsVersion = 0x0008; | ||
|
|
||
| struct SettingsData { | ||
| uint32_t version = settingsVersion; | ||
|
|
@@ -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. 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. There was a problem hiding this comment. @NeroBurner The |
||
| }; | ||
|
|
||
| SettingsData settings; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ namespace Pinetime { | |
| SettingChimes, | ||
| SettingShakeThreshold, | ||
| SettingBluetooth, | ||
| SettingOTA, | ||
| Error | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| #include "displayapp/screens/settings/SettingOTA.h" | ||
| #include <lvgl/lvgl.h> | ||
| #include "displayapp/DisplayApp.h" | ||
| #include "displayapp/Messages.h" | ||
| #include "displayapp/screens/Styles.h" | ||
| #include "displayapp/screens/Screen.h" | ||
| #include "displayapp/screens/Symbols.h" | ||
|
|
||
| using namespace Pinetime::Applications::Screens; | ||
|
|
||
| namespace { | ||
| struct Option { | ||
| const char* name; | ||
| Pinetime::Controllers::Settings::DfuAndFsMode mode; | ||
| }; | ||
|
|
||
| constexpr std::array<Option, 3> options = {{ | ||
| {"Enabled", Pinetime::Controllers::Settings::DfuAndFsMode::Enabled}, | ||
| {"Disabled", Pinetime::Controllers::Settings::DfuAndFsMode::Disabled}, | ||
| {"Till reboot", Pinetime::Controllers::Settings::DfuAndFsMode::EnabledTillReboot}, | ||
| }}; | ||
|
|
||
| std::array<CheckboxList::Item, CheckboxList::MaxItems> CreateOptionArray() { | ||
| std::array<Pinetime::Applications::Screens::CheckboxList::Item, CheckboxList::MaxItems> optionArray; | ||
| for (size_t i = 0; i < CheckboxList::MaxItems; i++) { | ||
| if (i >= options.size()) { | ||
| optionArray[i].name = ""; | ||
| optionArray[i].enabled = false; | ||
| } else { | ||
| optionArray[i].name = options[i].name; | ||
| optionArray[i].enabled = true; | ||
| } | ||
| } | ||
| return optionArray; | ||
| }; | ||
| } | ||
|
|
||
| SettingOTA::SettingOTA(Pinetime::Applications::DisplayApp* app, Pinetime::Controllers::Settings& settingsController) | ||
| : app {app}, | ||
| settingsController {settingsController}, | ||
| checkboxList( | ||
| 0, | ||
| 1, | ||
| "Firmware & files", | ||
| Symbols::shieldAlt, | ||
| settingsController.GetDfuAndFsMode() == Pinetime::Controllers::Settings::DfuAndFsMode::Enabled ? 0 | ||
| : settingsController.GetDfuAndFsMode() == Pinetime::Controllers::Settings::DfuAndFsMode::EnabledTillReboot ? 2 | ||
| : 1, | ||
| [&settings = settingsController](uint32_t index) { | ||
| settings.SetDfuAndFsMode(options[index].mode); | ||
| }, | ||
| CreateOptionArray()) { | ||
| } | ||
|
|
||
| SettingOTA::~SettingOTA() { | ||
| lv_obj_clean(lv_scr_act()); | ||
| settingsController.SaveSettings(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #pragma once | ||
|
|
||
| #include <array> | ||
| #include <cstdint> | ||
| #include <lvgl/lvgl.h> | ||
|
|
||
| #include "components/settings/Settings.h" | ||
| #include "displayapp/screens/Screen.h" | ||
| #include "displayapp/screens/CheckboxList.h" | ||
|
|
||
| namespace Pinetime { | ||
|
|
||
| namespace Applications { | ||
| namespace Screens { | ||
|
|
||
| class SettingOTA : public Screen { | ||
| public: | ||
| SettingOTA(DisplayApp* app, Pinetime::Controllers::Settings& settingsController); | ||
| ~SettingOTA() override; | ||
|
|
||
| private: | ||
| DisplayApp* app; | ||
| Pinetime::Controllers::Settings& settingsController; | ||
| CheckboxList checkboxList; | ||
| }; | ||
| } | ||
| } | ||
| } |
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.
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.
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 bestrlen()of the string plus 1 for the trailing zero-byte string terminator (unless the destination memory is guaranteed to be prefilled with zero-bytes).