-
-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Settings ui and service #717
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
using Microsoft.AspNetCore.Mvc; | ||
using System.IO; | ||
using System.Text.Json; | ||
|
||
[ApiController] | ||
[Route("api/settings")] | ||
public class SettingsController : ControllerBase | ||
{ | ||
private const string SettingsFilePath = "settings.json"; | ||
|
||
[HttpGet] | ||
public ActionResult<Settings> GetSettings() | ||
{ | ||
if (!System.IO.File.Exists(SettingsFilePath)) | ||
{ | ||
return new Settings(); | ||
} | ||
|
||
var json = System.IO.File.ReadAllText(SettingsFilePath); | ||
return JsonSerializer.Deserialize<Settings>(json); | ||
} | ||
DTTerastar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
[HttpPost] | ||
public IActionResult SaveSettings([FromBody] Settings settings) | ||
{ | ||
var json = JsonSerializer.Serialize(settings); | ||
System.IO.File.WriteAllText(SettingsFilePath, json); | ||
return Ok(); | ||
} | ||
DTTerastar marked this conversation as resolved.
Show resolved
Hide resolved
DTTerastar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public class Settings | ||
{ | ||
public Updating Updating { get; set; } | ||
public Scanning Scanning { get; set; } | ||
public Counting Counting { get; set; } | ||
public Filtering Filtering { get; set; } | ||
public Calibration Calibration { get; set; } | ||
} | ||
Comment on lines
+32
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initialize properties to prevent null reference exceptions. The Consider initializing the properties in the constructor: public class Settings
{
public Settings()
{
Updating = new Updating();
Scanning = new Scanning();
Counting = new Counting();
Filtering = new Filtering();
Calibration = new Calibration();
}
public Updating Updating { get; set; }
public Scanning Scanning { get; set; }
public Counting Counting { get; set; }
public Filtering Filtering { get; set; }
public Calibration Calibration { get; set; }
} |
||
|
||
public class Updating | ||
{ | ||
public bool AutoUpdate { get; set; } | ||
public bool PreRelease { get; set; } | ||
} | ||
|
||
public class Scanning | ||
{ | ||
public int? ForgetAfterMs { get; set; } | ||
} | ||
|
||
public class Counting | ||
{ | ||
public string IdPrefixes { get; set; } | ||
public double? StartCountingDistance { get; set; } | ||
public double? StopCountingDistance { get; set; } | ||
public int? IncludeDevicesAge { get; set; } | ||
} | ||
|
||
public class Filtering | ||
{ | ||
public string IncludeIds { get; set; } | ||
public string ExcludeIds { get; set; } | ||
public double? MaxReportDistance { get; set; } | ||
public double? EarlyReportDistance { get; set; } | ||
public int? SkipReportAge { get; set; } | ||
} | ||
|
||
public class Calibration | ||
{ | ||
public int? RssiAt1m { get; set; } | ||
public int? RssiAdjustment { get; set; } | ||
public double? AbsorptionFactor { get; set; } | ||
public int? IBeaconRssiAt1m { get; set; } | ||
} | ||
DTTerastar marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,22 +19,26 @@ | |||||||||||||||||||||||||||
_state = state; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
[HttpGet("{id}")] | ||||||||||||||||||||||||||||
public DeviceSettingsDetails Get(string id) | ||||||||||||||||||||||||||||
[HttpGet("{id}/details")] | ||||||||||||||||||||||||||||
public async Task<IList<KeyValuePair<string, string>>> Details(string id) | ||||||||||||||||||||||||||||
Check warning on line 23 in src/Controllers/DeviceController.cs GitHub Actions / build
|
||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
var deviceSettings = _deviceSettingsStore.Get(id); | ||||||||||||||||||||||||||||
var details = new List<KeyValuePair<string, string>>(); | ||||||||||||||||||||||||||||
if (deviceSettings?.Id != null && _state.Devices.TryGetValue(deviceSettings.Id, out var device)) | ||||||||||||||||||||||||||||
details.AddRange(device.GetDetails()); | ||||||||||||||||||||||||||||
return new DeviceSettingsDetails(deviceSettings ?? new DeviceSettings { Id = id, OriginalId = id }, details); | ||||||||||||||||||||||||||||
if (_state.Devices.TryGetValue(id, out var device)) | ||||||||||||||||||||||||||||
return device.GetDetails().ToList(); | ||||||||||||||||||||||||||||
return new List<KeyValuePair<string, string>>(); | ||||||||||||||||||||||||||||
DTTerastar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
[HttpPut("{id}")] | ||||||||||||||||||||||||||||
[HttpGet("{id}/settings")] | ||||||||||||||||||||||||||||
public Task<DeviceSettings?> Get(string id) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
var settings = _deviceSettingsStore.Get(id); | ||||||||||||||||||||||||||||
settings ??= new DeviceSettings { OriginalId = id, Id = id }; | ||||||||||||||||||||||||||||
return Task.FromResult(settings); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+30
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Address nullability mismatch and consider optimization The
- public Task<DeviceSettings?> Get(string id)
+ public Task<DeviceSettings> Get(string id)
- var settings = _deviceSettingsStore.Get(id);
- settings ??= new DeviceSettings { OriginalId = id, Id = id };
+ var settings = _deviceSettingsStore.Get(id) ?? new DeviceSettings { OriginalId = id, Id = id }; These changes will resolve the nullability warning and slightly improve the code's conciseness. 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: build
|
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
[HttpPut("{id}/settings")] | ||||||||||||||||||||||||||||
public async Task Set(string id, [FromBody] DeviceSettings value) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
await _deviceSettingsStore.Set(id, value); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public readonly record struct DeviceSettingsDetails(DeviceSettings? settings, IList<KeyValuePair<string, string>> details); | ||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,28 +11,62 @@ public class NodeSettings(string id) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[StringLength(64)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public string? Id { get; set; } = id; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonPropertyName("absorption")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonProperty("absorption")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Range(1, 10)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public double? Absorption { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public UpdatingSettings Updating { get; set; } = new UpdatingSettings(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public ScanningSettings Scanning { get; set; } = new ScanningSettings(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public CountingSettings Counting { get; set; } = new CountingSettings(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public FilteringSettings Filtering { get; set; } = new FilteringSettings(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public CalibrationSettings Calibration { get; set; } = new CalibrationSettings(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonPropertyName("rx_adj_rssi")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonProperty("rx_adj_rssi")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Range(-127, 128)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? RxAdjRssi { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public NodeSettings Clone() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return new NodeSettings(id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Updating = Updating.Clone(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Scanning = Scanning.Clone(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Counting = Counting.Clone(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Filtering = Filtering.Clone(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Calibration = Calibration.Clone() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue with constructor usage in the In the Apply this diff to define a proper constructor and fix the -public class NodeSettings(string id)
+public class NodeSettings
+{
+ public NodeSettings(string id)
+ {
+ Id = id;
+ }
[JsonPropertyName("id")]
[JsonProperty("id")]
[StringLength(64)]
- public string? Id { get; set; } = id;
+ public string? Id { get; set; }
public UpdatingSettings Updating { get; set; } = new UpdatingSettings();
public ScanningSettings Scanning { get; set; } = new ScanningSettings();
public CountingSettings Counting { get; set; } = new CountingSettings();
public FilteringSettings Filtering { get; set; } = new FilteringSettings();
public CalibrationSettings Calibration { get; set; } = new CalibrationSettings();
public NodeSettings Clone()
{
- return new NodeSettings(id)
+ return new NodeSettings(this.Id)
{
Updating = Updating.Clone(),
Scanning = Scanning.Clone(),
Counting = Counting.Clone(),
Filtering = Filtering.Clone(),
Calibration = Calibration.Clone()
};
}
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonPropertyName("tx_ref_rssi")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonProperty("tx_ref_rssi")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Range(-127, 128)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? TxRefRssi { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class UpdatingSettings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public bool? AutoUpdate { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public bool? PreRelease { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public UpdatingSettings Clone() => (UpdatingSettings)MemberwiseClone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Invalid use of The Apply this diff to properly implement the For example, in -public UpdatingSettings Clone() => (UpdatingSettings)MemberwiseClone();
+public UpdatingSettings Clone()
+{
+ return new UpdatingSettings
+ {
+ AutoUpdate = this.AutoUpdate,
+ PreRelease = this.PreRelease
+ };
+} Repeat similar changes for the other settings classes to explicitly clone each property. Also applies to: 43-43, 52-52, 62-62, 71-71 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonPropertyName("max_distance")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[JsonProperty("max_distance")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Range(0, 100)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class ScanningSettings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? ForgetAfterMs { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public ScanningSettings Clone() => (ScanningSettings)MemberwiseClone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class CountingSettings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public string? IdPrefixes { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public double? StartCountingDistance { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public double? StopCountingDistance { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? IncludeDevicesAge { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public CountingSettings Clone() => (CountingSettings)MemberwiseClone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class FilteringSettings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public string? IncludeIds { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public string? ExcludeIds { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public double? MaxDistance { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public double? EarlyReportDistance { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? SkipReportAge { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public FilteringSettings Clone() => (FilteringSettings)MemberwiseClone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public NodeSettings Clone() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return (NodeSettings)MemberwiseClone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class CalibrationSettings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? RssiAt1m { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? RxAdjRssi { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public double? Absorption { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public int? TxRefRssi { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public CalibrationSettings Clone() => (CalibrationSettings)MemberwiseClone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,8 +74,8 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) | |
Log.Information("Optimizer set {0,-20} to Absorption: {1:0.00} RxAdj: {2:00} Error: {3}", id, result.Absorption, result.RxAdjRssi, result.Error); | ||
var a = _nsd.Get(id); | ||
if (optimization == null) continue; | ||
if (result.Absorption != null && result.Absorption > optimization.AbsorptionMin && result.Absorption < optimization.AbsorptionMax) a.Absorption = result.Absorption; | ||
if (result.RxAdjRssi != null && result.RxAdjRssi > optimization.RxAdjRssiMin && result.RxAdjRssi < optimization.RxAdjRssiMax) a.RxAdjRssi = result.RxAdjRssi == null ? 0 : (int?)Math.Round(result.RxAdjRssi.Value); | ||
if (result.Absorption != null && result.Absorption > optimization.AbsorptionMin && result.Absorption < optimization.AbsorptionMax) a.Calibration.Absorption = result.Absorption; | ||
if (result.RxAdjRssi != null && result.RxAdjRssi > optimization.RxAdjRssiMin && result.RxAdjRssi < optimization.RxAdjRssiMax) a.Calibration.RxAdjRssi = result.RxAdjRssi == null ? 0 : (int?)Math.Round(result.RxAdjRssi.Value); | ||
Comment on lines
+77
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Issues Found: Incomplete Refactoring of The verification process revealed several remaining direct accesses to Affected Files:
Please ensure that all direct accesses to 🔗 Analysis chainVerify the impact of the structural change across the codebase The modification from To ensure consistency across the codebase, please run the following script: This script will help identify any remaining direct accesses to The changes themselves look good, maintaining the existing logic while adapting to the new structure. The null checks and range validations are preserved, which is important for data integrity. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining direct accesses to Absorption and RxAdjRssi
# Search for direct accesses to Absorption
echo "Checking for direct accesses to Absorption:"
rg --type csharp '\.Absorption\s*[^.]' -g '!src/Optimizers/OptimizationRunner.cs'
# Search for direct accesses to RxAdjRssi
echo "Checking for direct accesses to RxAdjRssi:"
rg --type csharp '\.RxAdjRssi\s*[^.]' -g '!src/Optimizers/OptimizationRunner.cs'
# Search for the new structure
echo "Verifying usage of new Calibration structure:"
rg --type csharp '\.Calibration\.Absorption' -g '!src/Optimizers/OptimizationRunner.cs'
rg --type csharp '\.Calibration\.RxAdjRssi' -g '!src/Optimizers/OptimizationRunner.cs'
Length of output: 4422 |
||
await _nsd.Set(id, a); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,22 +5,23 @@ | |||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct the constructor syntax for the class. In C#, constructors should be defined within the class body, not in the class declaration. The current syntax uses parameters in the class declaration, which is invalid and will lead to a compilation error. Apply this diff to fix the constructor definition: -public class NodeSettingsStore(MqttCoordinator mqtt, ILogger<NodeSettingsStore> logger) : BackgroundService
+public class NodeSettingsStore : BackgroundService
+{
+ private readonly MqttCoordinator mqtt;
+ private readonly ILogger<NodeSettingsStore> logger;
+
+ public NodeSettingsStore(MqttCoordinator mqtt, ILogger<NodeSettingsStore> logger)
+ {
+ this.mqtt = mqtt;
+ this.logger = logger;
+ }
|
||||||||||||||||||||||||||
public class NodeSettingsStore(MqttCoordinator mqtt, ILogger<NodeSettingsStore> logger) : BackgroundService | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
private readonly ConcurrentDictionary<string, NodeSettings> _storeById = new(); | ||||||||||||||||||||||||||
private readonly ConcurrentDictionary<string, Models.NodeSettings> _storeById = new(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public NodeSettings Get(string id) | ||||||||||||||||||||||||||
public Models.NodeSettings Get(string id) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
return _storeById.TryGetValue(id, out var ns) ? ns.Clone() : new NodeSettings(id); | ||||||||||||||||||||||||||
return _storeById.TryGetValue(id, out var ns) ? ns.Clone() : new Models.NodeSettings(id); | ||||||||||||||||||||||||||
Comment on lines
+10
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure In the Apply this diff to initialize -return _storeById.TryGetValue(id, out var ns) ? ns.Clone() : new Models.NodeSettings(id);
+return _storeById.TryGetValue(id, out var ns) ? ns.Clone() : new Models.NodeSettings(id)
+{
+ Calibration = new CalibrationSettings(),
+ Filtering = new FilteringSettings()
+}; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public async Task Set(string id, NodeSettings ds) | ||||||||||||||||||||||||||
public async Task Set(string id, Models.NodeSettings ns) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
var old = Get(id); | ||||||||||||||||||||||||||
if (ds.Absorption == null || ds.Absorption != old.Absorption) | ||||||||||||||||||||||||||
await mqtt.EnqueueAsync($"espresense/rooms/{id}/absorption/set", $"{ds.Absorption:0.00}"); | ||||||||||||||||||||||||||
if (ds.RxAdjRssi == null || ds.RxAdjRssi != old.RxAdjRssi) | ||||||||||||||||||||||||||
await mqtt.EnqueueAsync($"espresense/rooms/{id}/rx_adj_rssi/set", $"{ds.RxAdjRssi}"); | ||||||||||||||||||||||||||
if (ds.TxRefRssi == null || ds.TxRefRssi != old.TxRefRssi) | ||||||||||||||||||||||||||
await mqtt.EnqueueAsync($"espresense/rooms/{id}/tx_ref_rssi/set", $"{ds.TxRefRssi}"); | ||||||||||||||||||||||||||
var oCs = Get(id).Calibration; | ||||||||||||||||||||||||||
var nCs = ns.Calibration; | ||||||||||||||||||||||||||
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add null checks for Accessing Apply this diff to add null checks: var oCs = Get(id).Calibration;
var nCs = ns.Calibration;
+if (oCs == null || nCs == null)
+{
+ // Handle the null case appropriately, possibly initializing Calibration
+ return;
+}
|
||||||||||||||||||||||||||
if (nCs.Absorption != null && nCs.Absorption != oCs.Absorption) | ||||||||||||||||||||||||||
await mqtt.EnqueueAsync($"espresense/rooms/{id}/absorption/set", $"{nCs.Absorption:0.00}"); | ||||||||||||||||||||||||||
if (nCs.RxAdjRssi != null && nCs.RxAdjRssi != oCs.RxAdjRssi) | ||||||||||||||||||||||||||
await mqtt.EnqueueAsync($"espresense/rooms/{id}/rx_adj_rssi/set", $"{nCs.RxAdjRssi}"); | ||||||||||||||||||||||||||
if (nCs.TxRefRssi != null && nCs.TxRefRssi != oCs.TxRefRssi) | ||||||||||||||||||||||||||
await mqtt.EnqueueAsync($"espresense/rooms/{id}/tx_ref_rssi/set", $"{nCs.TxRefRssi}"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
protected override async Task ExecuteAsync(CancellationToken stoppingToken) | ||||||||||||||||||||||||||
|
@@ -29,20 +30,20 @@ | |||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
try | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
var ns = Get(arg.NodeId); | ||||||||||||||||||||||||||
switch (arg.Setting) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
case "absorption": | ||||||||||||||||||||||||||
ns.Absorption = double.Parse(arg.Payload); | ||||||||||||||||||||||||||
ns.Calibration.Absorption = double.Parse(arg.Payload); | ||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
case "rx_adj_rssi": | ||||||||||||||||||||||||||
ns.RxAdjRssi = int.Parse(arg.Payload); | ||||||||||||||||||||||||||
ns.Calibration.RxAdjRssi = int.Parse(arg.Payload); | ||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
case "tx_ref_rssi": | ||||||||||||||||||||||||||
ns.TxRefRssi = int.Parse(arg.Payload); | ||||||||||||||||||||||||||
ns.Calibration.TxRefRssi = int.Parse(arg.Payload); | ||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
case "max_distance": | ||||||||||||||||||||||||||
ns.MaxDistance = double.Parse(arg.Payload); | ||||||||||||||||||||||||||
ns.Filtering.MaxDistance = double.Parse(arg.Payload); | ||||||||||||||||||||||||||
Comment on lines
+37
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle potential parsing exceptions and invalid data. While exceptions are caught in the Apply this diff to add input validation: case "absorption":
ns.Calibration.Absorption = double.Parse(arg.Payload);
break;
case "rx_adj_rssi":
ns.Calibration.RxAdjRssi = int.Parse(arg.Payload);
break;
case "tx_ref_rssi":
ns.Calibration.TxRefRssi = int.Parse(arg.Payload);
break;
case "max_distance":
ns.Filtering.MaxDistance = double.Parse(arg.Payload);
break;
+default:
+ return Task.CompletedTask; Modify the parsing lines to include validation: +if (double.TryParse(arg.Payload, out var absorption))
+ ns.Calibration.Absorption = absorption;
+else
+ logger.LogWarning("Invalid absorption value received: {0}", arg.Payload); Repeat similar validation for other cases.
|
||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||
return Task.CompletedTask; | ||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider making the settings file path configurable.
The
SettingsFilePath
is currently hardcoded as a constant. To improve flexibility across different environments, consider making this path configurable through application settings or environment variables.You could modify the controller to accept the file path through dependency injection: