PepperDash / Essentials

The Essentials Application Framework Libraries
MIT License
133 stars 77 forks source link

[FEATURE]-Define common interfaces for PDUs #183

Closed TrevorPayne closed 1 year ago

TrevorPayne commented 4 years ago

Is your feature request related to a problem? Please describe. There are several types of PDUs in usage - at least two have plugins or support within essentials, and I'm sure more will follow. We need to create a series standard interfaces for implementation of PDUs and a standard join map for bridging of PDUs to ease implementation and integration.

Describe the solution you'd like Default PDU JoinMap and series of default interfaces.

Describe alternatives you've considered The wild west of un-regulated plugin development.

Additional context Specifically seeking input from @jtalborough @jkdevito @jonnyarndt @ngenovese11

jkdevito commented 4 years ago

What features are we supporting? At a minimum I would assume outlet [X] On, Off, Toggle, including feedback. Also how many outlets do we feel should be accounted for within the plugin standard? Do we need more than 20 outlets per PDU, or less?

TrevorPayne commented 4 years ago

I know that the standard Wattbox RackMount PDU has 24 Outlets - there's a few APC ones out there that have 48.

I'm not sure that we need to define a "by Outlet" as we could kind of set up each outlet as it's own device, kind of like what we do with the tesira plugin. We have a device that handles all the communications, and each control is its own device.

I think each outlet should have

Digital: On / On FB Off / Off Fb Toggle

Analog: Status Analog (Good / Fault / Warning, etc) Voltage Amperage

Serial: Name

And at the "holistic device" level

Digital: IsOnline

Analog: Online Status

Serial: Name

So maybe 10 for "Holitic Device Statuses"

And then 5 per outlet.

jonnyarndt commented 4 years ago

Please also include feedback for Power Failure and Device Temperature. Specifically for the APC type. I'm not sure if WattBox give that type of feedback.

TrevorPayne commented 4 years ago

Good ideas

I think I can get temp from a wattbox - though that doesn't matter too much

For Power Failure - would that be for a current fail condition, or reporting that there WAS a fail condition that has since recovered?

jonnyarndt commented 4 years ago

I have a project/consultant request to report APC 'Power Failure'. I agree it's a vague statement. I believe the 'Power Failure' condition would be reporting a previously failed condition that has since recovered.

TrevorPayne commented 4 years ago

If it's a UPS, I could see the need for displaying power failure when it occurs too though.....

I will also say that the APC UPS isn't quite on the roadmap for a plugin - it's SNMP and I've had some issues getting UDP to work appropriately with what I've done so far.

TrevorPayne commented 2 years ago

Ok - reviving this one...

@ngenovese11

I'm imagining a set of interfaces...

IHasPowerOutlets - includes IDeviceInfo and a collection of IOutlets

IOutlet - PowerOn, PowerOff, PowerReset(long timer), Voltage, Load

And a final one that extends IHasPowerOutlets:

IHasBatteryBackupOutlets - adds EstimatedRuntime, BatteryLevel, BatteryHealth, Energized

TrevorPayne commented 2 years ago

Along with this, we may want to create a unified joinMap - though I Imagine @ngenovese11 and I will have some strong disagreements on what that looks like ;)

jtalborough commented 2 years ago

@TrevorPayne See #508. We should be able to include these new interface definitions in that effort.

ndorin commented 2 years ago

Ok - reviving this one...

@ngenovese11

I'm imagining a set of interfaces...

IHasPowerOutlets - includes IDeviceInfo and a collection of IOutlets

IOutlet - PowerOn, PowerOff, PowerReset(long timer), Voltage, Load

And a final one that extends IHasPowerOutlets:

IHasBatteryBackupOutlets - adds EstimatedRuntime, BatteryLevel, BatteryHealth, Energized

I would offer one revision here to decouple IDeviceInfo from IHasPowerOutlets. Seems to me those are logically unrelated and should be implemented in parallel with each other on the target class.

