Essentials icon indicating copy to clipboard operation
Essentials copied to clipboard

[FEATURE]-a mechanism to load panel drivers via plugins

Open Rodney-Driscoll opened this issue 3 years ago • 10 comments

Is your feature request related to a problem? Please describe. Having a plugin mechanism for adding different user interface controllers would mean we can make completely custom user interfaces and systems without having to modify Essentials.

Describe the solution you'd like in EssentialsTouchpanelController.SetupPanelDrivers, search for a plugin before the line "if (room is IEssentialsHuddleSpaceRoom)". EssentialsTouchpanelController.SetupPanelDrivers currently loads panelDrivers that are tied to the room, by using plugins then the panel won't be tightly coupled to the drivers and we could have multiple completely different user interfaces in the same room.

Describe alternatives you've considered Touchpanels have the property "defaultRoomKey" which is how touchpanels currently associate with a room, and rooms have a "type" property (e.g. "huddle") which could be used but this restricts to a single touchPanelController per room. Sometimes users want a basic touchpanel and a second advanced one. Another property could be added in order to enable multiple touchpanels with different panelDrivers to be used in a single room (e.g. "touchpanelDriver": "huddle-basic" and "huddle-advanced").

How to handle a floating panel that can be moved from room to room, or control multiple rooms at once? I assume this could be done at run time and is probably for a different feature, but it could tie into this feature.

Additional context Related: [FEATURE]-Build Room Factory Mechanism to Allow Rooms to be Loaded from Plugins #925 This is not on a release branch, however I think room plugins can be done already (not tested) because ContorlSystem.LoadRoom() loads a default device even if it is not a known room type, it just doesn't load a FusionSystemController, I imagine you could load a custom FusionSystemController inside a room plugin.

Rodney-Driscoll avatar May 04 '22 10:05 Rodney-Driscoll

I think the following changes would make this work:

  1. Make the PanelDriverBase class inherit from EssentialsDevice. This would enable Essentials to use the existing mechanisms to load classes that implement PanelDriverBase from a plugin.
  2. Create interfaces for the existing panel drivers. Some of these already exist, namely the IAVDriver and IAVWithVCDriver. but may need to be decomposed to be used more generically.
  3. Currently configuration options for drivers are in the CrestronTouchPanelPropertiesConfig class. It might make sense to make this mechanism similar to the way devices are configured and add a property to the CrestronTouchPanelPropertiesConfig class that is itself an array of DeviceConfig that can be used to feed to the factory methods to build panel drivers.
  4. PanelDriverBase should be modified to keep a collection of children drivers. This way, they won't get accidentally garbage collected if nothing is holding a reference to them.

Not sure that's an exhaustive list, but I think it'd make some steps in the right direction. @ndorin @jtalborough @ngenovese11 any thoughts?

andrew-welker avatar May 05 '22 16:05 andrew-welker

I like ideas 1-3. Not sure what you're getting at for item for as I'm assuming the panel driver would hold a reference to the simpl # pro panel object that would never go away and would also be in the DeviceManager list... but I haven't looked at the code enough to fully grok.

Does the panelDriver get activated as part of the activation phase like anything else?

ngenovese11 avatar May 05 '22 16:05 ngenovese11

As for the changing rooms for a roaming panel, that capability does exist with theSetupPanelDrivers method, as you can feed in a room key to change the room the panel is associated with. This is how we're accomplishing room combining at the moment. Once Essentials can load drivers from a plugin, it shouldn't be difficult to write a driver that can get the room list from the Device Manager and handle the changing of rooms from there.

andrew-welker avatar May 05 '22 16:05 andrew-welker

I like ideas 1-3. Not sure what you're getting at for item for as I'm assuming the panel driver would hold a reference to the simpl # pro panel object that would never go away and would also be in the DeviceManager list... but I haven't looked at the code enough to fully grok.

Does the panelDriver get activated as part of the activation phase like anything else?

At the moment, no. PanelDriverBase doesn't inherit from EssentialsDevice or IKeyed, and they don't get created until the post-activation phase of the Essentials bootstrap process.

andrew-welker avatar May 05 '22 16:05 andrew-welker

Right, so we'd probably want to add it to the device list and run it in the activation phase like everything else?

ngenovese11 avatar May 05 '22 16:05 ngenovese11

Right, so we'd probably want to add it to the device list and run it in the activation phase like everything else?

Assuming of course we make it Inherit Essentials device

ngenovese11 avatar May 05 '22 16:05 ngenovese11

I'm not sure that really works with the paradigm/dependency structure. The panel drivers need to know about the Crestron touch panel, which is only created once the EssentialsTouchpanelController is built. I guess there's no reason that they couldn't be first-class devices in the configuration file and just be created in the normal flow, and then the driver configuration could refer to a EssentialsTouchpanelController to link to...there are other considerations though that make keeping them separate make sense, like being able to change the room that they're controlling and talking too.

andrew-welker avatar May 05 '22 16:05 andrew-welker

I'll be honest I don't know enough about the paradigm to really comment other then simpler is better. If we can get something viable faster within the existing paradigm and skip months of development/breaking changes that would be preferred.

ngenovese11 avatar May 05 '22 16:05 ngenovese11

I assume making a device first class in the configuration would make the configuration files change, I like the idea unless it would break backwards compatibility. I'd err away from anything that breaks backward compatibility. I like the idea of a collection of child drivers, these could be added to the configurable too or only loaded if an associated device is seen in the config, then the touch panels can be more flexible to the system.

Rodney-Driscoll avatar May 05 '22 23:05 Rodney-Driscoll

After some discussion yesterday between myself, @ndorin, and @jtalborough, I think what we've figured out as the best solution is 2 things:

  1. Create a base class that is something like PanelControllerBase. I'll add a proposal for this class later today or tomorrow. The main feature here is that the SetupPanelDrivers method will be an abstract method in the base class with the signature protected void SetupPanelDrivers(string roomKey). This will allow a plugin developer to inherit from this class and get all the benefits of the existing controller logic, along with customizing the behavior of the panel in reaction to button presses.
  2. The configuration will also have a property added, touchPanelType or something similar. This will allow the configuration to be inherited in a plugin and still have the existing configuration values, but also construct the correct type of touch panel device internally.

Thoughts, questions, suggestions?

andrew-welker avatar May 11 '22 15:05 andrew-welker