search

sharpbrick / powered-up

.NET implementation of the LEGO PoweredUp Protocol
MIT License
98 stars 19 forks source link

Discuss BLE Interface, BLE MAC Address and general Selection Handling #154

Closed tthiery closed 3 years ago

tthiery commented 3 years ago

Other?

@dkurok @vuurbeving @Berdsen

dkurok commented 3 years ago

Maybe a Task<bool>Configure(Dictionary<string , string>) in the IPoweredUpBluetoothAdapter for configuring an adapter if needed (for example for the BlueGiga I have to configure at least the COM-port it is working on; Trace-options or alike can also be configured over that). Can simply retrun true for non-configurable adapters. Should be called very early :-)

dkurok commented 3 years ago

iOS not exposing the MAC-adress of a BLE device: An iOS-adapter has to connect to a filtered (by LEGO-ServiceUUID) LEGO-Hub first and request the primary MAC-address (property 0x=D, which is not changeable according to LWP 3.0, chapter 3.5.1; see also 3.5.5: only upstream); change this into the format needed internally for uniquly identifiying a Hub (ulong actually) and then disconnect (until further calls from the library do a connect). All that in IPoweredUpBluetoothAdapter.Discover()! When IPoweredUpBluetoothAdapter.GetDeviceAsync(ulong bluetoothAddress) is called, it has to look for adverting-packets, connect to the first device it reaches (filtered again by LEGO-service), request the primary MAC-adress and if this matches the bluetoothAddress it can stay with that connection to the device; otherwise go for the next advertising Hub. This is the way Apple works with BLE! Even though the advertising packets have the MAC-address in, Apple's APIs will not expose them; every BLE-device must have a somehow unique identifier. They have their reasons: Under security aspects each device can (and sometimes must) change their MAC-address; some do it regulary on a timer-base. And a using client must have deep information about the device and obtain a still unique identifier (serial-number or alike). The problem is that Apple doesn't give an option here! But the way described above should work, because we can ask the LEGO-Hubs for their "unique" (unchangeable) MAC-address. So this is more a problem of the iOS-implementation of IPoweredUpBluetoothAdapter. Note: The property-request retruns the MAC-address of the LEGO-device in BIG endian; in the advertisment-packets the MAC-address is always little-endian;so be careful about order :-)

tthiery commented 3 years ago

Regards Configuration: Using a Method

Yes. That is the way to go. There is Microsoft.Extensions.Configuration component which is essentially a key/value pair and has adapters to json files, command line parsing etc.

using Microsoft.Extensions.Configuration;

public interface IPoweredUpBluetoothAdapter
{
    // ...
    Task ConfigureAsync(IConfiguration configuration);
    // ...
}

In the CLI or other systems we would then have a situation that ...

// ✅ get your config from somewhere
var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(
        new Dictionary<string, string>
        {
            ["BlueGigaCOMPort"] = "COM4",
            ["BlueGigaTracing"] = bool.TrueString,
        })
       .Build();

// ✅ globally configure the adapter
var adapter = serviceProvider.GetService<IPoweredUpBluetoothAdapter>();
await adapter.ConfigureAsync(configuration); // optionally

// ✅ for each active BLE device/hub to talk to ...
using var scope = serviceProvider.CreateScope(); // use the same adapter
var scopedServiceProvider = scope.ServiceProvider;

// 🕵️‍♀️ that is a fish to grill as well ... but as part of the iOS selection issue.
scopedServiceProvider.GetService<BluetoothKernel>().BluetoothAddress = bluetoothAddress;

// ✅ start using it
var protocol = scopedServiceProvider.GetService<ILegoWirelessProtocol>();

Thumbs Up for this is accepted solution for that problem

tthiery commented 3 years ago

Regards Configuration: Using the Options Pattern

No interface change, only implementations need to add a Option to the constructor

class BlueGigaAdapter
{
    public BlueGigaAdapter(IOption<BlueGigaOption> option)
   // ...
}

Invocation is less service provider style (anti-pattern) and more DI like.

// ✅ get your config from somewhere
var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(
        new Dictionary<string, string>
        {
            ["BlueGigaCOMPort"] = "COM4",
            ["BlueGigaTracing"] = bool.TrueString,
        })
       .Build();

// ✅ Init Service Provider including config
var serviceProvider = new ServicesCollection()
    .AddPoweredUp()
    .AddBlueGigaBLE()
    .Configure<BlueGigaOptions>(configuration)
    .Build()

