Redesign PG loading from EEPROM #2401

ledvinap · 2016-09-14T17:08:01Z

All parameter groups initialized, either from EEPROM or reset to
default.
Stored record version is checked, mismatched version is ignored

All parameter groups initialized, either from EEPROM or reset to default. Stored record version is checked, mismatched version is ignored

ledvinap · 2016-09-14T17:11:01Z

This should help with #2395.
Review/comments are welcome, but the code is not yet ready for merge. I'll force-push new version, don't base new work on this PR

ledvinap · 2016-09-14T17:15:03Z

@hydra: Is it OK to config_eeprom.c functions to start with EEPROM?

ledvinap · 2016-09-14T17:15:39Z

Unittests will be fixed soon

hydra · 2016-09-14T20:01:15Z

@ledvinap nice to see some progress on this, cool.

there are a few cases that need to be handled:

load existing pg
initialise pg's that cannot be found
load pg with matching version that has NEW fields at the end of the record - the new fields must be reset to defaults - size mismatch.
re-initialise pg's the have a version mismatch.

ledvinap · 2016-09-14T20:38:30Z

load existing pg

Stored configuration is used when it does exist (correct profile/system +PGN) and version does match

initialise pg's that cannot be found

PG is reset when EEPROM record is not found

load pg with matching version that has NEW fields at the end of the record - the new fields must be reset to defaults - size mismatch.

PG is reset first, than stored data are copied over it.

re-initialise pg's the have a version mismatch.

Record is reset on version mismatch (nothing is copied over default values)

Handling of arrays may be improved - pad/trim on per-item basis, then reset remaining items if necessary. But that can go into next PR.

Also it will be quite easy to implement PG upgrade mechanism - register function that will chane record to higher version number. Multiple upgrade steps may be performed automatically, oldest supported CF version may be handled by #define, including only upgrade functions that fit available space

hydra · 2016-09-14T20:50:47Z

@ledvinap great. so all the cases outlined above are all handled by this PR as-is?

martinbudden · 2016-09-14T21:00:38Z

src/main/config/config_eeprom.c

-// true if the config is valid.
-bool scanEEPROM(bool andLoad) // FIXME boolean argument.
+// Scan the EEPROM config. Returns true if the config is valid.
+bool scanEEPROM(void)


Good to see the boolean parameter gone.

martinbudden · 2016-09-14T21:01:26Z

src/main/config/config_eeprom.c

