Claude's Publish Bayesian Room Probabilities #1345
Conversation
📝 WalkthroughWalkthroughAdds configurable Bayesian probability computation and per-room MQTT outputs with Home Assistant auto-discovery; extends configuration and models to store/clone Bayesian settings; integrates probability publishing and cleanup into locator and tracking flows; adds tests verifying publish and teardown. Changes
Sequence DiagramsequenceDiagram
participant Device
participant MultiScenarioLocator
participant ProbabilityBuilder
participant MqttCoordinator
participant HomeAssistant
Device->>MultiScenarioLocator: Process device scenarios
alt bayesian enabled
MultiScenarioLocator->>ProbabilityBuilder: BuildProbabilityVector(scenarios)
ProbabilityBuilder-->>MultiScenarioLocator: Normalized probabilities by room
MultiScenarioLocator->>MqttCoordinator: PublishProbabilitySensorsAsync(probabilities)
loop publish sensors & discovery
MqttCoordinator->>HomeAssistant: Publish state (retain)
MqttCoordinator->>HomeAssistant: Publish discovery payload
end
loop cleanup stale
MqttCoordinator->>HomeAssistant: Publish null/tombstone (retain as configured)
MqttCoordinator->>HomeAssistant: Delete discovery (null payload)
end
else disabled
MultiScenarioLocator->>MqttCoordinator: ClearProbabilityOutputsAsync()
end
Device->>MultiScenarioLocator: Untrack device
MultiScenarioLocator->>Device: ResetBayesianState()
MultiScenarioLocator->>MqttCoordinator: Publish tombstones for probabilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
DTTerastar
changed the base branch from
codex/implement-changes-from-issue-#1278
to
main
DTTerastar
changed the title
DTTerastar
force-pushed
the
claude/fix-espresense-build-01Cn9KwMrsqzZjQJ6RZxrs6X
branch
from
00238f8 to
93cc908
Compare
DTTerastar
temporarily deployed
to
CI - release environment
GitHub Actions
Inactive
…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
claude/fix-espresense-build-01Cn9KwMrsqzZjQJ6RZxrs6X
branch
from
93cc908 to
84c3bb6
Compare
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
…tion Fixes CS0173 compilation errors where ternary operator couldn't determine type between double and null. Added explicit (double?) casts for x, y, z coordinates in the attributes payload.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Services/MultiScenarioLocator.cs (3)
306-306: Consider using ProbabilityEpsilon constant for consistency.The hardcoded value
0.0001differs from theProbabilityEpsilonconstant (0.001) defined at line 34. For consistency, consider using the constant or documenting why a different threshold is needed here.Apply this diff if you want to use the existing constant:
- if (remainder > 0.0001) + if (remainder > ProbabilityEpsilon)
339-351: Minor: Else-if clause catches both conditions.The
else ifat line 346 will execute for both synthetic rooms and rooms below the discovery threshold, which is correct but slightly less explicit than separate conditions. The current implementation works correctly sinceTryRemovehandles missing keys gracefully.If you prefer more explicit logic, consider:
- if (!IsSyntheticRoom(roomName) && probability >= config.DiscoveryThreshold) - { - var discovery = device.BayesianDiscoveries.GetOrAdd(roomName, key => CreateProbabilityDiscovery(device, key)); - if (!device.HassAutoDiscovery.Contains(discovery)) - device.HassAutoDiscovery.Add(discovery); - await discovery.Send(mqtt); - } - else if (device.BayesianDiscoveries.TryRemove(roomName, out var staleDiscovery)) - { - device.HassAutoDiscovery.Remove(staleDiscovery); - await staleDiscovery.Delete(mqtt); - changed = true; - } + var shouldHaveDiscovery = !IsSyntheticRoom(roomName) && probability >= config.DiscoveryThreshold; + if (shouldHaveDiscovery) + { + var discovery = device.BayesianDiscoveries.GetOrAdd(roomName, key => CreateProbabilityDiscovery(device, key)); + if (!device.HassAutoDiscovery.Contains(discovery)) + device.HassAutoDiscovery.Add(discovery); + await discovery.Send(mqtt); + } + else if (device.BayesianDiscoveries.TryRemove(roomName, out var staleDiscovery)) + { + device.HassAutoDiscovery.Remove(staleDiscovery); + await staleDiscovery.Delete(mqtt); + changed = true; + }
411-411: Centralize the version constant to avoid hardcoded duplication.
SwVersionis hardcoded as"1.0.0"in this file and also insrc/Models/AutoDiscovery.cs(line 34), with the same version also appearing insrc/Program.cs(line 98). Consider creating a single version constant or leveraging a configuration value to ensure all locations stay synchronized and simplify future version updates.
📜 Review details
Configuration used: Repository 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(4 hunks)src/Models/Config.cs(2 hunks)src/Models/Device.cs(2 hunks)src/Services/DeviceTracker.cs(1 hunks)src/Services/MultiScenarioLocator.cs(5 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/Config.cssrc/Models/AutoDiscovery.cssrc/Services/DeviceTracker.cssrc/config.example.yamlsrc/Services/MultiScenarioLocator.cssrc/Models/Config.Clone.cssrc/Models/Device.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/Config.cssrc/Models/AutoDiscovery.cssrc/Services/DeviceTracker.cssrc/Services/MultiScenarioLocator.cssrc/Models/Config.Clone.cstests/MultiScenarioLocatorTests.cssrc/Models/Device.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
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
🧬 Code graph analysis (6)
src/Models/Config.cs (1)
src/Models/Config.Clone.cs (2)
ConfigBayesianProbabilities(120-131)ConfigBayesianProbabilities(122-130)
src/Services/DeviceTracker.cs (1)
src/Models/Device.cs (1)
ResetBayesianState(152-161)
src/Services/MultiScenarioLocator.cs (3)
src/Models/Config.cs (2)
Config(8-61)ConfigBayesianProbabilities(149-165)src/Models/Device.cs (2)
Device(11-209)Device(23-28)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.Clone.cs (1)
src/Models/Config.cs (1)
ConfigBayesianProbabilities(149-165)
tests/MultiScenarioLocatorTests.cs (2)
src/Models/Room.cs (2)
Room(6-32)ToString(28-31)src/Models/Floor.cs (2)
Floor(7-47)ToString(36-39)
src/Models/Device.cs (2)
src/Services/MultiScenarioLocator.cs (1)
AutoDiscovery(395-420)src/Models/AutoDiscovery.cs (3)
AutoDiscovery(9-156)AutoDiscovery(17-40)AutoDiscovery(42-47)
🪛 GitHub Actions: Build and test
src/Services/MultiScenarioLocator.cs
[error] 228-228: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
src/Models/Config.Clone.cs
[warning] 39-39: CS8601: Possible null reference assignment.
[warning] 40-40: CS8601: Possible null reference assignment.
[warning] 41-41: CS8601: Possible null reference assignment.
[warning] 68-68: CS8601: Possible null reference assignment.
[warning] 15-15: CS8601: Possible null reference assignment.
[warning] 16-16: CS8601: Possible null reference assignment.
[warning] 19-19: CS8601: Possible null reference assignment.
[warning] 20-20: CS8601: Possible null reference assignment.
[warning] 21-21: CS8601: Possible null reference assignment.
[warning] 22-22: CS8601: Possible null reference assignment.
[warning] 23-23: CS8601: Possible null reference assignment.
[warning] 24-24: CS8601: Possible null reference assignment.
🪛 GitHub Actions: Deploy to Docker
src/Services/MultiScenarioLocator.cs
[error] 228-230: CS0173: Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''.
🪛 GitHub Check: build
src/Services/MultiScenarioLocator.cs
[failure] 230-230:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 229-229:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 228-228:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 230-230:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 229-229:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
[failure] 228-228:
Type of conditional expression cannot be determined because there is no implicit conversion between 'double' and ''
src/Models/Config.Clone.cs
[warning] 41-41:
Possible null reference assignment.
[warning] 40-40:
Possible null reference assignment.
[warning] 39-39:
Possible null reference assignment.
[warning] 68-68:
Possible null reference assignment.
⏰ 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 (23)
src/config.example.yaml (1)
53-57: LGTM! Clear configuration structure.The new
bayesian_probabilitiesconfiguration block follows the established YAML structure and provides clear inline documentation for each field.README.md (1)
23-58: Excellent documentation with practical examples.The documentation clearly explains the feature, configuration options, topic structure, and provides a concrete Home Assistant integration example. This will help users understand and implement the feature effectively.
src/Services/DeviceTracker.cs (1)
246-246: LGTM! Proper cleanup on untrack.The call to
ResetBayesianState()is correctly placed in the untrack path after removingHassAutoDiscoveryentries, ensuring complete cleanup of Bayesian-related state when a device stops being tracked.src/Models/Device.cs (2)
85-90: LGTM! Well-designed state management.The concurrent dictionaries with case-insensitive comparers are appropriate for thread-safe room name lookups. The
JsonIgnoreattributes correctly prevent serialization of internal state.
152-161: LGTM! Correct cleanup implementation.The method properly uses
ToList()to avoid collection modification during enumeration, removes discoveries fromHassAutoDiscovery, and clears both dictionaries. Good defensive programming.src/Models/Config.cs (2)
59-60: LGTM! Consistent configuration structure.The property follows the established pattern for configuration sections and is properly initialized.
149-165: Excellent input validation!The
Math.Clampin theDiscoveryThresholdsetter ensures values are always within the valid [0.0, 1.0] range, preventing invalid configuration from causing runtime issues. The defaults are sensible (feature disabled by default, retain enabled).src/Models/AutoDiscovery.cs (1)
122-130: LGTM! Proper Home Assistant discovery extensions.The new properties correctly follow Home Assistant's MQTT discovery specification with snake_case JSON property names and nullable types for optional fields.
tests/MultiScenarioLocatorTests.cs (2)
22-36: LGTM! Clean test helper implementation.The
FixedRoomLocatoruses modern C# primary constructor syntax and provides deterministic scenario setup for reliable testing.
85-193: Excellent test coverage!This comprehensive test verifies all critical aspects of the Bayesian probability feature:
- Probability topic publishing with correct retain flags
- Home Assistant discovery message creation
- Attributes payload structure
- Cleanup behavior with tombstone messages on untrack
The test setup is thorough and assertions are specific, providing strong confidence in the implementation.
src/Models/Config.Clone.cs (3)
19-28: LGTM! Null-conditional operators are used correctly.The static analysis warnings (CS8601) about "Possible null reference assignment" are false positives. The null-conditional operator
?.correctly handles null cases, and the properties have non-null defaults in their declarations. This is the proper pattern for deep cloning nullable nested configuration objects.
39-41: LGTM! Consistent null-safe cloning pattern.The null-conditional operators follow the same correct pattern as the parent
Config.Clonemethod. The static analysis warnings are false positives.Also applies to: 68-68
120-131: LGTM! Proper shallow clone for value types.The
ConfigBayesianProbabilities.Clonemethod correctly implements shallow copying of value-type properties, consistent with other simple configuration Clone methods in the codebase.src/Services/MultiScenarioLocator.cs (10)
7-9: LGTM: New imports support the Bayesian probability features.The added using directives are all utilized in the new functionality.
34-34: LGTM: Epsilon value is appropriate for probability comparison.The value aligns well with the 4-decimal rounding used in probability calculations.
58-62: LGTM: Correctly clears Bayesian state for anchored devices.Anchored devices have fixed locations, so clearing probability outputs is appropriate. The guard condition avoids unnecessary async operations.
165-184: LGTM: Bayesian probability handling is well-structured.The logic correctly gates probability publishing based on configuration and manages state appropriately for both enabled and disabled scenarios.
206-264: LGTM: Conditional location reporting logic is sound.The approach of publishing attributes when either movement or probability changes occur is correct. Gating location data, GPS coordinates, and device history recording based on
bestScenariopresence ensures consistency and avoids publishing incomplete data.
267-320: LGTM: Probability vector construction is comprehensive.The method correctly handles multiple edge cases including inactive scenarios, zero-sum probabilities, normalization, and floating-point precision issues. The case-insensitive dictionary and defensive null/whitespace handling are appropriate.
322-371: LGTM: Probability sensor publishing logic is thorough.The method correctly manages the full lifecycle of probability sensors: creation, updates, discovery management, and cleanup. The use of epsilon for change detection prevents unnecessary MQTT traffic, and the tombstone pattern (null with retain) properly clears stale data.
373-393: LGTM: Probability cleanup is implemented correctly.The method properly clears all Bayesian state using MQTT tombstones (null with retain) and removes Home Assistant discoveries. The use of
ToArray()prevents collection modification exceptions during enumeration.
395-420: LGTM: Home Assistant discovery configuration is well-formed.The discovery record includes all essential fields for proper Home Assistant integration. The state class "measurement" is appropriate for probability values.
422-449: LGTM: Utility methods are well-implemented.All three helper methods are defensive and handle edge cases appropriately:
BuildProbabilityTopicconstructs proper MQTT topic pathsSanitizeSegmentthoroughly sanitizes room names for use in topics/IDsIsSyntheticRoomprovides clear identification of special room namesThe use of
staticis appropriate for these pure utility functions.
DTTerastar
temporarily deployed
to
CI - release environment
GitHub Actions
Inactive
…ve room name matching The test was failing because it expected exact case-sensitive room name matches in the JSON probabilities object. Added case-insensitive lookup and better error messages to show available keys when assertions fail.
DTTerastar
temporarily deployed
to
CI - release environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/MultiScenarioLocatorTests.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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,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:
tests/MultiScenarioLocatorTests.cs
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: Deploy to Docker add-on with tag
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
tests/MultiScenarioLocatorTests.cs (1)
22-36: LGTM! Well-designed test helper.The
FixedRoomLocatorclass provides deterministic room assignment for testing Bayesian probability computation. Using a primary constructor and readonly fields is idiomatic modern C#, and the implementation correctly updates all scenario properties needed for the test.
| using MathNet.Spatial.Euclidean; | ||
| using SQLite; | ||
| using MathNet.Spatial.Euclidean; |
There was a problem hiding this comment.
Remove duplicate import statement.
The using MathNet.Spatial.Euclidean; directive appears on both lines 9 and 11, causing a compilation error or warning.
Apply this diff to remove the duplicate:
using Moq;
using Newtonsoft.Json.Linq;
using MathNet.Spatial.Euclidean;
using SQLite;
-using MathNet.Spatial.Euclidean;📝 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.
| using MathNet.Spatial.Euclidean; | |
| using SQLite; | |
| using MathNet.Spatial.Euclidean; | |
| using MathNet.Spatial.Euclidean; | |
| using SQLite; |
🤖 Prompt for AI Agents
In tests/MultiScenarioLocatorTests.cs around lines 9 to 11, there is a duplicate
using directive for MathNet.Spatial.Euclidean (appearing on lines 9 and 11);
remove the redundant using MathNet.Spatial.Euclidean; line (keep a single
import) so the file has no repeated using directives and then save/compile to
verify the warning/error is resolved.
Summary by CodeRabbit
New Features
Enhancements
Bug Fix
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.