// ✅ for each active BLE device/hub to talk to ...
using var scope = serviceProvider.CreateScope(); // use the same adapter
var scopedServiceProvider = scope.ServiceProvider;

// 🕵️‍♀️ that is a fish to grill as well ... but as part of the iOS selection issue.
scopedServiceProvider.GetService<BluetoothKernel>().BluetoothAddress = bluetoothAddress;

// ✅ start using it
var protocol = scopedServiceProvider.GetService<ILegoWirelessProtocol>();

Thumbs Up for this. Personally I like this more.

dkurok commented 3 years ago

Regards

Primary interface on the BLE abstractions (ulong vs ?)

What is about an abstraction of this like a BLEIdentifier, which can be constructed by a MAC-Address (little or big-endian over a bool-property) or a ulong or a String like "90:84.2B:4C:FF:F0" (or other format like . or - instead of ":") and then internally use a ulong? Also the byte[6] array could be used (but makes little more effort, because byte[] doesn't implement IEquatable or IEqualityComparer) - the class itself must be usable as a TKey in IDictionary<TKey, TValue> (e.g. for Dictionaries based on the BLEIdentifier). Even better would be an internal Hash derived from the value given to the constructor. This would for example also allow to use the Advertising Name (property 0x01 of LWP) as an identifier and combined with a property KindOfIdentifier being Address, PrimaryMACAdressProperty or FriendlyName (extensible if wanted; but I don't see no more unique property in LWP) the IPoweredUpBluetoothAdapter.GetDeviceAsync(BLEIdentifier) could decide its way for detecting / connecting to the device. Not fully thought on all aspects; just an idea...

tthiery commented 3 years ago

No idea whether it is feasible on iOS but the idea sound good. Would fix all problems at once.

The class PoweredUpBluetoothDeviceInfo is already half of it. Maybe we can create a IPoweredUpBluetoothDeviceInfo which has multiple implementations (one for iOS, one others). That would circle well between IPoweredUpBluetoothAdapter.Discover callback and IPoweredUpBluetoothAdapter.GetDeviceAsync.

For the cases we have an externally provided fixed address, we could construct these explicitly. That explicit construction, I have no idea how it would work on iOS.

dkurok commented 3 years ago

As said before: Apple has its very own way to deal with BLE. But also with the idea above, the iOS-implementation must do its own way when using MAC-adress; but the idea doesn't make this harder... BTW: When using FriendlyName, this can also be handled during advertisment. The Lego-Hub sends this during advertisment. Could be very interesting for someone with many Hubs (trains for example; user can name their Hubs according to locomotive ("BR-51", "Crocodile" or alike))

dkurok commented 3 years ago

Regards Configuration: Using the Options Pattern I like this more becasue its nearer to the way it works in Blazor / ASP.NET Core by using DI.

Berdsen commented 3 years ago

@dkurok is right about Apple's way to deal the mac and ble. But do we really require a Bluetooth MacAddress?

Perhaps when we're discovering devices in the BluetoothAdapter discovery we construct the PoweredUpBluetoothDeviceInfo where we assign an identifier and perhaps an BT stack depenendent object.

When then GetDeviceAsync() is called, we just give in this construct, so that the current bt adapter has to handle this by its own implementation:

public async Task<IPoweredUpBluetoothDevice> GetDeviceAsync(PoweredUpBluetoothDeviceInfo deviceInfo)
{
    var device = deviceInfo.NativeDevice as IDevice; // Xamarin
    // var device = deviceInfo.NativeDevice as BluetoothLEDevice // WinRT
}

The bad thing here would be, that a direct connection via known BT MacAddress would not work this way, so this must be handled on a different way.

tthiery commented 3 years ago

That is exactly my thought. Therefore the idea of the interface. The Mac address can then be a detail only a few implentations will expose. iOS object will then carry whatever information.

tthiery commented 3 years ago

Primary Device Identity Interface

Since iOS is abstracting the identity of the BLE device, each stack should expose its own identity object. Therefore I propose the following changes

Addition of a new interface

public interface IPoweredUpBluetoothDeviceInfo
{
    byte[] ManufacturerData { get; }
    string Name { get; set; }
}
public interface IPoweredUpMacAddress
{
    byte[] MacAddress { get; }
}

