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

Minimize DCT usage in bootloader #1325

Merged
merged 46 commits into from May 10, 2017

Conversation

@sergeuz
Copy link
Member

commented May 10, 2017

Problem

  • Bootloader doesn't fit into its flash region on P1 with the newer WICED.
  • Bootloader unnecessarily stores and updates system flags in DCT, potentially increasing chance of a DCT corruption.

Solution

  • Load DCT functions dynamically in bootloader on P1 and Photon.
  • Use backup registers to store system flags.

Steps to Test

  1. Build 0.6.2 and flash it to the P1 or Photon (downgrade bootloader if necessary).
  2. Build this branch with the following changes: bump firmware version to 0.7.0-rc.1 (expected module version is 200; use this commit as a reference), bump bootloader version, run hal/src/stm32f2xx/image_bootloader.sh (optional).
  3. Flash the newer part1, part2 and bootloader to the device OTA. Ensure that all modules have expected version numbers via particle serial inspect. Note: feature/photon/wiced-3.7.0-7, which this branch is based on, doesn't update bootloader automatically.
  4. Check that the bootloader's DCT functionality works as expected: device/server keys can be downloaded/uploaded, LED mirroring and themes work, etc.
  5. Downgrade part2 back to 0.6.2. Any attempts to write to the DCT while the device is in DFU mode should fail now. Read operations should return null data (TODO: would be nice to make them fail explicitly as well).
    Note: At this point, attempt to downgrade part1 will fail silently, since the bootloader can't access DCT and read module update info, thus, in order to downgrade from 0.7.0-rc.1, firmware modules should be flashed in the following order: part2 -> bootloader -> part1. Make sure release notes are updated accordingly.
  6. Downgrade bootloader and part1 to 0.6.2. Use particle serial inspect to check that all modules have correct version numbers.
  7. Run unit tests.

(feel free to extend the list!)

Example App

N/A

References

This PR supersedes #1320 and #1307.


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

Enhancement

  • [PR #1325] Use backup registers instead of DCT to store system flags to avoid chance of a DCT corruption.
sergeuz and others added 30 commits Apr 14, 2017
Changed DCT access to lock()/unlock() or read-with-copy style
Added DCT and flash locking support for Photon/P1
Fix bootloader build. It still overflows, but since we are going to d…
…ynalib import dct functions from system-part2, let's at least leave the source tree in a working state
Photon/P1 system-part2 static RAM needs to be increased once again du…
…e to static allocations of IDLE and Timer threads in new FreeRTOS

@sergeuz sergeuz added this to the 0.7.0 milestone May 10, 2017

@sergeuz sergeuz requested a review from avtolstoy May 10, 2017

namespace {

StaticRecursiveMutex dctLock;
bool dctLockDisabled = false;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 10, 2017

Member

Should this be volatile just in case?


#define SYSTEM_FLAGS_MAGIC_NUMBER 0x1ADEACC0u

int Load_SystemFlags_Impl(platform_system_flags_t* f) {

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 10, 2017

Member

Are system flags only read/written by system thread? We might want to add a lock here or just disable interrupts while reading/writing.

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 10, 2017

Author Member

Fixed in Load_SystemFlags() and Save_SystemFlags() which call these _Impl functions.

if (!mutex_) {
init();
}
return (xSemaphoreTakeRecursive(mutex_, portMAX_DELAY) == pdTRUE);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 10, 2017

Member

I still think we should add a timeout here at least in debug builds to make it easier to spot incorrect usage of DCT. That way the timeout will trigger an assert in dct_lock instead of a deadlock.

@avtolstoy

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Note: At this point, attempt to downgrade part1 will fail silently, since the bootloader can't access DCT and read module update info, thus, in order to downgrade from 0.7.0-rc.1, firmware modules should be flashed in the following order: part2 -> bootloader -> part1. Make sure release notes are updated accordingly.

We could also add a dependency to the new bootloader on system-part1. That way after downgrading system-part2 and attempting to downgrade system-part1 the dependency check will fail (even if we are already running pre-0.7.0 system-part2) and the device will not even attempt to replace the module. After the bootloader is downgraded (to pre-0.7.0 which doesn't have any dependencies), downgrade of system-part1 (and system-part3 (module index) on Electron) should succeed.

EDIT: Actually this won't work, because validate_module_dependencies_full doesn't check that bootloader dependency is satisfied. See https://github.com/spark/firmware/blob/develop/hal/src/stm32f2xx/ota_flash_hal_stm32f2xx.cpp#L113

sergeuz added 2 commits May 10, 2017

@technobly technobly merged commit aa22f1a into feature/photon/wiced-3.7.0-7 May 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/bootloader_dct branch May 10, 2017

avtolstoy added a commit to avtolstoy/firmware that referenced this pull request May 10, 2017
avtolstoy pushed a commit to avtolstoy/firmware that referenced this pull request May 10, 2017
@sergeuz sergeuz referenced this pull request Jun 7, 2017
4 of 6 tasks complete

@technobly technobly removed the bug label Jun 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.