Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions Controllers/SettingsController.cs
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";

Comment on lines +5 to +10
Copy link

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:

public class SettingsController : ControllerBase
{
    private readonly string _settingsFilePath;

    public SettingsController(IConfiguration configuration)
    {
        _settingsFilePath = configuration["SettingsFilePath"] ?? "settings.json";
    }

    // ... rest of the controller
}

[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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Initialize properties to prevent null reference exceptions.

The Settings class properties are of reference types but are not initialized. This could lead to null reference exceptions if these properties are accessed before being set.

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
24 changes: 14 additions & 10 deletions src/Controllers/DeviceController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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

View workflow job for this annotation

GitHub Actions / build

This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
{
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);

Check warning on line 35 in src/Controllers/DeviceController.cs

View workflow job for this annotation

GitHub Actions / build

Nullability of reference types in value of type 'Task<DeviceSettings>' doesn't match target type 'Task<DeviceSettings?>'.
}
Comment on lines +30 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address nullability mismatch and consider optimization

The Get method has been updated to return a Task<DeviceSettings?>, but there's a nullability mismatch with the actual returned value.

  1. To address the nullability warning, update the return type:
-        public Task<DeviceSettings?> Get(string id)
+        public Task<DeviceSettings> Get(string id)
  1. Consider optimizing the method by using C# 8.0's null-coalescing assignment operator:
-            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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[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);
}
[HttpGet("{id}/settings")]
public Task<DeviceSettings> Get(string id)
{
var settings = _deviceSettingsStore.Get(id) ?? new DeviceSettings { OriginalId = id, Id = id };
return Task.FromResult(settings);
}
🧰 Tools
🪛 GitHub Check: build

[warning] 35-35:
Nullability of reference types in value of type 'Task' doesn't match target type 'Task<DeviceSettings?>'.


[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);
}
10 changes: 5 additions & 5 deletions src/Controllers/NodeController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@ namespace ESPresense.Controllers;
[ApiController]
public class NodeController(NodeSettingsStore nodeSettingsStore, State state) : ControllerBase
{
[HttpGet("{id}")]
[HttpGet("{id}/settings")]
public NodeSettingsDetails Get(string id)
{
var nodeSettings = nodeSettingsStore.Get(id);
var details = new List<KeyValuePair<string, string>>();
if (nodeSettings?.Id != null && state.Nodes.TryGetValue(id, out var node))
details.AddRange(node.GetDetails());
return new NodeSettingsDetails(nodeSettings ?? new NodeSettings(id), details);
return new NodeSettingsDetails(nodeSettings ?? new Models.NodeSettings(id), details);
}

[HttpPut("{id}")]
public Task Set(string id, [FromBody] NodeSettings ds)
[HttpPut("{id}/settings")]
public Task Set(string id, [FromBody] Models.NodeSettings ds)
{
return nodeSettingsStore.Set(id, ds);
}
Expand All @@ -36,5 +36,5 @@ public async Task Restart(string id)
await nodeSettingsStore.Restart(id);
}

