Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First draft for new ComponentInformation support #1965

Merged
merged 24 commits into from May 18, 2023

Conversation

pgrawehr
Copy link
Contributor

@pgrawehr pgrawehr commented Oct 23, 2022

Concept for first part of #1962

All important classes (GpioDriver, GpiController, I2cDriver, SpiDriver etc.) now contain a method QueryComponentInformation that return an instance of ComponentInformation. This class represents a tree of the active drivers and/or bindings. Further extension will allow to provide possible drivers as well.

[Further extension: It is intended that later ComponentInformation can be provided to a factory that constructs a device instance from such an information tree. One problem there is however, how one would know what is available. Providing a class that knows all possible drivers is quite ugly, and even instantiating a driver without initializing it (e.g. just to find out it's capabilities or whether it would work) is currently not possible without relying on exceptions. Separating the constructor from the initialization could improve this. ]

Example

This code:

using Board b = Board.Create();
using ic2 = b.CreateOrGetI2cBus(1);
Console.WriteLine("Hardware detected: ");
Console.WriteLine(b.QueryComponentInformation(true));

Generates this output on a Raspberry Pi 4:

Hardware detected: 
{Iot.Device.Board.RaspberryPiBoard} Raspberry Pi
 └{Iot.Device.Board.ManagedGpioController} Managed GPIO Controller
  └{System.Device.Gpio.Drivers.RaspberryPi3Driver} Generic Raspberry Pi Wrapper driver
   └{System.Device.Gpio.Drivers.RaspberryPi3LinuxDriver} Raspberry Pi 4 Model B Rev 1.2 linux driver with 28 pins and an interrupt driver
    └{System.Device.Gpio.Drivers.LibGpiodDriver} Gpio Driver
 └{Iot.Device.Board.I2cBusManager} I2C Bus Manager, Bus number 1

Microsoft Reviewers: Open in CodeFlow

@pgrawehr pgrawehr added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins labels Oct 23, 2022
@raffaeler
Copy link
Contributor

With reagards to the output, it would be certainly useful to have both the tree (as an object graph), but also a string to uniquely identify the 'Component'.
In a first scenario I may disergard entirely which board I am using, but I do need libgpiod under the hood.
In an opposite scenario, I really need the RPi4 and I have to make sure the correct board was selected.

Finally I would split the driver and the board information in two different levels of the tree, so that comparing the information would be easier.

@pgrawehr
Copy link
Contributor Author

@raffaeler Looks like we have a similar understanding. Look at the code, I think it exactly has what you're asking for. Each component is currently identified by it's type name.

Copy link
Contributor

@raffaeler raffaeler left a comment

Choose a reason for hiding this comment

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

Sorry for not reading the code before, but I didn't have time.
Love the general structure. I added a couple of comments even if it is still very early for a deep review.
I am thinking whether or not it could be possible to go in much detail about the "capabilities" so that it is easier to take decisions based on that instead of a raw string with the board model.

private readonly List<ComponentInformation> _subComponents;

public ComponentInformation(object instance, string name)
: this(instance, name, string.Empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a record, why not using the primary ctor and optional parameters with a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer explicit overloads, but that's just syntax and can be adjustedat the end.

@krwq krwq self-assigned this Oct 27, 2022
@joperezr joperezr added this to the vNext milestone Dec 1, 2022
@joperezr joperezr added the Priority:0 Work that we can't release without label Dec 8, 2022
@krwq krwq removed this from the vNext milestone Dec 15, 2022
@krwq krwq added Priority:1 Work that is critical for the release, but we could probably ship without and removed Priority:0 Work that we can't release without labels Dec 15, 2022
Make interface IQueryComponentInformation internal for now (since not used for calling),
make ComponentInformation read-only, remove argument from QueryComponentInformation.
@Ellerbach
Copy link
Member

I like the idea. Now, in terms of implementation, that will require to add the IQueryComponentInformation and the implementation in all Boards and associated drivers, correct?

@raffaeler
Copy link
Contributor

I like the idea. Now, in terms of implementation, that will require to add the IQueryComponentInformation and the implementation in all Boards and associated drivers, correct?

Given that the abstract driver class implements the interface, the answer is yes.
But there is more. If I understood correctly, in your upcoming FTH binding, you should also have the opportunity to provide the IQueryComponentInformation there.

@pgrawehr
Copy link
Contributor Author

I like the idea. Now, in terms of implementation, that will require to add the IQueryComponentInformation and the implementation in all Boards and associated drivers, correct?

Given that the abstract driver class implements the interface, the answer is yes. But there is more. If I understood correctly, in your upcoming FTH binding, you should also have the opportunity to provide the IQueryComponentInformation there.

I'm actually not sure yet we really need the interface itself (just yet). We will have the method in several abstract base classes, but since we don't need something like a "list of all objects which can be queried" the interface itself is probably not needed. But this won't change the overall concept.

@raffaeler
Copy link
Contributor

@pgrawehr Not sure to understand what exactly is your current design.
Could you please describe it? (maybe the issue is a better place)

@pgrawehr
Copy link
Contributor Author

I'm trying out some refactoring, to be able to check the design for the next stage. This will be separated from this PR.

@pgrawehr
Copy link
Contributor Author

pgrawehr commented Feb 4, 2023

So, I think the concept should hold now. This is now waiting for PR #2019 to avoid conflicts.
Also, I haven't really done much testing yet.

}

return ci;
}
Copy link
Member

Choose a reason for hiding this comment

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

Possibly
string ToString() { return $"{{ {string.Join(Subcomponents.Select(...))} }}"; }

string ToString() { return $"{{ {_interruptDriver}, ... }}"; }

/// This event is raised when <see cref="QueryComponentInformation"/> of the base class is called. This is can be used for bindings
/// to intercept that call on their own I2C device, so they can add additional information
/// </summary>
public event Action<ComponentInformation>? OnQueryComponentInformation;
Copy link
Member

Choose a reason for hiding this comment

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

This should be just callback rather than event especially given 1:1 relationship with device. Why not Func<ComponentInformation> instead?

Copy link
Member

@krwq krwq Feb 27, 2023

Choose a reason for hiding this comment

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

ok my question in the end got answered by the code below. First sentence is still valid though

Copy link
Member

Choose a reason for hiding this comment

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

btw I like this approach much more than inheritance initially proposed but I'll suggest to do only printing in the initial PR and in the following one we can focus on the approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you mean to remove this for now?

/// <inheritdoc />
public override ComponentInformation QueryComponentInformation()
{
return new ComponentInformation(this, "Pca9685 PWM Channel");
Copy link
Member

Choose a reason for hiding this comment

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

I'll suggest to move this to base class and just use type name given spaces do not add virtually any new information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, however this merely serves as a place holder and, once the design is finalized, all the components should be reviewed and additional information added, if useful.

@pgrawehr
Copy link
Contributor Author

I had a look at ExpandoObject and DynamicObject but neither of them are really helpful here. They allow a lot of dynamic type construction, but have several drawbacks, too:

  • For netstandard, a reference to "Microsoft.CSharp" is necessary, which doesn't seem very lightweight
  • QueryComponentInformation would need to return dynamic, which is an untyped thing, leaving the caller confused about what he can do with the object. We don't want runtime exceptions trying to get debug information.
  • One can define new properties on ExpandoObject, but not methods. It doesn't seem possible to override ToString() as well, which we need. ExpandoObject is sealed, so deriving from it is also not possible. DynamicObject allows deriving, but still requires users to use dynamic all over the place.
  • In the next release, we want to be able to construct a driver tree from an user-defined ComponentInformation tree. We can't expect people to construct correct instances of ExpandoObject. This would also be a problem for us, as eventually all bindings would create ComponentInformation instances, and we want them to be (at least somewhat) comparable and consistent.

@raffaeler
Copy link
Contributor

  • For netstandard, a reference to "Microsoft.CSharp" is necessary, which doesn't seem very lightweight

This means just .NET Framework and Mono. I won't be too worried for those.

  • QueryComponentInformation would need to return dynamic, which is an untyped thing, leaving the caller confused about what he can do with the object. We don't want runtime exceptions trying to get debug information.

That's by design, if we want to get rid of a type.

  • One can define new properties on ExpandoObject, but not methods. It doesn't seem possible to override ToString() as well, which we need. ExpandoObject is sealed, so deriving from it is also not possible. DynamicObject allows deriving, but still requires users to use dynamic all over the place.

You are right, but this can be resolved with some extension methods.

  • In the next release, we want to be able to construct a driver tree from an user-defined ComponentInformation tree. We can't expect people to construct correct instances of ExpandoObject. This would also be a problem for us, as eventually all bindings would create ComponentInformation instances, and we want them to be (at least somewhat) comparable and consistent.

Let's discuss this. I understand @krwq being worried to introduce a new type which is basically everywhere.
The alternative solution we are discussing here is to minimize the impact by using dynamic + ExpandoObject and then a class defining the extension methods needed to make the dev experience better.
I am not completely against ComponentInformation, but the evaluation should be a fair comparison between the two solutions.

@raffaeler
Copy link
Contributor

BTW, even if I prefer the extension methods approach, this code using Expand includes the support to methods:

dynamic GetData()
{
    dynamic d = new ExpandoObject();
    d.Name = "abc";
    d.Count = 10;
    d.ToString = (Func<string>)(() =>
    {
        return $"Name:{d.Name} Count:{d.Count}";
    });
    return d;
}

dynamic x = GetData();
Console.WriteLine(x.ToString());
// output: 
// Name:abc Count:10

@pgrawehr pgrawehr requested a review from krwq April 3, 2023 17:00
@joperezr
Copy link
Member

joperezr commented Apr 6, 2023

@krwq looks like @pgrawehr has addressed your comments and this is ready to go. Can you take one final look and merge this one?

@joperezr joperezr added this to the v3.0.0 milestone Apr 13, 2023
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Looks good, if you can add the rest of the DeviceInformation property on the FT devices, it would be perfect!

{
var self = new ComponentInformation(this, "Ftx232HI2c I2C Bus driver");
self.Properties["BusNo"] = "0";
foreach (var device in _usedAddresses)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add all the properties of the DeviceInformation ?

/// <inheritdoc />
public override ComponentInformation QueryComponentInformation()
{
return base.QueryComponentInformation() with { Description = "Ftx232 Device" };
Copy link
Member

Choose a reason for hiding this comment

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

Can you add all the properties of the DeviceInformation ?

self.Properties["BusNo"] = "0";
self.Properties["Description"] = DeviceInformation.Description;
self.Properties["SerialNumber"] = DeviceInformation.SerialNumber;
foreach (var device in _usedAddresses)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add all the properties of the DeviceInformation ?

@pgrawehr pgrawehr merged commit 2460cd3 into dotnet:main May 18, 2023
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants