Skip to content

merging changes#9

Closed
mplogas wants to merge 45 commits intorefactorfrom
main
Closed

merging changes#9
mplogas wants to merge 45 commits intorefactorfrom
main

Conversation

@mplogas
Copy link
Owner

@mplogas mplogas commented Jul 31, 2025

PR Classification

Enhancement of sensor data handling and precision in the Ecowitt mapping system.

PR Summary

This pull request improves the precision of sensor data handling by updating several methods in the Ecowitt.Controller.Mapping namespace.

  • SensorBuilder.cs: Changed solar radiation sensor from BuildIntSensor to BuildDoubleSensor for better precision.
  • SensorBuilder.cs: Updated BuildCurrentSensor to include an isMilliAmp parameter for unit differentiation.
  • SensorBuilderLogic.cs: Adjusted water flow sensor logic to correctly apply unit conversions based on the isMetric flag.
  • SensorBuilderAddon.cs: Modified wind direction compass sensor to use the result of CalculateWindDirection directly.

mplogas and others added 30 commits September 20, 2024 19:32
…roptions need to be mapped to mqttconfig in statemachine before sending
…, i agree). most of the data now arrives in the state machine and device store.
mplogas and others added 14 commits March 3, 2026 00:03
…scovery topics are now retained. dynamically removing subdevices and sensors need to be implemented
Remove unused code: ConcurrentSensors, GatewayOptions.Passkey, HttpHost.Passkey,
HealthCheck.cs, old/ directory (superseded), in-source docs (moved to docs/).
Fix solarradiation to use BuildDoubleSensor (value can be decimal).
Upgrade both projects to net10.0, add InternalsVisibleTo for test access.

Add 7 test files (150 tests) covering SensorBuilder, SensorBuilder.Addon,
DeNoiserHelper, MqttPathBuilder, MqttPayloadBuilder, DiscoveryBuilder,
and ApiDataExtension. Update docs/api.md, docs/mqtt.md, README.md, CLAUDE.md.
Fix switch discovery: publish HA switch config for WFC01/AC1100/WFC02.
Fix HA reconnect: re-emit discovery on HomeAssistant online status.
Fix gateway HTTP auth: add Basic auth header when credentials configured.
Wire ecowitt.retries config to Polly retry policy.
Remove unused PublishingInterval from config, model, and docs.
Add DiscoveryRemovalEvent to clear retained HA discovery for stale sensors.
Register SubdeviceApiCommand on the message bus so MQTT commands
reach the Dispatcher. Add SendSubdeviceCommand to HttpPublishingService
with the correct gateway payload format (position, always_on, content-type
application/json). Wire the Dispatcher handler to resolve the gateway and
subdevice, default bare HA ON to always-on, and delegate to
HttpPublishingService. Remove old commented-out SendCommand from Dispatcher.

Fix WFC02 battery reporting 5% instead of 100% by using
BuildBatterySensor with the x20 multiplier (5 bars = 100%).
The project was upgraded to net10.0 but the Dockerfile still
referenced dotnet/sdk:8.0 and dotnet/aspnet:8.0, causing the
Docker build CI action to fail.
- Fix null guard in MqttService Subscribe/UnsubscribeHomeAssistant
  that would NRE when _client is null (condition was inverted)
- Replace throw in MQTT callback JSON deserialization with log+return
  to prevent crashing the MQTT client callback path
- Include port in HttpHost.BaseUrl when non-default
- Add explicit WithConsumer<Dispatcher> for GatewayApiData on bus
- Remove .idea/ files from source control (already in .gitignore)
- Reset appsettings.json to generic defaults (no local dev values)
- Fix "Updateing" typo in DeviceStore log message
Trigger on v* tags to produce versioned container images (e.g. v2.0.0
generates 2.0.0, 2.0, 2 tags). Skip push on PR builds. Keep latest
tag for main branch pushes.
Copilot AI review requested due to automatic review settings March 3, 2026 20:04
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes the Ecowitt Controller data pipeline and sensor mapping by introducing an orchestrated, message-bus-driven architecture, improving sensor parsing/precision, and updating documentation/tests to match the new behavior.

Changes:

  • Replaced legacy polling/publishing/discovery services with a Dispatcher + HTTP polling service + MQTT service communicating via SlimMessageBus messages.
  • Reworked sensor model + mapping (typed SensorDataType, stricter parsing, precision improvements like solar radiation as double, new WFC02 fields).
  • Added/updated docs and tests for MQTT topics, HTTP API, discovery payloads, and mapping behavior.