public readonly record struct NodeSettingsDetails(NodeSettings? settings, IList<KeyValuePair<string, string>> details);
public readonly record struct NodeSettingsDetails(Models.NodeSettings? settings, IList<KeyValuePair<string, string>> details);
}
6 changes: 3 additions & 3 deletions src/Controllers/StateController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@
{
var rxNs = _nsd.Get(rxId);
var rxM = txM.GetOrAdd(rx.Rx?.Name ?? rxId);
if (txNs.TxRefRssi is not null) rxM["tx_ref_rssi"] = txNs.TxRefRssi.Value;
if (rxNs.RxAdjRssi is not null) rxM["rx_adj_rssi"] = rxNs.RxAdjRssi.Value;
if (rxNs.Absorption is not null) rxM["absorption"] = rxNs.Absorption.Value;
if (txNs.Calibration.TxRefRssi is not null) rxM["tx_ref_rssi"] = txNs.Calibration.TxRefRssi.Value;
if (rxNs.Calibration.RxAdjRssi is not null) rxM["rx_adj_rssi"] = rxNs.Calibration.RxAdjRssi.Value;
if (rxNs.Calibration.Absorption is not null) rxM["absorption"] = rxNs.Calibration.Absorption.Value;
rxM["expected"] = rx.Expected;
rxM["actual"] = rx.Distance;
rxM["rssi"] = rx.Rssi;
Expand Down Expand Up @@ -145,9 +145,9 @@
foreach (var node in _state.Nodes.Values)
{
var nodeSettings = _nsd.Get(node.Id);
nodeSettings.TxRefRssi = null;

Check failure on line 148 in src/Controllers/StateController.cs

View workflow job for this annotation

GitHub Actions / build

'NodeSettings' does not contain a definition for 'TxRefRssi' and no accessible extension method 'TxRefRssi' accepting a first argument of type 'NodeSettings' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 148 in src/Controllers/StateController.cs

View workflow job for this annotation

GitHub Actions / build

'NodeSettings' does not contain a definition for 'TxRefRssi' and no accessible extension method 'TxRefRssi' accepting a first argument of type 'NodeSettings' could be found (are you missing a using directive or an assembly reference?)
nodeSettings.RxAdjRssi = null;

Check failure on line 149 in src/Controllers/StateController.cs

View workflow job for this annotation

GitHub Actions / build

'NodeSettings' does not contain a definition for 'RxAdjRssi' and no accessible extension method 'RxAdjRssi' accepting a first argument of type 'NodeSettings' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 149 in src/Controllers/StateController.cs

View workflow job for this annotation

GitHub Actions / build

'NodeSettings' does not contain a definition for 'RxAdjRssi' and no accessible extension method 'RxAdjRssi' accepting a first argument of type 'NodeSettings' could be found (are you missing a using directive or an assembly reference?)
nodeSettings.Absorption = null;

Check failure on line 150 in src/Controllers/StateController.cs

View workflow job for this annotation

GitHub Actions / build

'NodeSettings' does not contain a definition for 'Absorption' and no accessible extension method 'Absorption' accepting a first argument of type 'NodeSettings' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 150 in src/Controllers/StateController.cs

View workflow job for this annotation

GitHub Actions / build

'NodeSettings' does not contain a definition for 'Absorption' and no accessible extension method 'Absorption' accepting a first argument of type 'NodeSettings' could be found (are you missing a using directive or an assembly reference?)
await _nsd.Set(node.Id, nodeSettings);
}

Expand Down
72 changes: 53 additions & 19 deletions src/Models/NodeSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Issue with constructor usage in the Clone() method of NodeSettings

In the Clone() method, you're using new NodeSettings(id), but id is not defined within the scope of this method. Additionally, the class declaration public class NodeSettings(string id) is invalid in C#. Class declarations cannot have parameters in this way.

Apply this diff to define a proper constructor and fix the Clone() method:

-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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public NodeSettings Clone()
{
return new NodeSettings(id)
{
Updating = Updating.Clone(),
Scanning = Scanning.Clone(),
Counting = Counting.Clone(),
Filtering = Filtering.Clone(),
Calibration = Calibration.Clone()
};
}
public class NodeSettings
{
public NodeSettings(string id)
{
Id = id;
}
[JsonPropertyName("id")]
[JsonProperty("id")]
[StringLength(64)]
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(this.Id)
{
Updating = Updating.Clone(),
Scanning = Scanning.Clone(),
Counting = Counting.Clone(),
Filtering = Filtering.Clone(),
Calibration = Calibration.Clone()
};
}
}

}

[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();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Invalid use of MemberwiseClone() in Clone() methods

The MemberwiseClone() method is protected and cannot be called directly in this manner. Using it as shown will result in a compilation error. Consider implementing the ICloneable interface or manually cloning each property.

Apply this diff to properly implement the Clone() methods:

For example, in UpdatingSettings:

-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();
}
6 changes: 3 additions & 3 deletions src/Models/OptimizationResults.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ public double Evaluate(List<OptimizationSnapshot> oss, NodeSettingsStore nss)
var rx = nss.Get(m.Rx.Id);

RxNodes.TryGetValue(m.Rx.Id, out var pv);
double rxAdjRssi = pv?.RxAdjRssi ?? rx.RxAdjRssi ?? 0;
double txPower = tx.TxRefRssi ?? -59;
double pathLossExponent = pv?.Absorption ?? rx.Absorption ?? 3;
double rxAdjRssi = pv?.RxAdjRssi ?? rx.Calibration.RxAdjRssi ?? 0;
double txPower = tx.Calibration.TxRefRssi ?? -59;
double pathLossExponent = pv?.Absorption ?? rx.Calibration.Absorption ?? 3;
double distance = m.Rx.Location.DistanceTo(m.Tx.Location);
double predictedRssi = txPower + rxAdjRssi - 10 * pathLossExponent * Math.Log10(distance);

