michielpost / Q42.HueApi

C# helper library to talk to the Philips Hue bridge
MIT License
411 stars 114 forks source link

Incorrect mac address returned #282

Closed PonchoPowers closed 2 years ago

PonchoPowers commented 2 years ago

Sometimes the LocatedBridge BridgeId is not set correctly.

I'm using the following code:

using HomeSecurity.Hue.Business.Interfaces;
using Microsoft.Extensions.Logging;
using Q42.HueApi;
using Q42.HueApi.Models.Bridge;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

namespace HomeSecurity.Hue.Business
{
    public class HueBridgeDiscovery : IHueBridgeDiscovery
    {
        private ILogger<HueBridgeDiscovery> _logger;

        private static readonly TimeSpan _Timeout = new TimeSpan(0, 0, 30);

        public HueBridgeDiscovery(ILogger<HueBridgeDiscovery> logger)
        {
            _logger = logger;
        }

        public async Task<IEnumerable<LocatedBridge>> FindAsync()
        {
            _logger.LogDebug("Scanning for Hue Bridges.");

            using (var fastLocatorsCancellationToken = new CancellationTokenSource(_Timeout))
            using (var slowNetworkScanCancelCancellationToken = new CancellationTokenSource(_Timeout))
            {
                // Start all tasks in parallel without awaiting

                // Pack all fast locators in an array so we can await them in order
                var fastLocateTasks = new Task<IEnumerable<LocatedBridge>>[] {
                    // N-UPNP is the fastest (only one endpoint to check)
                    new HttpBridgeLocator().LocateBridgesAsync(fastLocatorsCancellationToken.Token),
                    // MDNS is the most reliable for bridge V2
                    new MdnsBridgeLocator().LocateBridgesAsync(fastLocatorsCancellationToken.Token),
                    // SSDP is older but works for bridge V1 & V2
                    new SsdpBridgeLocator().LocateBridgesAsync(fastLocatorsCancellationToken.Token),
                };

                // The network scan locator is the slowest way to discover Hue Bridges.
                var slowNetworkScanTask = (new LocalNetworkScanBridgeLocator()).LocateBridgesAsync(slowNetworkScanCancelCancellationToken.Token);

                IEnumerable<LocatedBridge> result;

                // We will loop through the fast locators and await each one in order.
                foreach (var fastLocateTask in fastLocateTasks)
                {
                    // Await this task to get its result.
                    result = await fastLocateTask;

                    // Check if it contains anything.
                    if (result.Any())
                    {
                        // Cancel all remaining tasks and return.
                        fastLocatorsCancellationToken.Cancel();
                        slowNetworkScanCancelCancellationToken.Cancel();

                        return result.ToList();
                    }
                }

                // All fast locators failed, we wait for the network scan to complete and return whatever we found.
                result = await slowNetworkScanTask;

                return result.ToList();
            }
        }
    }
}

If I run the code a few times in quick succession, the MAC address goes from ecb5fafffe2d12ab to ecb5fa2d12ab.

I haven't debugged yet to see where the problem is coming from, but I wanted to log this as a bug because the issue isn't arising in the code I've written, so I was wondering if this was a known issue or not?

PonchoPowers commented 2 years ago

I am wondering if the FFFE is irrelevant or not, as in do all Hue Bridges contain it and it is relatively meaningless? I will debug later and see if the MAC address (or serial number) as it is called in the code is being returned with the FFFE missing and report back here once I've done so.

spudwebb commented 2 years ago

I have seen the same thing: https://github.com/michielpost/Q42.HueApi/issues/226

PonchoPowers commented 2 years ago

Ah I never spotted that it had been previously reported. I've also just noticed that on the bottom of the bridge that it is only the last 6 characters that matter so this could be a path forwards to substring the value and rename it to serial number or something within the library itself, and then add an explanation as to why, otherwise people will keep coming across this and they will end up wasting time trying to work out what is going on.

PonchoPowers commented 2 years ago

I've created the following extension class, I'm wondering if it will be accepted into the library as a PR?

public static class HueBridgeSerialNumberExtensions
{
    private const int _SERIAL_NUMBER_LENGTH = 6;

    public static string GetSerialNumber(this LocatedBridge locatedBridge)
    {
        if (locatedBridge == null)
        {
            throw new ArgumentNullException(nameof(locatedBridge));
        }
        return GetSerialNumber(locatedBridge.BridgeId);
    }

    public static string GetSerialNumber(this BridgeConfig bridgeConfig)
    {
        if (bridgeConfig == null)
        {
            throw new ArgumentNullException(nameof(bridgeConfig));
        }
        return GetSerialNumber(bridgeConfig.BridgeId);
    }

    private static string GetSerialNumber(string bridgeId)
    {
        if (bridgeId == null)
        {
            throw new ArgumentNullException(nameof(bridgeId));
        }

        if (bridgeId.Length < _SERIAL_NUMBER_LENGTH)
        {
            throw new ArgumentException($"Hue Bridge ID must be at least {_SERIAL_NUMBER_LENGTH} characters in length.");
        }

        return bridgeId[^_SERIAL_NUMBER_LENGTH..];
    }
}

As doing so gives a consistent way to get the bridge serial number rather than the bridge ID, without breaking any existing code.

michielpost commented 2 years ago

For the HttpBridgeLocator, https://discovery.meethue.com is used, it returns an id property that is set as 'BridgeId' For the other locators, when a bridge is found, it is checked by doing a request to http://bridge-ip/description.xml, then the serialNumber field is used to fill BridgeId

The value of these fields does not match and description.xml misses the extra fffe

Getting the BridgeConfig also has the extra fffe for BridgeId V2 API also returns the extra fffe in the 'bridge_id' property

All Hue APIs return the extra fffe. So considering this, I'd think the best way is to treat that as the correct value to work with.

So what would be the best way to solve this? I can see a few options:

These changes would break compatibility. Might not be a huge problem? If it is a problem, we can create a new Id property that always includes the extra fffe

PonchoPowers commented 2 years ago

I think making it consistent by inserting the fffe makes the most sense. I've worked around the issue with my own extension method, so from my perspective it doesn't matter anymore, but future developers might be caught out by this and if this change is made I can update to the latest version and remove the extension methods I've added to my codebase.

michielpost commented 2 years ago

Ok. Fixed in version 3.20.0. Correct BridgeId is now always returned.