simplified config file comments, fixed settings not peristed, fixed F407 flash code

framework/settings.c

@@ -2,6 +2,7 @@
 // Created by MightyPork on 2017/11/26.
 //
 
+#include "utils/hexdump.h"
 #include "platform.h"
 #include "settings.h"
 #include "unit_registry.h"
@@ -16,6 +17,11 @@ void settings_load(void)
 {
     uint8_t *buffer = (uint8_t *) SETTINGS_FLASH_ADDR;
 
+#if DEBUG_FLASH_WRITE
+    dbg("reading @ %p", (void*)SETTINGS_FLASH_ADDR);
+    hexDump("Flash", buffer, 64);
+#endif
+
     PayloadParser pp = pp_start(buffer, SETTINGS_BLOCK_SIZE, NULL);
 
     // Check the integrity marker
@@ -46,7 +52,7 @@ static uint8_t save_buffer[SAVE_BUF_SIZE];
 static uint32_t save_addr;
 
 #if DEBUG_FLASH_WRITE
-#define fls_printf(fmt, ...) dbg(fmt, ##__VA_ARGS__)
+#define fls_printf(fmt, ...) PRINTF(fmt, ##__VA_ARGS__)
 #else
 #define fls_printf(fmt, ...) do {} while (0)
 #endif
@@ -180,6 +186,11 @@ void settings_save(void)
     hst = HAL_FLASH_Lock();
     assert_param(hst == HAL_OK);
     fls_printf("--- Flash done ---\r\n");
+
+#if DEBUG_FLASH_WRITE
+    dbg("written @ %p", (void*)SETTINGS_FLASH_ADDR);
+    hexDump("Flash", (void*)SETTINGS_FLASH_ADDR, 64);
+#endif
 }
 
 /**
@@ -189,8 +200,8 @@ void settings_write_ini(IniWriter *iw)
 {
     // File header
     iw_comment(iw, "CONFIG.INI");
-    iw_comment(iw, "Changes are applied on file save and can be immediately tested and verified.");
-    iw_comment(iw, "To persist to flash, replace the LOCK jumper before disconnecting from USB."); // TODO the jumper...
+    iw_comment(iw, "Overwrite this file to change settings.");
+    iw_comment(iw, "Close the LOCK jumper to save them to Flash.");
 
     systemsettings_write_ini(iw);
     iw_newline(iw);
@@ -204,7 +215,7 @@ void settings_read_ini_begin(void)
     SystemSettings.modified = true;
 
     // load defaults
-    systemsettings_init();
+    systemsettings_loadDefaults();
     ureg_remove_all_units();
 }
 

framework/system_settings.c

@@ -9,11 +9,19 @@
 
 struct system_settings SystemSettings;
 
-void systemsettings_init(void)
+/** Load defaults only */
+void systemsettings_loadDefaults(void)
 {
     SystemSettings.visible_vcom = true;
-    SystemSettings.modified = false;
+}
 
+/** Load defaults and init flags */
+void systemsettings_init(void)
+{
+    systemsettings_loadDefaults();
+
+    // Flags
+    SystemSettings.modified = false;
     LockJumper_ReadHardware();
 }
 
@@ -40,7 +48,7 @@ bool systemsettings_load(PayloadParser *pp)
 void systemsettings_write_ini(IniWriter *iw)
 {
     iw_section(iw, "SYSTEM");
-    iw_comment(iw, "Expose the comm. channel as a virtual comport (Y, N)");
+    iw_comment(iw, "Data link accessible as virtual comport (Y, N)");
     iw_entry(iw, "expose_vcom", str_yn(SystemSettings.visible_vcom));
 }
 

framework/system_settings.h

