# Gap: Configuration Validation & Migration

## Problem
The configuration system loads 7 XML files with no schema validation, no bounds checking, and silent error swallowing. Malformed files produce partial/default state with no user indication. There is no migration path from the old 32-CFG-file format.

## Critical Issues

### 1. SavePump() Stub
**File:** `Services/Impl/ConfigurationService.cs:191-194`
**Impact:** Pump definition edits (test phase enable/disable, parameter changes, new pumps) are lost on restart.
**Fix:** Implement full XML serialization mirroring the `LoadPump`/`ParsePumpElement` logic. Use `PumpDefinition.ToXml()`, `TestDefinition.ToXml()`, `PhaseDefinition.ToXml()`. Write to `pumps.xml` at the correct XPath location. Handle both insert (new pump) and update (existing pump) cases.

### 2. LastRotationDirection Never Loaded
**File:** `Services/Impl/ConfigurationService.cs` — `SaveSettings()` writes `<LastRotationDir>` but `LoadSettings()` never reads it.
**Fix:** Add `TryInt(r, "LastRotationDir", v => settings.LastRotationDirection = (short)v);` in `LoadSettings()`.

### 3. No SaveAlarms()
**File:** `Services/Impl/ConfigurationService.cs`
**Impact:** Alarm configuration changes cannot be persisted.
**Fix:** Add `SaveAlarms()` using `Alarm.ToXml()` for each alarm, write to `alarms.xml`.

### 4. SaveSettings() Side Effects
**File:** `Services/Impl/ConfigurationService.cs:101-102`
**Impact:** Calling `SaveSettings()` always also writes sensors.xml and clients.xml.
**Fix:** Split into `SaveSettings()`, `SaveSensors()`, `SaveClients()` called independently. Or document the coupling clearly.

## Bounds Validation Rules

Add these checks in the respective `FromXml`/`Load*` methods:

| Field | Location | Valid Range | Guard |
|-------|----------|-------------|-------|
| `byteh` / `bytel` | `CanBusParameter.FromXml`, `ParseParamElement` | 0–7 | Clamp or reject + log |
| `RefreshCanBusReadMs` | `LoadSettings` | >= 1 | Floor at 1 |
| `RefreshPumpParamsMs` | `LoadSettings` | >= 1 | Floor at 1 |
| `SecurityRpmLimit` | `LoadSettings` | 100–5000 | Clamp |
| `MaxPressureBar` | `LoadSettings` | 1–100 | Clamp |
| `PidP, PidI, PidD` | `LoadSettings` | >= 0 | Floor at 0 |
| `Relay.Bit` | `ParseRelayElement` | 0–63 | Reject + log |
| `SensorConfiguration.MaxVolt - MinVolt` | `GetValueFromRaw` | != 0 | Return 0 + log |
| P1–P6 denominator | `GetTransformResult` | != 0 | Return 0 + log |

## Locale-Safe Parsing
All `double.Parse` / `int.Parse` calls in XML loading must use `CultureInfo.InvariantCulture`. Known violations:
- `SensorConfiguration.FromXml` — `double.Parse(v)` with no culture (line ~78-83)
- `CanBusParameter.ParseDecimal` — already uses `InvariantCulture` (correct)
- `ConfigurationService.TryDouble` — uses `double.TryParse` with `InvariantCulture` (correct)

## Config Versioning Approach
Add a `version` attribute to each XML root element:
```xml
<Config version="2">...</Config>
<Pumps version="1">...</Pumps>
```
On load, check version. If missing or old, run migration logic. Bump version after migration. This enables safe format evolution.

## Error Reporting Strategy
Replace all `catch { }` and `catch (Exception) { }` blocks with:
```csharp
catch (Exception ex)
{
    _log.Warning(LogId, $"Skipped malformed element '{elementName}': {ex.Message}");
}
```
Files affected:
- `ConfigurationService.cs` — `TryInt`, `TryDouble`, `TryBool`, `TryString` helpers (lines 621-646)
- `SensorConfiguration.cs` — `TryParse` wrapper (line 100-103)

## Dead Fields
`AppSettings.Clients` and `AppSettings.PumpIds` are never populated by `LoadSettings()` — they are dead fields. Either remove them or wire up the loading. Current live data paths: `ConfigurationService._clients` and `GetPumpIds()`.

## Old CFG → XML Migration Strategy
See `gap-pump-data-migration.md` for the full field mapping. In summary:
1. Parse each CFG file using the pipe-delimited format documented in `old_source/HerlicScripts/Program.cs`
2. Map CAN IDs, P1-P6 formulas, status bit definitions, and EEPROM passwords to the pumps.xml and status.xml schemas
3. Run once as a migration tool, not as runtime logic