Expand Down
4 changes: 2 additions & 2 deletions src/Optimizers/OptimizationRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Issues Found: Incomplete Refactoring of Absorption and RxAdjRssi Properties

The verification process revealed several remaining direct accesses to Absorption and RxAdjRssi across multiple files in the codebase. This indicates that the structural changes to use Calibration.Absorption and Calibration.RxAdjRssi have not been consistently applied.

Affected Files:

  • src/Optimizers/AbsorptionLineFitOptimizer.cs
  • src/Optimizers/RxAdjRssiOptimizer.cs
  • src/Optimizers/AbsorptionAvgOptimizer.cs
  • src/Models/OptimizationResults.cs
  • src/Optimizers/AbsorptionErrOptimizer.cs
  • src/Services/NodeSettingsStore.cs
  • src/Controllers/StateController.cs

Please ensure that all direct accesses to Absorption and RxAdjRssi are updated to use the Calibration structure to maintain consistency and prevent potential issues.

🔗 Analysis chain

Verify the impact of the structural change across the codebase

The modification from a.Absorption to a.Calibration.Absorption and a.RxAdjRssi to a.Calibration.RxAdjRssi indicates a structural change in the data model. While this change is consistently applied here, it's crucial to ensure that all other parts of the codebase that interact with these properties have been updated accordingly.

To ensure consistency across the codebase, please run the following script:

This script will help identify any remaining direct accesses to Absorption and RxAdjRssi that might need to be updated, as well as confirm the usage of the new Calibration structure across the codebase.

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 executed

The 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);
}

Expand Down
31 changes: 16 additions & 15 deletions src/Services/NodeSettingsStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,23 @@
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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;
+    }

Committable suggestion was skipped due to low confidence.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure Calibration is not null to prevent NullReferenceException.

In the Get method, when creating a new Models.NodeSettings, verify that the Calibration property is initialized. If Calibration is null, accessing it later will cause a NullReferenceException.

Apply this diff to initialize Calibration:

-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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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);
public Models.NodeSettings Get(string id)
{
return _storeById.TryGetValue(id, out var ns) ? ns.Clone() : new Models.NodeSettings(id)
{
Calibration = new CalibrationSettings(),
Filtering = new FilteringSettings()
};
}

}

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null checks for Calibration properties to prevent exceptions.

Accessing Calibration properties without ensuring they are not null can lead to NullReferenceException. Before using oCs and nCs, verify that Calibration is not null in both the old and new settings.

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;
+}

Committable suggestion was skipped due to low confidence.

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)
Expand All @@ -29,20 +30,20 @@
{
try
{
var ns = Get(arg.NodeId);

Check warning on line 33 in src/Services/NodeSettingsStore.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference argument for parameter 'id' in 'NodeSettings NodeSettingsStore.Get(string id)'.
switch (arg.Setting)
{
case "absorption":
ns.Absorption = double.Parse(arg.Payload);
ns.Calibration.Absorption = double.Parse(arg.Payload);

Check warning on line 37 in src/Services/NodeSettingsStore.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference argument for parameter 's' in 'double double.Parse(string s)'.
break;
case "rx_adj_rssi":
ns.RxAdjRssi = int.Parse(arg.Payload);
ns.Calibration.RxAdjRssi = int.Parse(arg.Payload);

Check warning on line 40 in src/Services/NodeSettingsStore.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference argument for parameter 's' in 'int int.Parse(string s)'.
break;
case "tx_ref_rssi":
ns.TxRefRssi = int.Parse(arg.Payload);
ns.Calibration.TxRefRssi = int.Parse(arg.Payload);

Check warning on line 43 in src/Services/NodeSettingsStore.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference argument for parameter 's' in 'int int.Parse(string s)'.
break;
case "max_distance":
ns.MaxDistance = double.Parse(arg.Payload);
ns.Filtering.MaxDistance = double.Parse(arg.Payload);

Check warning on line 46 in src/Services/NodeSettingsStore.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference argument for parameter 's' in 'double double.Parse(string s)'.
Comment on lines +37 to +46
Copy link

Choose a reason for hiding this comment

The 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 try-catch block, relying on exception handling for control flow is not optimal. Validate arg.Payload before parsing to ensure it contains valid numeric data. This can prevent unnecessary exceptions and improve performance.

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.

Committable suggestion was skipped due to low confidence.

break;
default:
return Task.CompletedTask;
Expand Down
Loading
Loading