Codex: publish bayesian room probabilities #1279
Conversation
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
📝 WalkthroughWalkthroughAdds device-level Bayesian room-probability support: config options, model fields and cloning, device state and discovery records, MQTT publishing of per-room probabilities and attributes, Home Assistant autodiscovery metadata, locator logic to compute/publish probabilities, untracking reset, README docs, and a unit test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Device
participant MSL as MultiScenarioLocator
participant MQTT as MqttCoordinator
participant HA as Home Assistant (Auto-Discovery)
participant Hist as DeviceHistoryStore
Note over Dev,MSL: Processing cycle (Bayesian enabled)
Dev->>MSL: Provide scenarios & locators
MSL->>MSL: Build and normalize per-room probability vector (epsilon)
alt Probabilities available
MSL->>MQTT: Publish per-room `probabilities/<room>` topics (retain per config)
MSL->>MQTT: Publish device state/attributes including `probabilities` object
MSL->>HA: Create/Update discovery sensors for rooms >= discovery_threshold
opt Best location exists & reporting enabled
MSL->>Hist: Record history / emit device-changed events
end
else Disabled or no data
MSL->>MQTT: Clear probability topics/attributes (if configured)
MSL->>HA: Remove probability discovery entities
end
Note over Dev,MSL: Untracking path
Dev-->>MSL: Device becomes untracked
MSL->>HA: Delete autodiscovery entries
MSL->>Dev: ResetBayesianState()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus during review:
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (5)src/**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/config.example.yaml📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src,tests}/**/*.cs📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/**/*.cs📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/**/*Tests.cs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-09-16T00:51:01.127ZApplied to files:
🧬 Code graph analysis (4)src/Models/Config.cs (1)
tests/MultiScenarioLocatorTests.cs (2)
src/Models/Config.Clone.cs (1)
src/Services/MultiScenarioLocator.cs (3)
🪛 GitHub Actions: Build and testsrc/Services/MultiScenarioLocator.cs[error] 200-200: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and '' 🪛 GitHub Actions: Deploy to Dockersrc/Services/MultiScenarioLocator.cs[error] 200-202: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and '' 🪛 GitHub Check: buildsrc/Services/MultiScenarioLocator.cs[warning] 141-141: [failure] 202-202: [failure] 201-201: [failure] 200-200: [failure] 202-202: [failure] 201-201: [failure] 200-200: ⏰ 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)
🔇 Additional comments (14)
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: 2
🧹 Nitpick comments (9)
src/config.example.yaml (1)
55-59: Clarify threshold range in example configAdd explicit 0.0–1.0 range to avoid misconfiguration.
bayesian_probabilities: enabled: false # Publish per-room probability vectors (default disabled) - discovery_threshold: 0.1 # Minimum probability before creating Home Assistant sensor + discovery_threshold: 0.1 # Minimum probability before creating Home Assistant sensor (range: 0.0–1.0) retain: true # Retain MQTT probability topics so sensors restore on restartREADME.md (1)
23-58: Document probability range and topic naming
- Mention values are 0.0–1.0 to set expectations.
- Note room segment sanitization/lowercasing (as topics use
.../probabilities/kitchen).src/Models/Config.Clone.cs (1)
27-29: Make clone resilient to nullsDefensive null-guards prevent NREs if any sub-config is ever deserialized as null.
- Optimization = Optimization.Clone(), - BayesianProbabilities = BayesianProbabilities.Clone() + Optimization = Optimization?.Clone() ?? new ConfigOptimization(), + BayesianProbabilities = BayesianProbabilities?.Clone() ?? new ConfigBayesianProbabilities()src/Models/Config.cs (1)
146-156: Clamp discovery_threshold to 0.0–1.0Prevents invalid config values from propagating.
- public partial class ConfigBayesianProbabilities - { - [YamlMember(Alias = "enabled")] - public bool Enabled { get; set; } = false; - - [YamlMember(Alias = "discovery_threshold")] - public double DiscoveryThreshold { get; set; } = 0.1; - - [YamlMember(Alias = "retain")] - public bool Retain { get; set; } = true; - } + public partial class ConfigBayesianProbabilities + { + [YamlMember(Alias = "enabled")] + public bool Enabled { get; set; } = false; + + private double _discoveryThreshold = 0.1; + [YamlMember(Alias = "discovery_threshold")] + public double DiscoveryThreshold + { + get => _discoveryThreshold; + set => _discoveryThreshold = value < 0 ? 0 : (value > 1 ? 1 : value); + } + + [YamlMember(Alias = "retain")] + public bool Retain { get; set; } = true; + }tests/MultiScenarioLocatorTests.cs (1)
82-165: Great coverage; consider a couple more assertions
- Assert that probability topics respect
retain: true.- Assert a discovery config is published for rooms above
discovery_threshold.- Add a test that untracking a device deletes probability sensors (config topic with null payload).
Example additions:
+ // Retain flag honored + Assert.That(published.Any(p => p.topic == probabilityTopic && p.retain == true), Is.True); + + // Discovery config published for Kitchen + var disc = published.FirstOrDefault(p => p.topic.Contains($"/sensor/espresense-{device.Id}-kitchen-probability/config")); + Assert.That(disc.payload, Is.Not.Null.And.Not.Empty);src/Services/MultiScenarioLocator.cs (4)
271-281: Reduce publish churn by comparing rounded values to what you send.You publish with 4-decimal precision but compare using raw doubles with epsilon. Compare after rounding and persist the rounded value to avoid tiny oscillations.
Apply:
- var payloadChanged = !device.BayesianProbabilities.TryGetValue(roomName, out var existing) || Math.Abs(existing - probability) > ProbabilityEpsilon; + var rounded = Math.Round(probability, 4, MidpointRounding.AwayFromZero); + var payloadChanged = !device.BayesianProbabilities.TryGetValue(roomName, out var existing) || Math.Abs(existing - rounded) > ProbabilityEpsilon; if (payloadChanged) { - var payload = probability.ToString("0.####", CultureInfo.InvariantCulture); + var payload = rounded.ToString("0.####", CultureInfo.InvariantCulture); await mqtt.EnqueueAsync(BuildProbabilityTopic(device.Id, roomName), payload, retain: config.Retain); - device.BayesianProbabilities[roomName] = probability; + device.BayesianProbabilities[roomName] = rounded; changed = true; }
338-363: HA discovery: add unit and suggested display for probabilities.Publishing 0..1 benefits from a unit and suggested display. Consider percent for UX.
Example:
var record = new AutoDiscovery.DiscoveryRecord { Name = $"{device.Name ?? device.Id} {roomName} Probability", UniqueId = $"espresense-companion-{device.Id}-{sanitizedRoom}", StateTopic = BuildProbabilityTopic(device.Id, roomName), EntityStatusTopic = "espresense/companion/status", Device = new AutoDiscovery.DeviceRecord { Name = device.Name ?? device.Id, Manufacturer = "ESPresense", Model = "Companion", SwVersion = "1.0.0", Identifiers = new[] { $"espresense-{device.Id}" } }, Origin = new AutoDiscovery.OriginRecord { Name = "ESPresense Companion" }, StateClass = "measurement", - Icon = "mdi:account-location" + Icon = "mdi:account-location", + UnitOfMeasurement = "%", + SuggestedDisplayPrecision = 1 };If you keep 0..1 on the wire, alternatively set
DeviceClassor document scaling. Verify property names matchAutoDiscovery.DiscoveryRecord.
211-264: Probability vector edge-cases look good; minor naming consistency nit.Both “unknown” and “other” appear; consider consistently using one bucket for absent/overflow mass.
If desired, replace the initial
"not_home"with"other"(or vice versa) for uniformity across sensors and payloads.
163-165: Make lat/lon/elevation typing explicit (use nullable locals)gps.Add returns (double? lat, double? lon) — src/Extensions/GpsUtilExtensions.cs. The ternary with gps.Add(...): (null, null) can confuse the compiler; declare nullable locals and assign them inside the if.
File: src/Services/MultiScenarioLocator.cs (around lines 163–165)
Apply:
- var (lat, lon) = includeLocation && gps?.Report == true ? gps.Add(location.X, location.Y) : (null, null); - var elevation = includeLocation && gps?.Report == true ? location.Z + gps?.Elevation : null; + double? lat = null, lon = null, elevation = null; + if (includeLocation && gps?.Report == true) + { + var (latVal, lonVal) = gps.Add(location.X, location.Y); + lat = latVal; + lon = lonVal; + elevation = location.Z + gps?.Elevation; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(2 hunks)src/Models/AutoDiscovery.cs(1 hunks)src/Models/Config.Clone.cs(2 hunks)src/Models/Config.cs(2 hunks)src/Models/Device.cs(2 hunks)src/Services/DeviceTracker.cs(1 hunks)src/Services/MultiScenarioLocator.cs(4 hunks)src/config.example.yaml(1 hunks)tests/MultiScenarioLocatorTests.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Place backend C# ASP.NET Core code under src/ (controllers, services, models, utils)
Files:
src/Models/AutoDiscovery.cssrc/Models/Device.cssrc/Services/MultiScenarioLocator.cssrc/Services/DeviceTracker.cssrc/Models/Config.Clone.cssrc/config.example.yamlsrc/Models/Config.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/Models/AutoDiscovery.cssrc/Models/Device.cssrc/Services/MultiScenarioLocator.cssrc/Services/DeviceTracker.cssrc/Models/Config.Clone.cstests/MultiScenarioLocatorTests.cssrc/Models/Config.cs
tests/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
Place backend NUnit tests under tests/
Files:
tests/MultiScenarioLocatorTests.cs
tests/**/*Tests.cs
📄 CodeRabbit inference engine (AGENTS.md)
Name backend NUnit test files with the *Tests.cs suffix (e.g., TimeSpanExtensionsTests.cs)
Files:
tests/MultiScenarioLocatorTests.cs
src/config.example.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Use src/config.example.yaml as a template and do not commit real secrets
Files:
src/config.example.yaml
🧬 Code graph analysis (5)
src/Models/Device.cs (2)
src/Models/AutoDiscovery.cs (3)
AutoDiscovery(9-156)AutoDiscovery(17-40)AutoDiscovery(42-47)src/Services/MultiScenarioLocator.cs (1)
AutoDiscovery(338-363)
src/Services/MultiScenarioLocator.cs (4)
src/Models/Config.cs (2)
Config(8-58)ConfigBayesianProbabilities(146-156)src/Models/Device.cs (2)
Device(10-184)Device(22-27)src/Services/MqttCoordinator.cs (12)
Task(69-76)Task(78-120)Task(122-189)Task(204-261)Task(273-295)Task(306-334)Task(349-391)Task(407-433)Task(448-470)Task(473-498)Task(500-509)Task(511-533)src/Models/AutoDiscovery.cs (5)
AutoDiscovery(9-156)AutoDiscovery(17-40)AutoDiscovery(42-47)DiscoveryRecord(110-137)DeviceRecord(139-150)
src/Services/DeviceTracker.cs (1)
src/Models/Device.cs (1)
ResetBayesianState(139-148)
src/Models/Config.Clone.cs (1)
src/Models/Config.cs (1)
ConfigBayesianProbabilities(146-156)
src/Models/Config.cs (1)
src/Models/Config.Clone.cs (2)
ConfigBayesianProbabilities(120-131)ConfigBayesianProbabilities(122-130)
🪛 GitHub Check: build
src/Services/MultiScenarioLocator.cs
[failure] 174-174:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 173-173:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 172-172:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 174-174:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 173-173:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 172-172:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
🪛 GitHub Actions: Build and test
src/Services/MultiScenarioLocator.cs
[error] 172-172: dotnet build -c Release --no-restore failed: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''.
⏰ 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 (12)
src/Models/AutoDiscovery.cs (1)
122-131: Extended HA discovery metadata looks goodFields match HA discovery keys and are null-suppressed by serializer settings.
README.md (1)
12-12: Docs: nice additionBullet succinctly advertises the new capability.
src/Models/Config.Clone.cs (1)
120-132: BayesianProbabilities.Clone() is correctDeep copy of flags and settings is appropriate.
src/Models/Config.cs (1)
49-51: Config surface addition LGTMTop‑level
bayesian_probabilitieswith sensible defaults is consistent with the example YAML.src/Models/Device.cs (2)
76-81: Thread‑safe state containers for probabilities/discoveriesGood choice with case‑insensitive ConcurrentDictionary and JsonIgnore.
139-148: Ensure HA sensor deletion is emitted
ResetBayesianState()removes probability discoveries fromHassAutoDiscoveryand clears dictionaries but does not publish delete messages itself. Relying on the earlier loop overHassAutoDiscoveryis fine only if probability sensors were inserted there. Please confirm; otherwise, emit deletes forBayesianDiscoveriesbefore clearing.tests/MultiScenarioLocatorTests.cs (4)
1-1: Import changes OKNeeded for types used below.
3-3: Using order change is fineNo functional impact.
8-9: New test deps are appropriate
JObjectparsing andPoint3Dusage are justified.
21-35: FixedRoomLocator helper is cleanSets room/floor/confidence and advances fixes; suitable for deterministic tests.
src/Services/DeviceTracker.cs (1)
215-215: Resolved — probability discoveries are registered to HassAutoDiscoveryPublishProbabilitySensorsAsync/CreateProbabilityDiscovery add the probability AutoDiscovery to device.HassAutoDiscovery before sending (see src/Services/MultiScenarioLocator.cs:284–286, 338–343), so ResetBayesianState after removing entities will not leave probability sensors behind.
src/Services/MultiScenarioLocator.cs (1)
371-389: Segment sanitizer: safe and HA/MQTT-friendly. LGTM.Good normalization and fallback to “unknown”.
src/Services/MultiScenarioLocator.cs
Outdated
| source_type = includeLocation ? "espresense" : null, | ||
| latitude = includeLocation ? lat : null, | ||
| longitude = includeLocation ? lon : null, | ||
| elevation = includeLocation ? elevation : null, | ||
| x = includeLocation ? location.X : null, | ||
| y = includeLocation ? location.Y : null, | ||
| z = includeLocation ? location.Z : null, | ||
| confidence = bestScenario?.Confidence, |
There was a problem hiding this comment.
Fix CS0173: ternary mixes double with null (x/y/z).
Anonymous object initializers can’t infer nullable from double : null. Cast the doubles to double?.
Apply:
- x = includeLocation ? location.X : null,
- y = includeLocation ? location.Y : null,
- z = includeLocation ? location.Z : null,
+ x = includeLocation ? (double?)location.X : null,
+ y = includeLocation ? (double?)location.Y : null,
+ z = includeLocation ? (double?)location.Z : null,📝 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.
| source_type = includeLocation ? "espresense" : null, | |
| latitude = includeLocation ? lat : null, | |
| longitude = includeLocation ? lon : null, | |
| elevation = includeLocation ? elevation : null, | |
| x = includeLocation ? location.X : null, | |
| y = includeLocation ? location.Y : null, | |
| z = includeLocation ? location.Z : null, | |
| confidence = bestScenario?.Confidence, | |
| source_type = includeLocation ? "espresense" : null, | |
| latitude = includeLocation ? lat : null, | |
| longitude = includeLocation ? lon : null, | |
| elevation = includeLocation ? elevation : null, | |
| x = includeLocation ? (double?)location.X : null, | |
| y = includeLocation ? (double?)location.Y : null, | |
| z = includeLocation ? (double?)location.Z : null, | |
| confidence = bestScenario?.Confidence, |
🧰 Tools
🪛 GitHub Check: build
[failure] 174-174:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 173-173:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 172-172:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 174-174:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 173-173:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 172-172:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
🪛 GitHub Actions: Build and test
[error] 172-172: dotnet build -c Release --no-restore failed: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''.
🤖 Prompt for AI Agents
In src/Services/MultiScenarioLocator.cs around lines 168 to 175, the ternary
expressions for x, y, z (and similarly latitude/longitude/elevation if hitting
the same CS0173) mix plain double with null causing type inference errors; fix
by casting the double operands to nullable doubles, e.g. change includeLocation
? location.X : null to includeLocation ? (double?)location.X : null (and
likewise for y, z, latitude, longitude, elevation if needed).
…d test coverage This commit addresses all the feedback from PR #1279 review: **Compilation Fixes:** - Fix CS0173 type inference errors in MultiScenarioLocator.cs by casting double values to nullable (double?) - Properly handle x, y, z coordinate nullable types in JSON serialization **Logic Improvements:** - Fix MQTT retain logic for tombstone messages (always use retain:true when clearing topics) - Add validation clamping for DiscoveryThreshold to ensure 0.0-1.0 range - Round probability values to 4 decimal places before comparison to reduce publish churn **Code Quality:** - Add null-guards in Config.Clone.cs to prevent NullReferenceExceptions - Use null-conditional operators for all Clone() calls **Documentation:** - Clarify threshold range (0.0-1.0) in README and example config - Document room name normalization to lowercase for MQTT topics - Add inline comments about configuration ranges **Test Coverage:** - Add assertions to verify retain flag is honored on probability topics - Add assertions to verify discovery configs are published for rooms above threshold - Add test scenario for untracking devices to verify probability sensors are deleted - Add assertions to verify tombstone messages use retain:true
DTTerastar
force-pushed
the
codex/implement-changes-from-issue-#1278
branch
from
92fc9b4 to
212205f
Compare
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Services/MultiScenarioLocator.cs (2)
159-180: Fix CS0173 errors by using nullable doubles for GPS/location fieldsThe attribute payload currently mixes non‑nullable doubles with
nullin ternaries, which triggers CS0173 and breaks the build (as seen in the pipeline). Similar issues exist for the(lat, lon)tuple and thex/y/zproperties.Use nullable doubles and avoid tuple ternary with
(null, null). For example:- var gps = state?.Config?.Gps; - var location = device.Location ?? new Point3D(); - - // Only include GPS coordinates if reporting is enabled and we have a best scenario - var (lat, lon) = includeLocation && gps?.Report == true ? gps.Add(location.X, location.Y) : (null, null); - var elevation = includeLocation && gps?.Report == true ? location.Z + gps?.Elevation : null; + var gps = state?.Config?.Gps; + var location = device.Location ?? new Point3D(); + + double? lat = null; + double? lon = null; + double? elevation = null; + + // Only include GPS coordinates if reporting is enabled and we have a best scenario + if (includeLocation && gps?.Report == true) + { + var (rawLat, rawLon) = gps.Add(location.X, location.Y); + lat = rawLat; + lon = rawLon; + elevation = location.Z + gps?.Elevation; + } @@ - source_type = includeLocation ? "espresense" : null, - latitude = includeLocation ? lat : null, - longitude = includeLocation ? lon : null, - elevation = includeLocation ? elevation : null, - x = includeLocation ? location.X : null, - y = includeLocation ? location.Y : null, - z = includeLocation ? location.Z : null, + source_type = includeLocation ? "espresense" : null, + latitude = lat, + longitude = lon, + elevation = elevation, + x = includeLocation ? (double?)location.X : null, + y = includeLocation ? (double?)location.Y : null, + z = includeLocation ? (double?)location.Z : null,This keeps JSON semantics unchanged (nulls are still omitted via
SerializerSettings.NullIgnore) while satisfying the compiler.Please rerun your CI/build after applying this to ensure all CS0173 instances are resolved.
266-314: Always retain null “tombstone” messages when clearing probability topicsIn the stale‑key cleanup loop, null payloads currently use
retain: config.Retain:await mqtt.EnqueueAsync(BuildProbabilityTopic(device.Id, key), null, retain: config.Retain);If
config.Retainis false, this will not clear an already‑retained probability value on the broker; consumers may continue to see stale probabilities.Align this with
ClearProbabilityOutputsAsync(which usesretain: true) so tombstones reliably clear retained state:- await mqtt.EnqueueAsync(BuildProbabilityTopic(device.Id, key), null, retain: config.Retain); + await mqtt.EnqueueAsync(BuildProbabilityTopic(device.Id, key), null, retain: true);Retaining the null payload is the standard MQTT pattern for clearing retained topics, independent of the normal “retain” setting for non‑null updates.
Please confirm this matches your intended MQTT semantics, especially for users who flip
retainoff after running with it enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(2 hunks)src/Models/AutoDiscovery.cs(1 hunks)src/Models/Config.Clone.cs(2 hunks)src/Models/Config.cs(2 hunks)src/Models/Device.cs(2 hunks)src/Services/DeviceTracker.cs(1 hunks)src/Services/MultiScenarioLocator.cs(4 hunks)src/config.example.yaml(1 hunks)tests/MultiScenarioLocatorTests.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/MultiScenarioLocatorTests.cs
- src/Models/AutoDiscovery.cs
- src/Models/Device.cs
- src/Models/Config.Clone.cs
- src/config.example.yaml
🧰 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/Services/DeviceTracker.cssrc/Services/MultiScenarioLocator.cssrc/Models/Config.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/Services/DeviceTracker.cssrc/Services/MultiScenarioLocator.cssrc/Models/Config.cs
🧬 Code graph analysis (3)
src/Services/DeviceTracker.cs (1)
src/Models/Device.cs (1)
ResetBayesianState(139-148)
src/Services/MultiScenarioLocator.cs (5)
src/Models/Config.cs (2)
Config(8-58)ConfigBayesianProbabilities(146-156)src/Utils/JsonSerializerSettings.cs (1)
SerializerSettings(6-9)src/Models/Device.cs (3)
Device(10-184)Device(22-27)ToString(29-36)src/Services/MqttCoordinator.cs (12)
Task(73-80)Task(82-130)Task(132-219)Task(251-304)Task(306-334)Task(349-406)Task(423-446)Task(457-485)Task(500-542)Task(558-584)Task(599-621)Task(624-649)src/Models/AutoDiscovery.cs (6)
AutoDiscovery(9-156)AutoDiscovery(17-40)AutoDiscovery(42-47)DiscoveryRecord(110-137)DeviceRecord(139-150)OriginRecord(152-155)
src/Models/Config.cs (1)
src/Models/Config.Clone.cs (2)
ConfigBayesianProbabilities(120-131)ConfigBayesianProbabilities(122-130)
🪛 GitHub Actions: Build and test
src/Services/MultiScenarioLocator.cs
[error] 172-172: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
🪛 GitHub Actions: Deploy to Docker
src/Services/MultiScenarioLocator.cs
[error] 172-174: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
🪛 GitHub Check: build
src/Services/MultiScenarioLocator.cs
[failure] 174-174:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 173-173:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 172-172:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 174-174:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 173-173:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 172-172:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
⏰ 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 (6)
src/Services/DeviceTracker.cs (1)
212-216: Resetting Bayesian state on untrack is a good lifecycle cleanupClearing Bayesian discoveries and probabilities when a device becomes untracked avoids stale in‑memory state and keeps
HassAutoDiscoveryaligned with the MQTT deletions above.README.md (1)
12-57: Bayesian probability docs match the implementationThe new description, config snippet, MQTT topics, and Home Assistant examples line up with
ConfigBayesianProbabilities, the probability topics, and attribute payload shape in code.src/Models/Config.cs (1)
49-51: ConfigBayesianProbabilities wiring and defaults look consistentThe
bayesian_probabilitiessection, its mapping toConfigBayesianProbabilities, and the Enabled/DiscoveryThreshold/Retain fields (with aliases and defaults) are consistent with the README and consumer code inMultiScenarioLocator.If you adjust defaults or field names later, please double‑check
Config.Clone.csand any YAML docs stay in sync with this shape.Also applies to: 146-156
src/Services/MultiScenarioLocator.cs (3)
7-9: Bayesian smoothing and config gating are well‑structuredThe new imports, smoothing constants, and the
BayesianProbabilitiesblock (config check → vector → publish/clear) integrate cleanly intoProcessDeviceand keep the feature nicely feature‑flagged, with attribute payloads only built when probabilities or state changed.Also applies to: 30-33, 109-129
211-264: Probability vector construction and topic sanitization look solidThe
BuildProbabilityVectorlogic (room key selection, normalization,other/not_homehandling, and sum correction) plusSanitizeSegment/BuildProbabilityTopicgive a coherent, stable probability distribution and safe MQTT topic segments. Skipping synthetic rooms viaIsSyntheticRoomis a nice touch to avoid noisy entities.If you later introduce additional synthetic room labels, remember to extend
IsSyntheticRoomto keep discovery behavior consistent.Also applies to: 365-392
316-336: ClearProbabilityOutputsAsync behavior is correct and consistentThe probability clear helper reliably sends retained null “tombstones” for all probability topics and deletes all Bayesian discovery configs, then clears the in‑memory dictionaries. This matches the expected behavior when the feature is disabled or turned off in config.
Once the stale‑key cleanup in
PublishProbabilitySensorsAsyncis aligned to also useretain: true, the clearing behavior will be consistent across both paths.
…nd test coverage This commit addresses all the feedback from PR #1279 review: **Compilation Fixes:** - Fix CS0173 type inference errors in MultiScenarioLocator.cs by casting double values to nullable (double?) - Properly handle x, y, z coordinate nullable types in JSON serialization **Logic Improvements:** - Fix MQTT retain logic for tombstone messages (always use retain:true when clearing topics) - Add validation clamping for DiscoveryThreshold to ensure 0.0-1.0 range - Round probability values to 4 decimal places before comparison to reduce publish churn **Code Quality:** - Add null-guards in Config.Clone.cs to prevent NullReferenceExceptions - Use null-conditional operators for all Clone() calls **Documentation:** - Clarify threshold range (0.0-1.0) in README and example config - Document room name normalization to lowercase for MQTT topics - Add inline comments about configuration ranges **Test Coverage:** - Add assertions to verify retain flag is honored on probability topics - Add assertions to verify discovery configs are published for rooms above threshold - Add test scenario for untracking devices to verify probability sensors are deleted - Add assertions to verify tombstone messages use retain:true
…d test coverage This commit addresses all the feedback from PR #1279 review: **Compilation Fixes:** - Fix CS0173 type inference errors in MultiScenarioLocator.cs by casting double values to nullable (double?) - Properly handle x, y, z coordinate nullable types in JSON serialization **Logic Improvements:** - Fix MQTT retain logic for tombstone messages (always use retain:true when clearing topics) - Add validation clamping for DiscoveryThreshold to ensure 0.0-1.0 range - Round probability values to 4 decimal places before comparison to reduce publish churn **Code Quality:** - Add null-guards in Config.Clone.cs to prevent NullReferenceExceptions - Use null-conditional operators for all Clone() calls **Documentation:** - Clarify threshold range (0.0-1.0) in README and example config - Document room name normalization to lowercase for MQTT topics - Add inline comments about configuration ranges **Test Coverage:** - Add assertions to verify retain flag is honored on probability topics - Add assertions to verify discovery configs are published for rooms above threshold - Add test scenario for untracking devices to verify probability sensors are deleted - Add assertions to verify tombstone messages use retain:true
DTTerastar
changed the title
DTTerastar
force-pushed
the
codex/implement-changes-from-issue-#1278
branch
from
212205f to
8ca8d2c
Compare
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
…d test coverage This commit addresses all the feedback from PR #1279 review: **Compilation Fixes:** - Fix CS0173 type inference errors in MultiScenarioLocator.cs by casting double values to nullable (double?) - Properly handle x, y, z coordinate nullable types in JSON serialization **Logic Improvements:** - Fix MQTT retain logic for tombstone messages (always use retain:true when clearing topics) - Add validation clamping for DiscoveryThreshold to ensure 0.0-1.0 range - Round probability values to 4 decimal places before comparison to reduce publish churn **Code Quality:** - Add null-guards in Config.Clone.cs to prevent NullReferenceExceptions - Use null-conditional operators for all Clone() calls **Documentation:** - Clarify threshold range (0.0-1.0) in README and example config - Document room name normalization to lowercase for MQTT topics - Add inline comments about configuration ranges **Test Coverage:** - Add assertions to verify retain flag is honored on probability topics - Add assertions to verify discovery configs are published for rooms above threshold - Add test scenario for untracking devices to verify probability sensors are deleted - Add assertions to verify tombstone messages use retain:true
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d00873dd8c8324bde1d937e7cd16f8
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.