+{
+    const uint8_t *p = &__config_start;
+    p += sizeof(configHeader_t);             // skip header
+    while(true) {


while (true) space after while. Similarly for a few ifs in the code.

Thanks, I'll fix it

BTW: find . -type f -exec grep -nH -E '\<(if|switch|for|do|while)[(]' {} +

ledvinap · 2016-09-14T21:08:03Z

@hydra : I hope so .. It needs testing, I'll prepare some unittests ...

remove unnecessary code

hydra · 2016-09-15T08:44:05Z

@hydra: Is it OK to config_eeprom.c functions to start with EEPROM?

if you mean, is it ok to change the prefix of the eeprom functions to have eeprom at the start. yes. but not in this PR please. as @martinbudden metioned recently let's keep larger renames a little more focused.

hydra

😄

hydra · 2016-09-15T18:30:56Z

@ledvinap ready?

hydra · 2016-09-15T18:55:03Z

@ledvinap in doing a bit of testing and monitoring the reg->pgn from inside pgLoad I noticed unexpected values, such as 4123 and 4107. I had basically flashed the 1.13.0 binary, let it boot and then upgraded the firmware to master via SWD to see what what was being reset and what wasn't when i noticed these unexpected values.

hydra · 2016-09-15T18:56:13Z

ahh wait, never mind, lol, that's the version in the top 4 bits.

hydra · 2016-09-15T19:00:54Z

I do note, that after the config is loaded and some settings are reset the config is not immediately saved and the 'upgrade' process is followed on every boot until the config is saved once.

I think that's probably acceptable but I'd like your thoughts.

ledvinap · 2016-09-15T19:45:24Z

IMO repeated upgrade is fine. If you downgrade/change version before saving, you get original config back. Can be especially useful if you flash wrong version.
If config upgrade using intermediate versions is desired, you can save explicitly.

ledvinap · 2016-09-15T19:48:36Z

BTW: It should be quite easy to skip save when config did not change (use code identical to load and compare EEPROM records to PG).

hydra · 2016-09-15T19:57:42Z

yeah, not-saving is useful feature actually 😄

nice.

ledvinap · 2016-09-15T20:17:46Z

This PR is not that large, I don't see any problem. But I did not test it on actual hardware.
If you tested it on HW (any board should do), is should be safe to merge.,

hydra · 2016-09-15T20:57:16Z

Yes I tested it on an SP Racing F3 EVO and went from 1.13.0 to the latest master + this PR.

I'll merge it now, we can fix in next RC if something comes up.

hydra · 2016-09-21T10:26:32Z

@ledvinap I'm finding if I upgrade from 1.13.0 to master then cli dump fails due to incorrect value of telemetry_send_cells. This needs investigation.

ledvinap · 2016-09-22T09:21:52Z

fixed in #2421. The bug was there before this PR, I totally missed it ... At least new code is a bit nicer ...

Preparation for conversion to parameter groups 5

Redesign PG loaging from EEPROM

6a1943d

All parameter groups initialized, either from EEPROM or reset to default. Stored record version is checked, mismatched version is ignored

hydra changed the title ~~Redesign PG loaging from EEPROM~~ Redesign PG loading from EEPROM Sep 14, 2016

hydra added this to the 1.14.0 milestone Sep 14, 2016

hydra added the Critical label Sep 14, 2016

martinbudden reviewed Sep 14, 2016

View reviewed changes

Update parameter_group.c

7a667ba

remove unnecessary code

ledvinap added 2 commits September 15, 2016 12:55

Fix formating

787a581

Fix findEEPROM bug

5893b2c

hydra reviewed Sep 15, 2016

View reviewed changes

hydra merged commit 3d36914 into cleanflight:master Sep 15, 2016

hydra mentioned this pull request Sep 15, 2016

Ensure new/changed PG are reset when firmware is upgraded. #2395

Closed

ledvinap deleted the improvement-pg-reset branch September 22, 2016 09:21

martinbudden added a commit to martinbudden/cleanflight that referenced this pull request Feb 16, 2017

Merge pull request cleanflight#2401 from martinbudden/bf_pg_preparation5

23302aa

Preparation for conversion to parameter groups 5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign PG loading from EEPROM #2401

Redesign PG loading from EEPROM #2401

ledvinap commented Sep 14, 2016

ledvinap commented Sep 14, 2016

ledvinap commented Sep 14, 2016

ledvinap commented Sep 14, 2016

hydra commented Sep 14, 2016 •

edited

Loading

ledvinap commented Sep 14, 2016

hydra commented Sep 14, 2016

martinbudden Sep 14, 2016

hydra Sep 14, 2016

martinbudden Sep 14, 2016

ledvinap Sep 14, 2016

ledvinap Sep 15, 2016 •

edited

Loading

ledvinap commented Sep 14, 2016

hydra commented Sep 15, 2016 •

edited

Loading

hydra left a comment

hydra commented Sep 15, 2016

hydra commented Sep 15, 2016

hydra commented Sep 15, 2016

hydra commented Sep 15, 2016

ledvinap commented Sep 15, 2016

ledvinap commented Sep 15, 2016

hydra commented Sep 15, 2016

ledvinap commented Sep 15, 2016

hydra commented Sep 15, 2016

hydra commented Sep 21, 2016

ledvinap commented Sep 22, 2016

Redesign PG loading from EEPROM #2401

Redesign PG loading from EEPROM #2401

Conversation

ledvinap commented Sep 14, 2016

ledvinap commented Sep 14, 2016

ledvinap commented Sep 14, 2016

ledvinap commented Sep 14, 2016

hydra commented Sep 14, 2016 • edited Loading

ledvinap commented Sep 14, 2016

hydra commented Sep 14, 2016

martinbudden Sep 14, 2016

Choose a reason for hiding this comment

hydra Sep 14, 2016

Choose a reason for hiding this comment

martinbudden Sep 14, 2016

Choose a reason for hiding this comment

ledvinap Sep 14, 2016

Choose a reason for hiding this comment

ledvinap Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

ledvinap commented Sep 14, 2016

hydra commented Sep 15, 2016 • edited Loading

hydra left a comment

Choose a reason for hiding this comment

hydra commented Sep 15, 2016

hydra commented Sep 15, 2016

hydra commented Sep 15, 2016

hydra commented Sep 15, 2016

ledvinap commented Sep 15, 2016

ledvinap commented Sep 15, 2016

hydra commented Sep 15, 2016

ledvinap commented Sep 15, 2016

hydra commented Sep 15, 2016

hydra commented Sep 21, 2016

ledvinap commented Sep 22, 2016

hydra commented Sep 14, 2016 •

edited

Loading

ledvinap Sep 15, 2016 •

edited

Loading

hydra commented Sep 15, 2016 •

edited

Loading