v2 dcd format and tests #1295
v2 dcd format and tests #1295
Conversation
…omputed on data that is subsequently modified after writing the header.
m-mcgowan
force-pushed
the
feature/dcd_crc
branch
from
34fdcaf
to
ecaa8dd
Compare
| @@ -1,37 +1,37 @@ | |||
| const unsigned char bootloader_platform_10_bin[] = { | |||
There was a problem hiding this comment.
If a new bootloader binary is bundled here, it should probably have a bumped module version, otherwise it is not automatically upgraded, which causes a device to enter an endless loop, as well as DCT corruption.
There was a problem hiding this comment.
Totes, I probably tested against an older version so got the upgrade anyway. Although I did also see the endless loop when developing this initially. That's a bit worrying - anything we can to do help folks out of that if things do go wrong?
| */ | ||
| Sector _currentSector(uint8_t count0, uint8_t count1, Sector sector0, Sector sector1) | ||
| { | ||
| if (((count0+1) & 3)==count1) { |
There was a problem hiding this comment.
I can't think of a reason why the distance between counter/sequence numbers might be > 1, but having run into multiple issues with dct and flash operations in general already it will most likely happen than not. Having that in mind these checks look a bit unsafe.
I propose that 0 is treated as an initial value (only assigned when dcd is initialized for the first time) and a valid range is clamped to 1 - 3, with 3 wrapping to 1 instead of 0.
In that case the check could be implemented like this:
static const int dcd_counter_bits = 2;
static const int dcd_max_counter = (1 << dcd_counter_bits) - 1;
const int distance = (int)count0 - (int)count1;
if ((counter0 != 0) && (counter1 == 0 || (distance < 0 && distance > -dcd_max_counter)) {
// prefer sector1
return sector1;
}
// In all other cases, prefer sector0
return sector0;The results table:
| count0 | count1 | current | proposed |
|---|---|---|---|
| 0 | 0 | sector0 | sector0 |
| 0 | 1 | sector1 | sector0 |
| 0 | 2 | sector0 | sector0 |
| 0 | 3 | sector0 | sector0 |
| 1 | 0 | sector0 | sector1 |
| 1 | 1 | sector0 | sector0 |
| 1 | 2 | sector1 | sector1 |
| 1 | 3 | sector0 | sector1 |
| 2 | 0 | sector0 | sector1 |
| 2 | 1 | sector0 | sector0 |
| 2 | 2 | sector0 | sector0 |
| 2 | 3 | sector1 | sector1 |
| 3 | 0 | sector1 | sector1 |
| 3 | 1 | sector0 | sector0 |
| 3 | 2 | sector0 | sector0 |
| 3 | 3 | sector0 | sector0 |
Although I might be overthinking it.
| Sector newSector = alternateSectorTo(current); | ||
| const uint8_t* existing = store.dataAt(addressOf(current)); | ||
| Result error = this->writeSector(offset, data, length, existing, newSector); | ||
| Result error = this->_writeSector(offset, data, length, existing, newSector); |
There was a problem hiding this comment.
I suggest we don't mix tabs and spaces within the same file. This PR is full of such cases.
| } | ||
| error = _write_v2_footer(newSector, existing ? &existingFooter : nullptr, counter); | ||
| if (error) return error; | ||
| typename Footer::crc_type crc = computeSectorCRC(newSector); |
There was a problem hiding this comment.
Why not CRC the header as well?
There was a problem hiding this comment.
The values in the header is checked fully so no need to CRC it. Also the header is invalidated when a new sector written, but we may want to also still validate the data, so the header is excluded from the CRC.
[Original #1295] v2 dcd format and tests
[Original particle-iot#1295] v2 dcd format and tests
|
Closing due to changes being in |
Do not merge as-is, will require a rebase
Problem
Hypothesised write errors in flash that appear only after power cycle can corrupt the configuration data.
Flash access is not thread-safe.
Solution
Adds CRC checking to the Electron DCD implementation so that write errors are detected.
Add a critical section around flash operations and around DCD operations.
Steps to Test
Run the firmware unit tests.
Manual testing:
Example App
This is an internal system change. No example application.
Completeness