Reviewed changes

Copilot reviewed 82 out of 83 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
src/docker-compose.yml Updates dev bind-mount paths for Mosquitto/Home Assistant.
src/Ecowitt.Controller/mqtt.md Removes old MQTT topic notes (superseded by docs/mqtt.md).
src/Ecowitt.Controller/ha_discovery_payload.md Removes old HA discovery payload examples (superseded by docs/mqtt.md).
src/Ecowitt.Controller/appsettings.json Updates defaults (base topic, polling interval) and trims sample gateway config.
src/Ecowitt.Controller/Subdevice/SubdeviceService.cs Removes legacy subdevice polling background service.
src/Ecowitt.Controller/Store/DataPublishService.cs Removes legacy MQTT publishing service.
src/Ecowitt.Controller/Service/Orchestrator/Dispatcher.cs Adds orchestrator responsible for config emission and event routing.
src/Ecowitt.Controller/Service/Orchestrator/Dispatcher.ConsumerMqtt.cs Handles MQTT lifecycle + HA status events inside Dispatcher.
src/Ecowitt.Controller/Service/Orchestrator/Dispatcher.ConsumerHttp.cs Handles inbound gateway data, subdevice aggregates, and command routing.
src/Ecowitt.Controller/Service/Orchestrator/DeviceStore.cs Moves store into orchestrator namespace and migrates GatewayDevice.
src/Ecowitt.Controller/Service/Orchestrator/DeNoiser.cs Adds tolerance-based change detection to reduce MQTT noise.
src/Ecowitt.Controller/Service/Mqtt/MqttService.cs Introduces MQTTnet-based service with heartbeat + HA integration.
src/Ecowitt.Controller/Service/Mqtt/MqttService.Publisher.cs Publishes gateway/subdevice payloads and availability topics.
src/Ecowitt.Controller/Service/Mqtt/MqttService.Events.cs Adds MQTT client event handling + reconnect logic.
src/Ecowitt.Controller/Service/Mqtt/MqttService.DiscoveryPublisher.cs Publishes Home Assistant discovery configs (retained).
src/Ecowitt.Controller/Service/Mqtt/MqttService.Consumer.cs Consumes bus messages to publish MQTT + handle discovery removals.
src/Ecowitt.Controller/Service/Mqtt/MqttPayloadBuilder.cs Centralizes MQTT payload construction (gateway/subdevice/sensor).
src/Ecowitt.Controller/Service/Mqtt/MqttPathBuilder.cs Replaces old helper with sanitized MQTT topic builder.
src/Ecowitt.Controller/Service/Http/HttpPublishingService.cs Adds HTTP polling + command sender for gateway subdevice API.
src/Ecowitt.Controller/Service/Http/HttpPublishingService.Events.cs Adds polling loop + emits HTTP service lifecycle events.
src/Ecowitt.Controller/Service/Http/HttpPublishingService.Consumer.cs Consumes HttpConfig updates for polling targets/interval.
src/Ecowitt.Controller/Program.cs Rewires SlimMessageBus topology and registers new hosted services.
src/Ecowitt.Controller/Mqtt/MqttService.cs Removes legacy MQTT service implementation.
src/Ecowitt.Controller/Mqtt/MqttClient.cs Removes legacy MQTT client wrapper.
src/Ecowitt.Controller/Model/SubdeviceApiAggregate.cs Removes old aggregate model location (moved under Model/Message/Data).
src/Ecowitt.Controller/Model/Subdevice.cs Adds WFC02 to subdevice model enum.
src/Ecowitt.Controller/Model/Sensor.cs Replaces generic sensor model with a unified Sensor + SensorDataType.
src/Ecowitt.Controller/Model/Message/Event/MqttServiceEvent.cs Adds MQTT service lifecycle event type.
src/Ecowitt.Controller/Model/Message/Event/MqttConnectionEvent.cs Adds MQTT connection event type.
src/Ecowitt.Controller/Model/Message/Event/HttpServiceEvent.cs Adds HTTP service lifecycle event type.
src/Ecowitt.Controller/Model/Message/Event/HomeAssistantStatusEvent.cs Adds HA online/offline status event type.
src/Ecowitt.Controller/Model/Message/Event/HomeAssistantDiscoveryEvent.cs Adds event for triggering discovery publication.
src/Ecowitt.Controller/Model/Message/Event/DiscoveryRemovalEvent.cs Adds event for removing retained discovery entities.
src/Ecowitt.Controller/Model/Message/Data/SubdeviceDataFull.cs Adds “full subdevice” publish message.
src/Ecowitt.Controller/Model/Message/Data/SubdeviceData.cs Adds “changed subdevice sensors” publish message.
src/Ecowitt.Controller/Model/Message/Data/SubdeviceApiAggregate.cs Adds message wrapper for subdevice API polling results.
src/Ecowitt.Controller/Model/Message/Data/DeviceDataFull.cs Adds “full gateway” publish message.
src/Ecowitt.Controller/Model/Message/Data/DeviceData.cs Adds “changed gateway sensors” publish message.
src/Ecowitt.Controller/Model/Message/Config/MqttConfig.cs Adds runtime MQTT configuration message.
src/Ecowitt.Controller/Model/Message/Config/HttpConfig.cs Adds runtime HTTP polling configuration message.
src/Ecowitt.Controller/Model/Mapping/SensorBuilder.cs Improves mapping (solar radiation as double, WFC02 fields, logging).
src/Ecowitt.Controller/Model/Mapping/SensorBuilder.Builder.cs Adds robust parsing + unit conversion helpers + sensor builders.
src/Ecowitt.Controller/Model/Mapping/SensorBuilder.Addon.cs Updates addon calculations to use new Sensor API + compass output.
src/Ecowitt.Controller/Model/Mapping/ApiDataExtension.cs Updates mapping targets (GatewayDevice) and adds debug logging.
src/Ecowitt.Controller/Model/Discovery/Origin.cs Moves discovery models into Model.Discovery namespace.
src/Ecowitt.Controller/Model/Discovery/Enums.cs Moves discovery enums into Model.Discovery namespace.
src/Ecowitt.Controller/Model/Discovery/DiscoveryBuilder.cs Moves/updates discovery builder and bumps origin version string.
src/Ecowitt.Controller/Model/Discovery/Device.cs Moves discovery device model into Model.Discovery.
src/Ecowitt.Controller/Model/Discovery/Config.cs Moves discovery config model into Model.Discovery.
src/Ecowitt.Controller/Model/Discovery/Availability.cs Moves discovery availability model into Model.Discovery.
src/Ecowitt.Controller/Model/Device.cs Renames Gateway domain model to Device and removes concurrent bag.
src/Ecowitt.Controller/Model/Configuration/MqttOptions.cs Moves options into Model.Configuration namespace.
src/Ecowitt.Controller/Model/Configuration/EcowittOptions.cs Moves options into Model.Configuration and trims gateway options.
src/Ecowitt.Controller/Model/Configuration/ControllerOptions.cs Moves options into Model.Configuration and removes publishing interval.
src/Ecowitt.Controller/Model/Api/SubdeviceApiData.cs Moves API DTOs into Model.Api namespace.
src/Ecowitt.Controller/Model/Api/SubdeviceApiCommand.cs Moves API DTOs into Model.Api namespace.
src/Ecowitt.Controller/Model/Api/GatewayApiData.cs Moves API DTOs into Model.Api and adds non-null defaults.
src/Ecowitt.Controller/Mapping/SensorBuilderLogic.cs Removes old mapping logic file (replaced by new builder).
src/Ecowitt.Controller/Ecowitt.Controller.sln Adds solution file inside project directory.
src/Ecowitt.Controller/Ecowitt.Controller.csproj Updates target framework and excludes removed legacy MQTT folder from build.
src/Ecowitt.Controller/Discovery/DiscoveryPublishService.cs Removes legacy discovery publishing service.
src/Ecowitt.Controller/Controller/DataController.cs Updates logging + DTO namespace usage for inbound gateway posts.
src/Ecowitt.Controller/Consumer/DataConsumer.cs Removes legacy consumer replaced by Dispatcher logic.
src/Ecowitt.Controller/Consumer/CommandConsumer.cs Removes legacy command consumer replaced by Dispatcher + HTTP service.
src/Ecowitt.Controller.sln Updates root solution items and groups docs/docker items.
src/EcoWitt.Controller.Tests/SubdeviceMappingTest.cs Adjusts tests for new mapping behavior/sensor counts + namespace moves.
src/EcoWitt.Controller.Tests/SensorMappingTest.cs Updates tests for new unified Sensor model and SensorDataType.
src/EcoWitt.Controller.Tests/SensorBuilderTest.cs Adds comprehensive sensor mapping + parsing tests (including edge tokens).
src/EcoWitt.Controller.Tests/SensorBuilderAddonTest.cs Adds tests for calculated addon values (dewpoint, compass, WFC delta).
src/EcoWitt.Controller.Tests/MqttPayloadBuilderTest.cs Adds tests for MQTT payload output and rounding behavior.
src/EcoWitt.Controller.Tests/MqttPathBuilderTest.cs Adds tests for topic building + sanitization.
src/EcoWitt.Controller.Tests/EcoWitt.Controller.Tests.csproj Updates test target framework.
src/EcoWitt.Controller.Tests/DiscoveryBuilderTest.cs Adds tests for discovery payload builders and device class mapping.
src/EcoWitt.Controller.Tests/DeNoiserTest.cs Adds tests for tolerance-based change detection.
src/EcoWitt.Controller.Tests/ApiDataExtensionTest.cs Adds tests for gateway/subdevice DTO mapping and calculated values.
src/Dockerfile Updates base SDK/runtime images to match new target framework.
docs/mqtt.md Adds consolidated MQTT topic + HA discovery documentation.
docs/api.md Adds HTTP API docs for inbound reporting and outbound gateway polling.
README.md Rewrites README with new architecture/features/docs links and config examples.
CLAUDE.md Adds repository architecture + conventions guidance (tooling/structure).
.gitignore Adds backup/editor artifacts and local tooling directories.
.github/workflows/main.yml Adjusts Docker tagging strategy and avoids pushing on PR builds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +63 to +67
var firstGateway = _deviceStore.GetGatewaysShort().Count == 0;
if (_deviceStore.UpsertGateway(updatedGateway))
{
_logger.LogDebug("gateway added: {Serialize})", JsonSerializer.Serialize(storedGateway));
if (_ecowittOptions is { AutoDiscovery: true, Gateways.Count: 0 } && firstGateway)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

When a new gateway is added, the log statement serializes storedGateway, but storedGateway is known to be null in this branch. This makes the log misleading and likely not what was intended.

Serialize/log updatedGateway (or the value returned from the store) instead.

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +139
if (storedGateway == null)
{
if (_ecowittOptions.AutoDiscovery)
{
_logger.LogWarning("Gateway {Ip} not found while in autodiscovery mode. Not updating subdevices. (Try turning off autodiscovery)", ip);
return;
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Inside the loop over distinct gateway IPs, if a gateway isn't found and autodiscovery is enabled, the code returns from the handler. This exits the whole SubdeviceApiAggregate processing and can skip subdevice updates for other gateway IPs present in the same aggregate.

Use continue (or otherwise scope the early-exit to the current ip) so other gateways in the message are still processed.

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +13
protected override async Task ExecuteAsync(CancellationToken stoppingToken)
{
_logger.LogInformation("Starting SubdeviceService");

using var timer = new PeriodicTimer(TimeSpan.FromSeconds(_config.PollingInterval));
try
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

ExecuteAsync creates a PeriodicTimer using _config.PollingInterval immediately. If HttpConfig arrives after the service starts (likely at startup), the polling loop will run with the default interval and an empty host list, and later config changes won't affect the timer interval.

Consider waiting until a valid HttpConfig has been received before starting the timer, and/or restarting the timer when PollingInterval changes.

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +12
_logger.LogInformation("Starting SubdeviceService");

using var timer = new PeriodicTimer(TimeSpan.FromSeconds(_config.PollingInterval));
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The log message still says "Starting SubdeviceService" even though this is HttpPublishingService, which makes operational logs confusing.

Update the start/stop log messages to match the actual service name.

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +41
catch (OperationCanceledException)
{
_logger.LogInformation("Stopping SubdeviceService");
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The stop log message still says "Stopping SubdeviceService" even though this is HttpPublishingService, which makes operational logs confusing.

Update the stop log message to match the actual service name.

Copilot uses AI. Check for mistakes.
Comment on lines 62 to +74
builder.Services.AddSingleton<IDeviceStore, DeviceStore>();
builder.Services.AddSingleton<MqttService>();
builder.Services.AddSingleton<HttpPublishingService>();
builder.Services.AddSingleton<Dispatcher>();

var sp = builder.Services.BuildServiceProvider();

builder.Services.AddSlimMessageBus(smb =>
{
smb.WithProviderMemory(cfg => { cfg.EnableMessageSerialization = true; });
smb.AddJsonSerializer();
smb.Produce<GatewayApiData>(x => x.DefaultTopic("api-data"));
smb.Produce<SubdeviceApiAggregate>(x => x.DefaultTopic("subdevice-data"));
smb.Produce<SubdeviceApiCommand>(x => x.DefaultTopic("subdevice-command"));
smb.Consume<GatewayApiData>(x => x
.Topic("api-data")
.WithConsumer<DataConsumer>()
);
smb.Consume<SubdeviceApiAggregate>(x => x
.Topic("subdevice-data")
.WithConsumer<DataConsumer>()
);
smb.Consume<SubdeviceApiCommand>(x => x
.Topic("subdevice-command")
.WithConsumer<CommandConsumer>()
);
smb.AddJsonSerializer(jsonSerializerSettings: JsonSettings);
smb.WithDependencyResolver(sp);

Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

builder.Services.BuildServiceProvider() is creating a second service provider and then SlimMessageBus is configured to resolve consumers from that separate sp. This can lead to different singleton instances being used for hosted services vs bus consumers (e.g., the hosted MqttService instance differs from the consumer instance), which can break lifecycle/event handling and state.

Prefer using the application's final IServiceProvider for SlimMessageBus resolution (avoid building a separate provider during registration), so hosted services and bus consumers share the same instances.

Copilot uses AI. Check for mistakes.
Comment on lines +155 to +159
private static readonly JsonSerializerSettings JsonSettings = new()
{
TypeNameHandling = TypeNameHandling.Auto,
NullValueHandling = NullValueHandling.Ignore
};
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

JsonSerializerSettings is configured with TypeNameHandling.Auto. This setting is widely considered unsafe because it enables polymorphic type metadata during deserialization and can allow gadget-based attacks if any untrusted data ever reaches the serializer.

If polymorphic serialization is needed for message bus payloads, consider using an allow-listed binder/known-type approach, or avoid TypeNameHandling entirely and serialize concrete DTOs instead.

Copilot uses AI. Check for mistakes.
``` json
# Ecowitt Controller

A .NET 10 bridge that connects Ecowitt weather stations and IoT subdevices to MQTT, with native Home Assistant auto-discovery.
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

README states this is a ".NET 8 bridge", but the project file and Dockerfile in this PR target .NET 10 (net10.0, mcr.microsoft.com/dotnet/*:10.0). This inconsistency will confuse users and can lead to build/runtime issues if they follow the docs.

Update the README to match the actual target/runtime (or revert the target framework/runtime versions to .NET 8 if that was the intent).

Suggested change
A .NET 10 bridge that connects Ecowitt weather stations and IoT subdevices to MQTT, with native Home Assistant auto-discovery.
A .NET 10 (net10.0) bridge that connects Ecowitt weather stations and IoT subdevices to MQTT, with native Home Assistant auto-discovery.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to +10
using System.Net;
using System.Reflection;
using Ecowitt.Controller.Configuration;
using Ecowitt.Controller.Consumer;
using Ecowitt.Controller.Discovery;
using Ecowitt.Controller.Model;
using Ecowitt.Controller.Mqtt;
using Ecowitt.Controller.Store;
using Ecowitt.Controller.Subdevice;
using Ecowitt.Controller.Model.Api;
using Ecowitt.Controller.Model.Configuration;
using Ecowitt.Controller.Model.Message.Config;
using Ecowitt.Controller.Model.Message.Data;
using Ecowitt.Controller.Model.Message.Event;
using Ecowitt.Controller.Service.Http;
using Ecowitt.Controller.Service.Mqtt;
using Ecowitt.Controller.Service.Orchestrator;
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The PR description focuses on sensor-mapping precision changes, but this diff also includes a substantial architectural refactor (new Dispatcher/HttpPublishingService/MqttService, message bus rewiring, config changes, removal of older services, target framework/runtime changes, docs move). Consider updating the PR description/scope to reflect these broader changes so reviewers can assess impact correctly.

Copilot uses AI. Check for mistakes.
"ip": "192.168.1.101",
"name": "weatherstation-01",
"model": "GW2000A",
"passkey": "...",
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The documented gateway MQTT payload includes a passkey field, which means the Ecowitt gateway passkey is exposed to any client that can subscribe to this topic. That passkey functions as a gateway credential/identifier and leaking it over MQTT (and in downstream consumers or logs) allows other systems or users on the broker to capture and misuse it. Consider removing passkey from the published MQTT payload entirely (and updating MqttPayloadBuilder.BuildGatewayPayload and this documentation accordingly), or replacing it with a non-sensitive identifier that cannot be used to impersonate or reconfigure the gateway.

Suggested change
"passkey": "...",

Copilot uses AI. Check for mistakes.
@mplogas mplogas closed this Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants