Conversation
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
📝 WalkthroughWalkthroughThis PR adds a new Rx/Tx RSSI optimizer, refactors CombinedOptimizer with enhanced logging and error handling, replaces hardcoded -59 RSSI constants with a configurable Changes
Sequence DiagramsequenceDiagram
participant Caller
participant CombinedOptimizer
participant RxTxAdjRssiOptimizer
participant BfgsBMinimizer
Caller->>CombinedOptimizer: Optimize(snapshot)
alt enough nodes
CombinedOptimizer->>RxTxAdjRssiOptimizer: Optimize(snapshot)
activate RxTxAdjRssiOptimizer
Note over RxTxAdjRssiOptimizer: collect measurements<br/>build param vector & bounds
RxTxAdjRssiOptimizer->>BfgsBMinimizer: Solve(objective, gradient, bounds)
activate BfgsBMinimizer
BfgsBMinimizer-->>RxTxAdjRssiOptimizer: optimized vector + error
deactivate BfgsBMinimizer
RxTxAdjRssiOptimizer->>CombinedOptimizer: OptimizationResults
deactivate RxTxAdjRssiOptimizer
CombinedOptimizer->>CombinedOptimizer: CreateDeviceParamsFromVector()
CombinedOptimizer->>CombinedOptimizer: CalculateError(deviceParams)
CombinedOptimizer-->>Caller: final results
else insufficient nodes
CombinedOptimizer-->>Caller: early-exit default results (logs)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Optimizers/CombinedOptimizer.cs (1)
137-268: Critical:OptimizeNodeAbsorptionsmethod has severe syntax errors preventing compilation.This method has multiple overlapping code fragments that break the build. The pipeline failures confirm errors at lines 174, 180, 203, 204, 213, and others.
Key issues:
- Lines 140-173: Incomplete objective function lambda with mixed code
- Lines 175-212: Malformed gradient function with duplicate variable declarations (
gradientdeclared at 183 and 188)- Lines 216 and 229: Duplicate
solverdeclarations- Missing/mismatched braces throughout
- Method signature says it returns
Dictionary<string, double>but the code attempts to return a tuple at line 268This method needs to be completely rewritten. Here's a structural outline:
private (Dictionary<string, DeviceParameters> DeviceParams, double Error) OptimizeNodeAbsorptions( List<Measure> allNodes, List<string> uniqueDeviceIds, Dictionary<string, double> rxAdjRssiDict, Dictionary<(string, string), double> pathAbsorptionDict, ConfigOptimization optimization, Dictionary<string, NodeSettings> existingSettings) { // Build initial guess vector var initialGuess = Vector<double>.Build.Dense(uniqueDeviceIds.Count * 2); for (int i = 0; i < uniqueDeviceIds.Count; i++) { existingSettings.TryGetValue(uniqueDeviceIds[i], out var nodeSettings); initialGuess[i] = Math.Clamp(nodeSettings?.Calibration?.RxAdjRssi ?? 0, optimization.RxAdjRssiMin, optimization.RxAdjRssiMax); initialGuess[i + uniqueDeviceIds.Count] = Math.Clamp( nodeSettings?.Calibration?.Absorption ?? 3.0, optimization.AbsorptionMin, optimization.AbsorptionMax); } // Define objective with gradient var objGradient = ObjectiveFunction.Gradient( x => { var deviceParams = CreateDeviceParamsFromVector(x, uniqueDeviceIds, optimization); return CalculateError(allNodes, deviceParams); }, x => { var gradient = Vector<double>.Build.Dense(x.Count); double h = 1e-5; var deviceParams = CreateDeviceParamsFromVector(x, uniqueDeviceIds, optimization); double baseError = CalculateError(allNodes, deviceParams); for (int i = 0; i < x.Count; i++) { var xPlus = x.Clone(); xPlus[i] += h; var paramsPlus = CreateDeviceParamsFromVector(xPlus, uniqueDeviceIds, optimization); gradient[i] = (CalculateError(allNodes, paramsPlus) - baseError) / h; } return gradient; } ); try { var solver = new ConjugateGradientMinimizer(1e-3, 1000); var result = solver.FindMinimum(objGradient, initialGuess); var deviceParams = CreateDeviceParamsFromVector(result.MinimizingPoint, uniqueDeviceIds, optimization); return (deviceParams, result.FunctionInfoAtMinimum.Value); } catch (Exception ex) { _logger.Error(ex, "Optimization failed"); // Return defaults... } }
🧹 Nitpick comments (2)
src/Optimizers/RxTxAdjRssiOptimizer.cs (2)
224-227: Hardcoded initial guess may fall outside configured bounds.The initial
txRefRssiis hardcoded to-59, but the bounds are configurable viaTxRefRssiMin/TxRefRssiMax. If the bounds don't include-59, this could cause issues with the optimizer.Apply a consistent approach using bounds midpoint like you did for absorption:
foreach (var txId in uniqueTxIds) { - initialGuess[txIndexMap[txId]] = -59; + initialGuess[txIndexMap[txId]] = (optimization.TxRefRssiMin + optimization.TxRefRssiMax) / 2.0; }
263-266: Use named placeholder for structured logging.Serilog prefers named placeholders for better structured logging support.
catch (Exception ex) { - Log.Error("Error optimizing all nodes: {0}", ex.Message); + Log.Error(ex, "Error optimizing all nodes"); }Passing the exception as the first argument allows Serilog to capture the full stack trace.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Models/Config.cs(1 hunks)src/Optimizers/CombinedOptimizer.cs(7 hunks)src/Optimizers/GlobalAbsorptionRxTxOptimizer.cs(1 hunks)src/Optimizers/RxTxAdjRssiOptimizer.cs(1 hunks)src/Optimizers/TwoStageRxAdjAbsorptionOptimizer.cs(2 hunks)src/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Place backend C# ASP.NET Core code under src/ (controllers, services, models, utils)
Files:
src/Optimizers/GlobalAbsorptionRxTxOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cssrc/Optimizers/RxTxAdjRssiOptimizer.cssrc/Models/Config.cssrc/Optimizers/CombinedOptimizer.cssrc/Optimizers/TwoStageRxAdjAbsorptionOptimizer.cs
{src,tests}/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.cs: C#: Use spaces with an indent size of 4
C#: Use PascalCase for types and methods
C#: Use camelCase for local variables and parameters
Files:
src/Optimizers/GlobalAbsorptionRxTxOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cssrc/Optimizers/RxTxAdjRssiOptimizer.cssrc/Models/Config.cssrc/Optimizers/CombinedOptimizer.cssrc/Optimizers/TwoStageRxAdjAbsorptionOptimizer.cs
🧠 Learnings (1)
📚 Learning: 2025-09-16T00:51:01.127Z
Learnt from: CR
Repo: ESPresense/ESPresense-companion PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T00:51:01.127Z
Learning: Applies to src/**/Controllers/StateController.cs : StateController must populate Device.ConfiguredRefRssi from DeviceSettings during API calls
Applied to files:
src/Models/Config.cs
🧬 Code graph analysis (2)
src/Optimizers/RxTxAdjRssiOptimizer.cs (2)
src/Models/Config.cs (1)
Config(8-58)src/Models/OptimizationSnapshot.cs (1)
Measure(40-57)
src/Optimizers/CombinedOptimizer.cs (1)
src/Models/OptimizationSnapshot.cs (1)
Measure(40-57)
🪛 GitHub Actions: Build and test
src/Optimizers/CombinedOptimizer.cs
[error] 174-174: CS1513: } expected
🪛 GitHub Actions: Deploy to Docker
src/Optimizers/CombinedOptimizer.cs
[error] 174-174: /App/src/Optimizers/CombinedOptimizer.cs(174,14): error CS1513: } expected
[error] 180-180: /App/src/Optimizers/CombinedOptimizer.cs(180,17): error CS1524: Expected catch or finally
[error] 203-203: /App/src/Optimizers/CombinedOptimizer.cs(203,18): error CS1002: ; expected
[error] 204-204: /App/src/Optimizers/CombinedOptimizer.cs(204,34): error CS1026: ) expected
[error] 204-204: /App/src/Optimizers/CombinedOptimizer.cs(204,36): error CS1002: ; expected
[error] 213-213: /App/src/Optimizers/CombinedOptimizer.cs(213,9): error CS1519: Invalid token ')' in class, record, struct, or interface member declaration
[error] 220-220: /App/src/Optimizers/CombinedOptimizer.cs(220,9): error CS1519: Invalid token 'for' in class, record, struct, or interface member declaration
[error] 220-220: /App/src/Optimizers/CombinedOptimizer.cs(220,20): error CS8124: Tuple must contain at least two elements
[error] 220-220: /App/src/Optimizers/CombinedOptimizer.cs(220,34): error CS1026: ) expected
[error] 226-226: /App/src/Optimizers/CombinedOptimizer.cs(226,29): error CS1519: Invalid token '=' in class, record, struct, or interface member declaration
[error] 226-226: /App/src/Optimizers/CombinedOptimizer.cs(226,41): error CS1519: Invalid token '(' in class, record, struct, or interface member declaration
[error] 226-226: /App/src/Optimizers/CombinedOptimizer.cs(226,55): error CS8124: Tuple must contain at least two elements
[error] 226-226: /App/src/Optimizers/CombinedOptimizer.cs(226,98): error CS1026: ) expected
[error] 239-239: /App/src/Optimizers/CombinedOptimizer.cs(239,26): error CS1002: ; expected
[error] 239-239: /App/src/Optimizers/CombinedOptimizer.cs(239,28): error CS1001: Identifier expected
[error] 241-241: /App/src/Optimizers/CombinedOptimizer.cs(241,31): error CS1001: Identifier expected
[error] 255-255: /App/src/Optimizers/CombinedOptimizer.cs(255,9): error CS1022: Type or namespace definition, or end-of-file expected
[error] 269-269: /App/src/Optimizers/CombinedOptimizer.cs(269,5): error CS1022: Type or namespace definition, or end-of-file expected
[error] 271-271: /App/src/Optimizers/CombinedOptimizer.cs(271,5): error CS0106: The modifier 'private' is not valid for this item
[error] 299-299: /App/src/Optimizers/CombinedOptimizer.cs(299,5): error CS0106: The modifier 'private' is not valid for this item
[error] 348-348: /App/src/Optimizers/CombinedOptimizer.cs(348,1): error CS1022: Type or namespace definition, or end-of-file expected
🪛 GitHub Check: build
src/Optimizers/CombinedOptimizer.cs
[failure] 213-213:
Invalid token ')' in a member declaration
[failure] 204-204:
} expected
[failure] 204-204:
; expected
[failure] 204-204:
; expected
[failure] 204-204:
) expected
[failure] 203-203:
} expected
[failure] 203-203:
; expected
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
src/Optimizers/GlobalAbsorptionRxTxOptimizer.cs (1)
341-341: LGTM!This is a cosmetic change adding a newline at the end of the file, which is a good practice for version control.
src/Models/Config.cs (1)
137-138: Verify the widened TxRefRssi bounds are intentional.The default range has been expanded from
[-70, -50](20 dBm) to[-80, -40](40 dBm). This doubles the optimization search space forTxRefRssi.Ensure this change is intentional and won't cause optimization instability or convergence issues with the wider bounds.
src/Optimizers/RxTxAdjRssiOptimizer.cs (2)
12-21: LGTM!Clean class structure with proper constructor injection and property implementation.
65-89: LGTM!Good use of inverse variance weighting with appropriate safeguards against extreme weights and proper normalization.
| private Dictionary<string, DeviceParameters> CreateDeviceParamsFromVector(Vector<double> x, List<string> uniqueDeviceIds, ConfigOptimization optimization) | ||
| { | ||
| return nodes.Select(n => | ||
| var deviceParams = new Dictionary<string, DeviceParameters>(); | ||
|
|
||
| for (int i = 0; i < uniqueDeviceIds.Count; i++) | ||
| { | ||
| var distance = n.Rx.Location.DistanceTo(n.Tx.Location); | ||
| var rxAdjRssi = rxAdjRssiDict[n.Rx.Id]; | ||
| var txAdjRssi = rxAdjRssiDict[n.Tx.Id]; | ||
| double absorption; | ||
| var rxAdjRssi = x[i]; | ||
| var absorption = x[i + uniqueDeviceIds.Count]; | ||
|
|
||
| if (pathAbsorptionDict != null) | ||
| // Enforce constraints by clamping values to valid ranges | ||
| rxAdjRssi = Math.Clamp(rxAdjRssi, | ||
| optimization?.RxAdjRssiMin ?? -20, | ||
| optimization?.RxAdjRssiMax ?? 20); | ||
|
|
||
| absorption = Math.Clamp(absorption, | ||
| optimization?.AbsorptionMin ?? 1.5, | ||
| optimization?.AbsorptionMax ?? 4.5); | ||
|
|
||
| deviceParams[uniqueDeviceIds[i]] = new DeviceParameters | ||
| { | ||
| var pathKey = (Min(n.Rx.Id, n.Tx.Id), Max(n.Rx.Id, n.Tx.Id)); | ||
| absorption = pathAbsorptionDict[pathKey]; | ||
| } | ||
| else if (nodeAbsorptionDict != null) | ||
| RxAdjRssi = rxAdjRssi, | ||
| Absorption = absorption | ||
| }; | ||
| } | ||
|
|
||
| return deviceParams; | ||
| } |
There was a problem hiding this comment.
Missing DeviceParameters class definition.
The DeviceParameters class is used throughout but is not defined in this file. Either define it as a nested class or ensure it exists in the Models namespace.
Add the class definition (as a nested class or in a separate file):
private class DeviceParameters
{
public double RxAdjRssi { get; set; }
public double Absorption { get; set; }
public double? TxRefRssi { get; set; } // Add for txRefRssi support
}🧰 Tools
🪛 GitHub Actions: Deploy to Docker
[error] 271-271: /App/src/Optimizers/CombinedOptimizer.cs(271,5): error CS0106: The modifier 'private' is not valid for this item
🤖 Prompt for AI Agents
In src/Optimizers/CombinedOptimizer.cs around lines 271 to 297, the
DeviceParameters type used to build deviceParams is missing; add a
DeviceParameters definition (either as a private nested class in this file or as
a public/internal class in the Models namespace) with at least the properties
double RxAdjRssi { get; set; }, double Absorption { get; set; } and a nullable
double? TxRefRssi { get; set; } so callers compile and txRefRssi support is
available.
| // Calculate distance based on RSSI | ||
| var calculatedDistance = Math.Pow(10, (-59 + rxAdjRssi + txAdjRssi - node.Rssi) / (10.0d * absorption)); |
There was a problem hiding this comment.
Inconsistency: Hardcoded -59 contradicts the PR objective.
The PR aims to replace hardcoded RSSI constants with configurable txRefRssi, but this calculation still uses the hardcoded value -59. For consistency with other optimizers, consider using the transmitter's TxRefRssi parameter.
- // Calculate distance based on RSSI
- var calculatedDistance = Math.Pow(10, (-59 + rxAdjRssi + txAdjRssi - node.Rssi) / (10.0d * absorption));
+ // Calculate distance based on RSSI using txRefRssi from tx parameters
+ var txRefRssi = txParams.TxRefRssi ?? -59;
+ var calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi + txAdjRssi - node.Rssi) / (10.0d * absorption));Note: This requires DeviceParameters to include a TxRefRssi property, which appears to be missing from the current definition.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Optimizers/CombinedOptimizer.cs around lines 327-328, the distance
calculation still uses a hardcoded -59 RSSI constant which contradicts the PR
goal to use configurable transmitter reference RSSI. Replace the -59 literal
with the transmitter reference value (e.g., node.DeviceParameters.TxRefRssi or
the appropriate TxRefRssi field on the transmitter/DeviceParameters object),
add/ensure a TxRefRssi property exists on DeviceParameters, and handle
missing/null values by falling back to a sensible default or by validating and
throwing a clear exception; update any callers/tests that construct
DeviceParameters to provide TxRefRssi.
| var or = new OptimizationResults(); | ||
| var optimization = _state.Config?.Optimization; | ||
|
|
||
| // Group all valid measurements | ||
| var allRxNodes = os.ByRx().SelectMany(g => g).ToList(); | ||
|
|
||
| if (allRxNodes.Count < 3) | ||
| { | ||
| Log.Warning("Not enough valid measurements for optimization"); | ||
| return or; | ||
| } |
There was a problem hiding this comment.
Potential NullReferenceException if Config is null.
The optimization variable is accessed throughout the method (lines 62, 203-206, 212-213, 243-244, 256) without null checks. If _state.Config is null, this will throw.
Apply this diff to add null safety:
var or = new OptimizationResults();
var optimization = _state.Config?.Optimization;
+ if (optimization == null)
+ {
+ Log.Warning("Configuration not available for optimization");
+ return or;
+ }
// Group all valid measurements
- var allRxNodes = os.ByRx().SelectMany(g => g).ToList();
+ var allMeasurements = os.ByRx().SelectMany(g => g).ToList();
- if (allRxNodes.Count < 3)
+ if (allMeasurements.Count < 3)
{
Log.Warning("Not enough valid measurements for optimization");
return or;
}Also consider renaming allRxNodes to allMeasurements throughout the file for clarity, as it contains all measurements, not just Rx nodes.
🤖 Prompt for AI Agents
In src/Optimizers/RxTxAdjRssiOptimizer.cs around lines 25-35, the code grabs
_state.Config?.Optimization into a local but doesn't handle a null
Config/Optimization anywhere else (used later at lines 62, 203-206, 212-213,
243-244, 256) which can cause NullReferenceExceptions; add an explicit
null-safety check right after retrieving optimization (e.g., if
(_state?.Config?.Optimization == null) { Log.Warning("Optimization config
missing"); return or set default Optimization values }) and update all
subsequent uses to rely on this guarded variable; also rename the local
allRxNodes to allMeasurements and replace that identifier throughout the file to
better reflect it contains all measurements, not just Rx nodes.
| double rxAdjRssi = x[rxBaseIndex]; | ||
| double absorption = x[rxBaseIndex + 1]; | ||
| double txRefRssi = x[txBaseIndex]; | ||
|
|
||
| double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * absorption)); | ||
| double mapDistance = node.Rx.Location.DistanceTo(node.Tx.Location); |
There was a problem hiding this comment.
Potential division by zero if absorption approaches zero.
The distance calculation (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * absorption) will fail if absorption is zero. While bounds should prevent this, consider adding a defensive check.
double absorption = x[rxBaseIndex + 1];
double txRefRssi = x[txBaseIndex];
- double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * absorption));
+ double safeAbsorption = Math.Max(absorption, 0.1);
+ double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * safeAbsorption));The same pattern applies in the gradient calculation (lines 166, 177).
📝 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.
| double rxAdjRssi = x[rxBaseIndex]; | |
| double absorption = x[rxBaseIndex + 1]; | |
| double txRefRssi = x[txBaseIndex]; | |
| double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * absorption)); | |
| double mapDistance = node.Rx.Location.DistanceTo(node.Tx.Location); | |
| double rxAdjRssi = x[rxBaseIndex]; | |
| double absorption = x[rxBaseIndex + 1]; | |
| double txRefRssi = x[txBaseIndex]; | |
| double safeAbsorption = Math.Max(absorption, 0.1); | |
| double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * safeAbsorption)); | |
| double mapDistance = node.Rx.Location.DistanceTo(node.Tx.Location); |
| for (int i = 0; i < nodesToUse.Length; i++) | ||
| { | ||
| double dist = Math.Pow(10, (-59 + x[0] - nodesToUse[i].Node.Rssi) / (10.0 * fixedAbsorption)); | ||
| double dist = Math.Pow(10, (txRefRssi + x[0] - nodesToUse[i].Node.Rssi) / (10.0 * fixedAbsorption)); |
There was a problem hiding this comment.
Critical: txRefRssi is undefined and will cause a compilation error.
The variable txRefRssi is used in the Stage 1 distance calculation but is never declared in this file. This applies to all three usages (lines 73, 100, and 102).
Define txRefRssi before the Stage 1 optimization block, retrieving it from the node settings or using a default:
Log.Information("Bounds: RxAdj [{0}, {1}], Absorption [{2}, {3}]", rxAdjMin, rxAdjMax, absorptionMin, absorptionMax);
try
{
+ // Get txRefRssi from node settings or use default
+ double txRefRssi = nodeSettings?.Calibration?.TxRefRssi ?? -59;
+
// Stage 1: Optimize RxAdjRssi with fixed absorption, close nodes only
double fixedAbsorption = absorptionMiddle;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Optimizers/TwoStageRxAdjAbsorptionOptimizer.cs around lines 73, 100 and
102, txRefRssi is referenced but not declared which causes a compilation error;
declare and initialize txRefRssi before the Stage 1 optimization block by
reading it from the relevant node/settings (e.g., nodesToUse[i].Node or a
config/settings object) and fall back to a sensible default value if not
present, then use that variable in the existing distance calculations so the
references at lines 73, 100 and 102 compile and behave predictably.
| double Distance(Vector<double> x, Measure dn) | ||
| { | ||
| double exponent = (-59 + x[0] - dn.Rssi) / (10.0d * x[1]); | ||
| double exponent = (txRefRssi + x[0] - dn.Rssi) / (10.0d * x[1]); | ||
| return (x[1] > 0 && !double.IsInfinity(exponent)) ? Math.Pow(10, exponent) : double.MaxValue; | ||
| } |
There was a problem hiding this comment.
Critical: txRefRssi is undefined and will cause a compilation error.
The variable txRefRssi is used in the exponent calculation but is never declared or initialized in this scope. This will fail to compile.
Based on the PR context (replacing hardcoded -59 with configurable txRefRssi), you likely need to retrieve this from the node settings or optimization config:
+ double txRefRssi = nodeSettings?.Calibration?.TxRefRssi ?? -59;
+
double Distance(Vector<double> x, Measure dn)
{
double exponent = (txRefRssi + x[0] - dn.Rssi) / (10.0d * x[1]);
return (x[1] > 0 && !double.IsInfinity(exponent)) ? Math.Pow(10, exponent) : double.MaxValue;
}Alternatively, if txRefRssi should come from the transmitter device, you may need to retrieve it per-measurement from the Measure object (e.g., dn.RefRssi or similar).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cs around lines 58 to
62, the local function uses txRefRssi which is undefined and causes a compile
error; fix it by obtaining txRefRssi from the correct source and wiring it into
this scope — either read it from the optimizer/config (add a field or method
parameter and pass the configured txRefRssi into the function) or pull it from
the Measure if that object contains the reference RSSI (e.g., use dn.RefRssi
per-measurement); update the Distance signature/closure accordingly so txRefRssi
is defined and used instead of the missing identifier.
DTTerastar
force-pushed
the
fancy-optimize
branch
from
129857d to
f1a2713
Compare
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Optimizers/CombinedOptimizer.cs (1)
140-213: Critical: Severe syntax errors causing build failure.The
OptimizeNodeAbsorptionsmethod has catastrophic syntax issues that prevent compilation. The code appears to have overlapping/interleaved blocks from an incomplete refactor:
- Lines 140-161: Initial guess calculation starts but never completes properly
- Lines 162-174: An objective function lambda begins but structure is broken
- Lines 175-213: A gradient function is partially defined with mismatched braces
- Line 174: Pipeline error reports
CS1513: } expectedThe method signature returns
Dictionary<string, double>but the code attempts to return tuples and has multiplevar solverdeclarations.This file needs to be completely restructured. The intended flow appears to be:
- Create device parameters from optimization vector
- Calculate error using those parameters
- Run gradient-based optimization
- Return results
Consider reverting to a working state and applying changes incrementally.
♻️ Duplicate comments (5)
src/Optimizers/RxTxAdjRssiOptimizer.cs (1)
120-125: Potential division by zero if absorption approaches zero.While bounds should prevent this, add a defensive check to avoid runtime errors:
Proposed fix
double rxAdjRssi = x[rxBaseIndex]; double absorption = x[rxBaseIndex + 1]; double txRefRssi = x[txBaseIndex]; - double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * absorption)); + double safeAbsorption = Math.Max(absorption, 0.1); + double calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi - node.Rssi) / (10.0 * safeAbsorption));Apply the same pattern in the gradient calculation at lines 164-166 and 174-177.
src/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cs (1)
58-62: Critical:txRefRssiis undefined and will cause a compilation error.The variable
txRefRssiis used in theDistancefunction but is never declared in this scope. This will fail to compile.Based on the PR context, you need to retrieve
txRefRssifrom the node settings or use a default value:Proposed fix
+ double txRefRssi = nodeSettings?.Calibration?.TxRefRssi ?? -59; + double Distance(Vector<double> x, Measure dn) { double exponent = (txRefRssi + x[0] - dn.Rssi) / (10.0d * x[1]); return (x[1] > 0 && !double.IsInfinity(exponent)) ? Math.Pow(10, exponent) : double.MaxValue; }src/Optimizers/CombinedOptimizer.cs (3)
48-56: Critical:deviceParamsis undefined in this scope.The variable
deviceParamsis referenced at line 48 but is never declared. The methodOptimizeNodeAbsorptionsreturnsDictionary<string, double>(node absorptions), not a device parameters dictionary.This was flagged in a previous review. The optimization flow needs restructuring to properly capture and use the device parameters.
271-297: MissingDeviceParametersclass definition.The
DeviceParametersclass is used throughout but is not defined. Add a nested class or create it in the Models namespace:Proposed fix
private class DeviceParameters { public double RxAdjRssi { get; set; } public double Absorption { get; set; } public double? TxRefRssi { get; set; } }
327-328: Hardcoded-59contradicts the PR objective.The PR aims to replace hardcoded RSSI constants with configurable
txRefRssi, but this calculation still uses-59. For consistency with other optimizers, use the transmitter'sTxRefRssiparameter.Proposed fix
- var calculatedDistance = Math.Pow(10, (-59 + rxAdjRssi + txAdjRssi - node.Rssi) / (10.0d * absorption)); + var txRefRssi = txParams.TxRefRssi ?? -59; + var calculatedDistance = Math.Pow(10, (txRefRssi + rxAdjRssi + txAdjRssi - node.Rssi) / (10.0d * absorption));Note: This requires
DeviceParametersto include aTxRefRssiproperty.
🧹 Nitpick comments (1)
src/Optimizers/RxTxAdjRssiOptimizer.cs (1)
216-227: Consider using existing settings for initial guesses.The initial
txRefRssiguess is hardcoded to-59. Since the method should acceptexistingSettings, leverage it for better convergence:Proposed fix
// Initialize with a reasonable guess (ensure within bounds) var initialGuess = Vector<double>.Build.Dense(totalParams); foreach (var rxId in uniqueRxIds) { int baseIndex = rxIndexMap[rxId]; - initialGuess[baseIndex] = 0; // initial rxAdjRssi + existingSettings.TryGetValue(rxId, out var rxSettings); + initialGuess[baseIndex] = Math.Clamp( + rxSettings?.Calibration?.RxAdjRssi ?? 0, + optimization.RxAdjRssiMin, + optimization.RxAdjRssiMax); initialGuess[baseIndex + 1] = ((optimization.AbsorptionMax - optimization.AbsorptionMin) / 2.0) + optimization.AbsorptionMin; } foreach (var txId in uniqueTxIds) { - initialGuess[txIndexMap[txId]] = -59; + existingSettings.TryGetValue(txId, out var txSettings); + initialGuess[txIndexMap[txId]] = Math.Clamp( + txSettings?.Calibration?.TxRefRssi ?? -59, + optimization.TxRefRssiMin, + optimization.TxRefRssiMax); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Models/Config.cssrc/Optimizers/CombinedOptimizer.cssrc/Optimizers/GlobalAbsorptionRxTxOptimizer.cssrc/Optimizers/RxTxAdjRssiOptimizer.cssrc/Optimizers/TwoStageRxAdjAbsorptionOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Optimizers/TwoStageRxAdjAbsorptionOptimizer.cs
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Place backend C# ASP.NET Core code under src/ (controllers, services, models, utils)
Files:
src/Optimizers/RxTxAdjRssiOptimizer.cssrc/Models/Config.cssrc/Optimizers/CombinedOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cssrc/Optimizers/GlobalAbsorptionRxTxOptimizer.cs
{src,tests}/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.cs: C#: Use spaces with an indent size of 4
C#: Use PascalCase for types and methods
C#: Use camelCase for local variables and parameters
Files:
src/Optimizers/RxTxAdjRssiOptimizer.cssrc/Models/Config.cssrc/Optimizers/CombinedOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cssrc/Optimizers/GlobalAbsorptionRxTxOptimizer.cs
🧠 Learnings (2)
📚 Learning: 2025-09-16T00:51:01.127Z
Learnt from: CR
Repo: ESPresense/ESPresense-companion PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T00:51:01.127Z
Learning: Applies to src/**/Controllers/StateController.cs : StateController must populate Device.ConfiguredRefRssi from DeviceSettings during API calls
Applied to files:
src/Models/Config.cssrc/Optimizers/CombinedOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cs
📚 Learning: 2025-09-16T00:51:01.127Z
Learnt from: CR
Repo: ESPresense/ESPresense-companion PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T00:51:01.127Z
Learning: Applies to src/ui/src/routes/calibration/**/*.svelte : Calibration UI should determine "Calibrated" status only from configured values (Device.ConfiguredRefRssi), not measured values
Applied to files:
src/Optimizers/CombinedOptimizer.cssrc/Optimizers/WeightedJointRxAdjAbsorptionOptimizer.cs
🧬 Code graph analysis (1)
src/Optimizers/CombinedOptimizer.cs (2)
src/Models/State.cs (2)
State(13-272)State(25-88)src/Models/OptimizationSnapshot.cs (1)
Measure(40-57)
🪛 GitHub Actions: Build and test
src/Optimizers/CombinedOptimizer.cs
[error] 174-174: CS1513: } expected. Compilation failed at CombinedOptimizer.cs(174,14) while running 'dotnet build -c Release --no-restore'.
🪛 GitHub Actions: Deploy to Docker
src/Optimizers/CombinedOptimizer.cs
[error] 174-239: dotnet publish failed. Compilation errors in CombinedOptimizer.cs: multiple CS1002/CS1003/CS1513/CS1524/CS1026/CS1519 errors indicating syntax issues (e.g., missing braces, invalid tokens) leading to build failure during publishing.
🪛 GitHub Check: build
src/Optimizers/CombinedOptimizer.cs
[failure] 213-213:
Invalid token ')' in a member declaration
[failure] 204-204:
} expected
[failure] 204-204:
; expected
[failure] 204-204:
; expected
[failure] 204-204:
) expected
[failure] 203-203:
} expected
[failure] 203-203:
; expected
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/Optimizers/GlobalAbsorptionRxTxOptimizer.cs (1)
341-341: LGTM!The trailing newline addition is a minor formatting improvement with no functional impact. The optimizer implementation is well-structured with proper bounds validation, error handling, and logging.
src/Models/Config.cs (1)
137-138: Verify the impact of widened TxRefRssi bounds on existing deployments.The default bounds have been significantly widened from
[-70, -50]to[-80, -40](40dB range vs previous 20dB range). This gives the optimizer more flexibility but may affect existing configurations that relied on the narrower defaults.Consider documenting this change for users who may need to adjust their
limitsconfiguration.src/Optimizers/RxTxAdjRssiOptimizer.cs (1)
91-107: Asymmetric error function uses fourth power for penalties.The
calculateDistanceErrorfunction usesMath.Pow(map - calculated, 4)for "impossible" cases (calculated < map) butMath.Pow(map - calculated, 2)for normal cases. This is a valid approach for asymmetric penalization, but be aware:
- Fourth-power penalties can dominate optimization and cause numerical instability with large distance errors
- Consider adding a cap similar to
GlobalAbsorptionRxTxOptimizer'sCappedError = 2560to prevent outliers from skewing results
| public OptimizationResults Optimize(OptimizationSnapshot os) | ||
| { | ||
| var or = new OptimizationResults(); | ||
| var optimization = _state.Config?.Optimization; | ||
|
|
||
| // Group all valid measurements | ||
| var allRxNodes = os.ByRx().SelectMany(g => g).ToList(); | ||
|
|
||
| if (allRxNodes.Count < 3) | ||
| { | ||
| Log.Warning("Not enough valid measurements for optimization"); | ||
| return or; | ||
| } |
There was a problem hiding this comment.
Method signature doesn't match IOptimizer interface.
The Optimize method takes only OptimizationSnapshot os, but based on other optimizers in this PR (e.g., GlobalAbsorptionRxTxOptimizer, WeightedJointRxAdjAbsorptionOptimizer), the IOptimizer interface requires:
OptimizationResults Optimize(OptimizationSnapshot os, Dictionary<string, NodeSettings> existingSettings)This will cause a compilation error. Additionally, add a null check for optimization before accessing its properties.
Proposed fix
- public OptimizationResults Optimize(OptimizationSnapshot os)
+ public OptimizationResults Optimize(OptimizationSnapshot os, Dictionary<string, NodeSettings> existingSettings)
{
var or = new OptimizationResults();
var optimization = _state.Config?.Optimization;
+
+ if (optimization == null)
+ {
+ Log.Warning("Configuration not available for optimization");
+ return or;
+ }
// Group all valid measurements
var allRxNodes = os.ByRx().SelectMany(g => g).ToList();🤖 Prompt for AI Agents
In src/Optimizers/RxTxAdjRssiOptimizer.cs around lines 23 to 35, the Optimize
method currently has the wrong signature and may dereference a null
configuration: change the method signature to match the IOptimizer interface:
OptimizationResults Optimize(OptimizationSnapshot os, Dictionary<string,
NodeSettings> existingSettings), update all local usages/call sites accordingly,
and add a null check for var optimization = _state.Config?.Optimization before
accessing its members (return empty OptimizationResults or log and exit early if
optimization is null).
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.