Is the ICommand interface necessary for display bindings.

[Frankenslag]

I am writing a binding for the Ssd1351 display controller and have recently submitted a PR #442 for the same that is being reviewed by @joperezr and @krwq so far.

My intent was to follow the way that @shaggygi engineered the Ssd1306 binding where an ICommand interface was used and then classes created for each command to be submitted like below

device.SendCommand(new SetContrastABC(0xC8, 0x80, 0xC8));

I have had feedback from @krwq with the opinion that we would be better using just methods and properties to give the following calling convention

device.SetContrastABC(0xC8, 0x80, 0xC8);

I do prefer the second example but as I don't know the reasoning behind using the first way of invoking a command on the device then I would like to open this up for discussion.

What opinions do others have ?

EDIT by @krwq:

• [ ] For 3.0: clarify guidelines
• [ ] Future: refactoring

[shaggygi]

I'm on the fence for this approach. I thought it was better at the time for separation, testing, and possible future simulations. Although, we could get around this when we focus on #231.

My vote is to add the preferred method either in Contributing to Binding or Coding Guidelines sooner than later as it will create inconsistencies or refactoring work.

[shaggygi]

Mcp25xxx is another binding where we used interfaces. See IRegister.

In the case with this binding, there were dozens of registers. Many registers were structured differently making up the bits of a byte that represented the register. It seemed more appropriate to pass as an interface with a method signature of ToByte() so the WriteByte method could easily convert/send to binding via protocol.

I'm open/flexible to ideas to help future bindings.

[krwq]

I'm marking this as 3.0 issue but scope for 3.0 is only clarifying the guidelines and not the refactoring.

[Frankenslag]

@krwq Are you suggesting that I remove ICommands from PR #442 and replace them with direct methods or that I can leave as-is for now and we look at it when guidelines are clearer ?

[joperezr]

@Frankenslag I would suggest you keep them as ICommands for now, and then once we make a decision on the right model we go and change all of the bindings impacted. I'd like to keep consistency if we can, so let's keep consistency for now.

[krwq]

@Frankenslag I'd personally leave it as is and do any refactoring if needed in separate PR after the issue is resolved although we don't have clear guidelines yet so if you've already done/almost finished refactoring locally it's fine to refactor as well (we can revert commit if we decide ICommand is the way to go) - if you haven't started I wouldn't start just yet.

[Frankenslag]

Ok. When I get back from vacation I will attend to the other comments in #442

[joperezr]

In the open PR #575 for Ssd1351 we made some comments about the guidelines for this saying that we should ideally not use the ICommand interface approach for display bindings. Given that this has been communicated now, I will change the milestone of this to Future since the remaining work for this would be to remove the ICommand design from the existing displays.

[Ellerbach]

[Triage] For the small displays and recent added ones like the Nokia screen, we all used the [ICharacterLcd] (https://github.com/dotnet/iot/blob/main/src/devices/CharacterLcd/ICharacterLcd.cs) which brings a lot of value when it comes to displaying cursors, printing text and managing fonts. That's something that should be added for other existing screens.

Now, we should as well align on some basic interface for more advance displays where it's possible to draw a line, a circle, display an image.

This issue is related to #189