Add production battery charge limit Device control - #513
Conversation
onehoon
left a comment
There was a problem hiding this comment.
Two blocking issues remain before merge.
- Battery load/modify/save is not atomic under the shared
ProfileMutationGate, so a normal concurrent Device/Profile edit can be lost.
SetEnabled() / SetPercent() load profiles.json before taking _mutationGate.Sync, then Commit() later locks only around Save(updated). The bootstrap path similarly loads, releases the gate, reads hardware, and then saves a document derived from the earlier load. This defeats the existing gate's purpose: another CPU/TDP/Power/Profile mutation can save between those two points, after which Battery writes the stale document and silently overwrites that newer change.
Please keep each persisted load-modify-save transaction on one fresh document under the existing shared gate. Do not add another lock/manager. Also avoid holding the gate across WMI I/O; persist under the gate, then apply hardware afterward.
For example:
private BatteryChargeLimitMutationResult SetPercentCore(int percent)
{
DeviceBatteryChargeLimitSettings desired;
lock (_mutationGate.Sync)
{
var loaded = _profileStore.Load();
if (!loaded.CanSafelyReplace)
return Result(BatteryChargeLimitMutationOutcome.PersistenceFailed,
"Profile state is not safe to replace.", loaded);
var saved = loaded.Document.Device.Battery?.ChargeLimit;
var enabled = IsValidTarget(saved) ? saved!.Enabled : ReadEnabledForInitialization();
if (enabled is null)
return Result(BatteryChargeLimitMutationOutcome.Unavailable, "BatteryLimit read failed.", loaded);
desired = new DeviceBatteryChargeLimitSettings
{
Enabled = enabled.Value,
LimitPercent = percent
};
var updated = WithBatteryChargeLimit(loaded.Document, desired);
try
{
_profileStore.Save(updated);
}
catch (Exception ex)
{
return Result(BatteryChargeLimitMutationOutcome.PersistenceFailed, ex.Message, loaded);
}
}
// WMI/readback stays outside ProfileMutationGate.
return Apply(desired);
}For first-run bootstrap, after the hardware read succeeds, reacquire _mutationGate.Sync, reload the current profile, verify ChargeLimit is still null, and add Battery to that fresh document before saving. Please add a regression test where another Device/Profile field is changed between the battery's initial observation and battery persistence, and assert that field survives.
- The slider commit currently discards the user's draft before sending the RPC, so the requested percentage is not actually applied.
CommitBatteryChargeLimitPercentAsync() sets _batteryChargeLimitMutationBusy = true and immediately calls:
RenderBatteryChargeLimit(_batteryChargeLimitSnapshot, preserveDirtyDraft: false);That render resets _batteryChargeLimitDraftPercent from the authoritative snapshot. Example: desired=80, user drags to 85, commit begins, render resets the draft back to 80, and the following RPC sends 80. The control also visibly snaps back before the request.
Capture the committed value first and do not re-render authoritative state until the mutation result arrives. A small local enable/disable helper is enough; no new state machine is needed. For example:
private async Task CommitBatteryChargeLimitPercentAsync()
{
if (_suppressBatteryChargeLimitEvents || !_batteryChargeLimitDraftDirty ||
_frontend is null || _batteryChargeLimitMutationBusy)
return;
var percent = _batteryChargeLimitDraftPercent;
if (_batteryChargeLimitSnapshot.DesiredLimitPercent == percent)
{
_batteryChargeLimitDraftDirty = false;
return;
}
_batteryChargeLimitDraftDirty = false;
_batteryChargeLimitMutationBusy = true;
UpdateBatteryChargeLimitControlEnabledState(); // must not rewrite Value/draft
try
{
var result = await _frontend.SetDeviceBatteryChargeLimitPercentAsync(percent);
RenderBatteryChargeLimit(result.Snapshot, preserveDirtyDraft: false);
// existing failure presentation...
}
finally
{
_batteryChargeLimitMutationBusy = false;
UpdateBatteryChargeLimitControlEnabledState();
}
}Please add a focused regression around the draft/commit policy (80 authoritative -> user draft 85 -> exactly one 85 commit after end-of-interaction). The current source-presence UI test does not exercise this behavior.
Everything else I checked is directionally aligned with the work order: existing block-215 hardware adapter/shared helper reuse, production-vs-developer RPC separation, protocol v30, Device-only surface, additive Device.Battery storage, enabled/disabled reconcile ordering, delayed resume integration, and no QAM/Overlay expansion in this PR.
|
Addressed both blocking review comments in commit
Validation:
|
|
Follow-up: the new-head GitHub Actions �uild-and-test run completed successfully for 3bdf836 (5m47s). PR merge state is CLEAN. |
Summary
Validation
Manual validation