public class WinRTDeviceInfo : IPoweredUpBluetoothDeviceInfo, IPoweredUpMacAddress { }
public class XamarinOnIOSDeviceInfo : IPoweredUpBluetoothDeviceInfo { }
public class XamarinOnAndroidDeviceInfo : IPoweredUpBluetoothDeviceInfo, IPoweredUpMacAddress { }
public class BlueGigaDeviceInfo : IPoweredUpBluetoothDeviceInfo, IPoweredUpMacAddress { }

Change to the existing Adapter interface

Use the new interface

public interface IPoweredUpBluetoothAdapter
{
    void Discover(Func<IPoweredUpBluetoothDeviceInfo, Task> discoveryHandler, CancellationToken cancellationToken = default);

    Task<IPoweredUpBluetoothDevice> GetDeviceAsync(IPoweredUpBluetoothDeviceInfo deviceInfo);
}

public class BluetoothKernel
{
    // ...
    // remove: public ulong BluetoothAddress { get; set; }
    public IPoweredUpBluetoothDevice BluetoothDeviceInfo { get; set; }
}

Finally Change all the interfaces using the ulong bluetooth addresses.

I like the change. Is that working for team iOS? Must be fixed in v4 timeline. Thumbs Up?

dkurok commented 3 years ago

regards Primary Device Identity Interface: Why do you want to split it into two interfaces here? The byte[] MacAddress could go directly into IPoweredUpBluetoothDeviceInfo IMHO. At some point in implementation EVERY BLE-stack will need it to uniquly identify a LWP-device/Hub by its MAC-address (getting it over advertisment or in case of iOS over connect & request property 0x0D) , because the LWP-Hubs don't have any other really unique value (serial# or alike) actually. And beeing prepared that something like that may change (so some other kind of unique identifier inside the hubs; may come with firmware-updates) we can take any object instead of byte[] as long as it is unique. Alternative Suggestion:

public interface IPoweredUpBluetoothDeviceInfo: IEquatable<IPoweredUpBluetoothDeviceInfo>
{
    byte[] ManufacturerData { get; }
    string Name { get; set; }
    object UniqueIdentifier { get; }

    override bool Equals(IPoweredUpBluetoothDeviceInfo obj)
    {
        if (obj == null || GetType() != obj.GetType())
        {
            return false;
        }
        //here there may be some change needed depending on what concrete type is used in the real implementation
        //byte[] for example must override it, because byte[] doensn't adhere to IComparable/IEquatable
        if (this.UniqueIdentifier.Equals(obj.UniqueIdentifier))
        {
            return true;
        }
    }

    // override object.GetHashCode
    public override int GetHashCode()
    {
        return UniqueIdentifier.GetHashCode();
    }  
}

Rest is nearly identical to above suggestion from @tthiery (just leave out the IPoweredUpMacAddress)

rickjansen-dev commented 3 years ago

I do agree with you on making the thing IEquatable, mainly for dictionary support. But that's only if we step away from using ulong as the unique identifier of course (which is probably a good thing).

I'm not fond of it being an object maybe, it seems to generic/broad in a definition. You really need to be aware that whatever you are using as the unique identifier should be properly implementing equals and gethashcode (but that might be a very reasonable assumption/choice though). You are simply relaying the uniqeness from the interface to the object. You could have a dictionary with the uniqueidentifier as Tkey instead everywhere, might not be as pretty, but it would also work.

Just a suggestion: maybe we can replace object by a UniqueIdentifer class that contains methods to convert from (and to) various mac address representations (eg FromMacAddressLong(), FromMacAddressString()) and ios device info (FromIOSDeviceRepresentation()). If and only if we can agree on a common storage for a unique identifier (mac address or otherwise), including iOS. From and To methods could maybe be extension methods so they can reside in specific implementation binaries/namespaces.

Berdsen commented 3 years ago

Just a small update. I finally made a working setup to be able to deploy to some Apple HW. It's really tough without having an own mac or an apple developer account but I'm finally able to deploy to a real device. The next days I would try to look into what's happening with the Xamarin BLE adapter for iOS and perhaps make it in the first step like it is done in sharpbrick/example-xamarin#1 and commit it also to this PR.

tthiery commented 3 years ago

Hi Friends ... can you have a look at the PR #173 I created. It is the implementation of the Device Identification Abstraction.

Further, there is a detail about Mac Address stringification where I need your thought power. I am not sure if all our adapters have they ulong mac address we used so far in the same way (regards byte order etc). Also not regards little and big endianess.

@Berdsen @vuurbeving @dkurok