@@ -21,10 +21,15 @@ struct system_settings {
 extern struct system_settings SystemSettings;
 
 /**
- * Load defaults
+ * Init the store
  */
 void systemsettings_init(void);
 
+/**
+ * Load defaults
+ */
+void systemsettings_loadDefaults(void);
+
 /**
  * Write system settings to the pack context
  */

framework/unit_registry.c

@@ -131,8 +131,6 @@ Unit *ureg_instantiate(const char *driver_name)
 {
     bool suc = true;
 
-    dbg("Creating unit of type %s", driver_name);
-
     // Find type in the repository
     UregEntry *re = ureg_head;
     while (re != NULL) {
@@ -157,7 +155,7 @@ Unit *ureg_instantiate(const char *driver_name)
                 // in which case the data structure is not populated and keeping the
                 // broken unit doesn't serve any purpose. Just ditch it...
 
-                dbg("!! Unit failed to pre-init!");
+                dbg("!! Unit type %s failed to pre-init!", driver_name);
                 clean_failed_unit(pUnit);
                 free(le);
                 return NULL;
@@ -176,7 +174,7 @@ Unit *ureg_instantiate(const char *driver_name)
 // remove before init()
 void ureg_clean_failed(Unit *unit)
 {
-    dbg("Cleaning failed unit from registry");
+    dbg("Removing failed unit from registry...");
 
     UlistEntry *le;
     UlistEntry *parent;
@@ -193,7 +191,7 @@ void ureg_clean_failed(Unit *unit)
 // remove after successful init()
 void ureg_remove_unit(Unit *unit)
 {
-    dbg("Cleaning & removing unit from registry");
+    dbg("Cleaning & removing unit from registry...");
 
     UlistEntry *le;
     UlistEntry *parent;
@@ -239,6 +237,7 @@ bool ureg_load_units(PayloadParser *pp)
 
     if (pp_char(pp) != 'U') return false;
     uint16_t unit_count = pp_u16(pp);
+    dbg("Units to load: %d", (int)unit_count);
 
     for (uint32_t j = 0; j < unit_count; j++) {
         // We're now unpacking a single unit
@@ -256,7 +255,7 @@ bool ureg_load_units(PayloadParser *pp)
         pUnit->name = strdup(typebuf);
         assert_param(pUnit->name);
 
-        // CALLSIGN
+        // callsign
         pUnit->callsign = pp_u8(pp);
         assert_param(pUnit->callsign != 0);
 
@@ -264,16 +263,12 @@ bool ureg_load_units(PayloadParser *pp)
         pUnit->driver->cfgLoadBinary(pUnit, pp);
         assert_param(pp->ok);
 
+        dbg("Adding unit \"%s\" of type %s", pUnit->name, pUnit->driver->name);
+
         suc = pUnit->driver->init(pUnit); // finalize the load and init the unit
         if (pUnit->status == E_LOADING) {
             pUnit->status = suc ? E_SUCCESS : E_BAD_CONFIG;
         }
-
-        // XXX we want to keep the failed unit to preserve settings and for error reporting
-//        if (!suc) {
-//            // Discard, remove from registry
-//            ureg_clean_failed(unit);
-//        }
     }
 
     return pp->ok;
@@ -284,6 +279,9 @@ void ureg_remove_all_units(void)
 {
     UlistEntry *le = ulist_head;
     UlistEntry *next;
+
+    dbg("Removing all units");
+
     while (le != NULL) {
         next = le->next;
 
@@ -349,7 +347,7 @@ bool ureg_read_unit_ini(const char *restrict name,
         if (streq(li->unit.name, name)) {
             Unit *const pUnit = &li->unit;
 
-            if (streq(key, "CALLSIGN")) {
+            if (streq(key, "callsign")) {
                 // handled separately from unit data
                 pUnit->callsign = (uint8_t) avr_atoi(value);
                 return true;
@@ -403,10 +401,13 @@ static void export_unit_do(UlistEntry *li, IniWriter *iw)
     Unit *const pUnit = &li->unit;
 
     iw_section(iw, "%s:%s", pUnit->driver->name, pUnit->name);
-    iw_comment(iw, ">> Status: %s", error_get_string(pUnit->status));
+    if (pUnit->status != E_SUCCESS) {
+        iw_comment(iw, "!!! %s", error_get_string(pUnit->status));
+    }
     iw_newline(iw);
-    iw_comment(iw, "Address for control messages (1-255)");
-    iw_entry(iw, "CALLSIGN", "%d", pUnit->callsign);
+
+    iw_comment(iw, "Unit address 1-255");
+    iw_entry(iw, "callsign", "%d", pUnit->callsign);
 
     pUnit->driver->cfgWriteIni(pUnit, iw);
     iw_newline(iw);
@@ -436,10 +437,8 @@ void ureg_export_combined(IniWriter *iw)
 
     // Unit list
     iw_section(iw, "UNITS");
-    iw_comment(iw, "Here is a list of all unit types supported by the current firmware.");
-    iw_comment(iw, "To manage units, simply add/remove their comma-separated names next to");
-    iw_comment(iw, "the desired unit type. Reload the file and the corresponding unit");
-    iw_comment(iw, "sections should appear below, ready to configure.");
+    iw_comment(iw, "Create units by adding their names next to a type (e.g. PIN=A,B),");
+    iw_comment(iw, "remove the same way. Reload to update the unit sections below.");
 
     // This could certainly be done in some more efficient way ...
     re = ureg_head;

gex.mk

@@ -72,6 +72,7 @@ GEX_CDEFS = \
     -DUSE_FULL_ASSERT=1 \
     -DVERBOSE_ASSERT=1 \
     -DDEBUG_VFS=0 \
+    -DDEBUG_FLASH_WRITE=0 \
     -DVERBOSE_HARDFAULT=1 \
     -DUSE_STACK_MONITOR=1 \
     -DUSE_DEBUG_UART=1

platform/lock_jumper.c

@@ -52,10 +52,13 @@ static void jumper_changed(void)
         dbg("LOCK jumper removed, enabling MSC!");
     } else {
         // Lock
-        dbg("LOCK jumper replaced, saving to Flash & disabling MSC!");
+        dbg("LOCK jumper replaced, disabling MSC!");
 
         if (SystemSettings.modified) {
+            dbg("Saving settings to Flash...");
             settings_save();
+        } else {
+            dbg("No changes to save.");
         }
     }
 

platform/plat_compat.h

@@ -180,7 +180,7 @@
     // size, determines position of the flash storage
 
     // we use the first 128kB sector. Unfortunately the whole sector must be erased before writing.
-    #define SETTINGS_FLASH_SECTOR 6
+    #define SETTINGS_FLASH_SECTOR FLASH_SECTOR_5
     #define SETTINGS_BLOCK_SIZE (1024*2)
     #define SETTINGS_FLASH_ADDR (0x08000000 + (16*4+64)*1024)
 

tasks/task_main.c

@@ -48,7 +48,7 @@ void TaskMain(void const * argument)
             // Periodically check stacks for overrun
             stackmon_check_canaries();
             // Periodically dump all stacks - for checking levels before critical (to reduce size if not needed)
-            if ((cnt%50)==0) stackmon_dump();
+//            if ((cnt%50)==0) stackmon_dump();
             continue;
         }