Interface composition is a powerful tool, but we need to be careful to only use it in logically related scenarios. Otherwise we end up making assumptions about a class' capabilities that hurt us down the line.

ndorin commented 2 years ago

Also helps to think about any potential feedback that such an interface could provide and what mechanism we might want to provide it by.

Should IOutlet have an event or any Feedback properties?

TrevorPayne commented 2 years ago

100% agree it should have both - I was trying to just get something down so there would be more discussion.

I really want to move away from feedbacks and more to events for things like this, because I think our medium to long term goals should be focused on folding business logic into essentials - feedbacks are more straighforward, but if we could also just provide an OutletEvent that just has all of the properties of an outlet tracked alongside feedbacks, that's likely ideal.

Additionally, IHasBatteryBackupOutlets should have feedbacks alongside events for the power status of the device holistically.

ndorin commented 2 years ago

Well, Feedbacks have a good use case, even outside trilist operation in that they include an event that can be subscribed to easily if the value is a bool/int/string without the overhead of writing a separate event in the publishing class.

For example, if we’re reporting something like voltage level or battery level remaining, those properties will change independently, so deciding to report them as an int type it makes a lot of sense to do so via an IntFeedback.

However, in the case of a set of properties that will change at the same time, I agree and strongly prefer a single event to notify of the change to the set of properties.

ngenovese11 commented 2 years ago

I think it's important to remember when designing these interfaces the "has a" vs "is a" relationship. IOutlet isn't a "has a" relationship (unless its in the context of IHasOutlets). Outlets have "Power" and "Online" and "Current" and other things... If you want an "Is A" relationship then it should be a base class but beware...

ngenovese11 commented 2 years ago

"Well, Feedbacks have a good use case, even outside trilist operation in that they include an event that can be subscribed to easily if the value is a bool/int/string without the overhead of writing a separate event in the publishing class. For example, if we’re reporting something like voltage level or battery level remaining, those properties will change independently, so deciding to report them as an int type it makes a lot of sense to do so via an IntFeedback. However, in the case of a set of properties that will change at the same time, I agree and strongly prefer a single event to notify of the change to the set of properties. "

Agree I love feedbacks!

TrevorPayne commented 2 years ago

I mean - hmmm...I guess we could go with a base class - utilize some existing interfaces we use for power control - do we treat each outlet as a device (I think we probably should) that can be bridged as part of a IHasOutlets or individually bridged or accessed by another class - so that means more interfaces....

IEnergizedProperties current and voltage, etc, IHasPowerReset for resetting an outlet, and such....

I like more interfaces, TBH.

ndorin commented 2 years ago

In general, there should be an abstract base class for each device type. This allows common logic to be defined and shared between all variations of that device type. The base class can implement some common interfaces that are a requirement.

Then, on actual device instances, more interfaces are implemented as appropriate to describe the specifics of each device.

This allows us to use the advantages of inheriting common logic from an abstract base class, but to treat the instance as the interface types when interacting with it at runtime.

Best practice is to refer to instances of the abstract base class by interface as much as possible, rather than the base class type. This allows two classes that don't inherit from a common class, but do implement the same interface to be treated interchangeably.

psuedocode example:


public class MyAbstractBaseType: IPowerOutlets, IDeviceInfo
{
    public List<IPowerOutlet> PowerOutlets { get; private set; }

    public DeviceInfo DeviceInfo { get; private set; }
}

public interface IPowerOutlets
{
    List<IPowerOutlet> PowerOutlets { get; }
}

public interface IPowerOutlet  : ICurrentDraw, IOutletPowerState, ISomeOtherInterface
{
}

public class MyRealPDUClass: MyAbstractBaseType, IMoreSpecificPowerInterfaceThatOnlyThisDeviceHas
{
}
TrevorPayne commented 1 year ago

This issue has been resolved and these interfaces exist in base essentials and are being used by plugins.

Closing.