NEEOInc / neeo-sdk

NEEO Brain SDK
https://neeoinc.github.io/neeo-sdk/
MIT License
49 stars 17 forks source link

Driver export format and location definitions #92

Closed pfiaux closed 6 years ago

pfiaux commented 6 years ago

As highlighted in #82 and #74 as of the last release we've added new ways to export devices, and to transition we still have some of the old ones.

Goal: a clear definition driver export format and location that covers development and production needs (writing/debugging drivers and running driver via cli, driver manager and ...) for future SDK versions.

Driver export standard

Driver export format

The format from 0.50.x should be flexible enough:

module.exports = {
  devices: [
    deviceA,
    // and so on if multiple devices
  ]
}

Recommendation: 1 device per export/module, unless the devices are variations or similar (v1 & v2 or lite & premium).

Driver export location

{
  ...
  "main": "devices/index.js",
  ...
}

This makes the devices easily accessible via require('neeo-driver-example').devices and more flexible as that location could also be "index.js" or "dist/index.min.js" and so on.

0.50.x export:

Driver export format

Currently as of 0.50.5 with the neeo-sdk cli the expected format of a device driver is:

module.exports = {
  devices: [
    deviceA,
    deviceB,
    // and so on
  ]
}

Regardless of if that driver has one or more devices in it. The CLI then combines all the drivers it finds and loads them in a server.

Driver export location

As of 0.50.5 will be loaded from different locations based on use case. If we consider the neeo-sdk cli as the emerging standard, it will look for devices in the driver export format at these locations:

0.50.5 export location example:

Let's say we're in a work space using:

The local setup looks like this:

devices/
  localDeviceA/
    index.js (exports localDeviceA in the driver export format)
  localDeviceB/
    index.js (exports localDeviceB in the driver export format)
  localDeviceC/
    nonStandardDevice.js (exports localDeviceC but not in index.js)
  index.js (exports localDeviceC in the driver export format, after loading it from nonStandardDevice.js)
node_modules/
  externalDeviceA/
    devices/
      index.js (exports externalDeviceA in the driver export format)
  externalDeviceB/
    devices/
      index.js (exports externalDeviceB in the driver export format)
package.json

This will load all drivers from 3 different paths in different ways and only partially leveraging the NPM package loading sytem.

0.49.x and before export:

Drivers didn't export themselves but started their servers.

Driver export format

None.

Driver export location

None.

pfiaux commented 6 years ago

Some background and thoughts:

Driver export format: I think the 0.50.x format is fine and gives flexibility to add other elements later or to export extra non standard properties if needed.

Driver export location: I think the devices/ folder needs to go. It's imposing structure that doesn't need to be imposed.

So then require('neeo-driver-example') would return the driver export format to it's user (cli or driver manager) which I think would be much cleaner (and flexible). It leaves the local development use case open tho.

Some history bits: It came about because we wanted to make it easy to drop several device drivers in a folder and run them all at once without editing code or ports to make it more user friendly. Then we realized this might make it tricky to install (put the modules in, run npm install in each sub folder) and we could instead load packages from npm directly and that would allow npm install to only be run once. We thought the devices option would be useful for local development, and keeping the same format on the external driver side minimizes the effort switching between development and production.

bdauria commented 6 years ago

Basically, all drivers are loaded "by convention". There is the possibility to prevent drivers from being loaded using the env variable: NEEO_DEVICES_EXCLUDED_DIRECTORIES=["file1", "file2"]. It's also possible to configure the devices folder using NEEO_DEVICES_DIRECTORY="directory".

But I agree with @pfiaux, we should move towards a more defined way of loading devices, but also keep the configuration centralised in the package.json to make it less confusing.

pfiaux commented 6 years ago

Not only by convention but also implicitly, (a driver called my-awesome-driver would not match the neeo- filter and be excluded even if it was a valid driver).

we could move to explicit naming using the package.json (similar to say cordova plugins):

"neeoSdkOptions": {
  "drivers": [
    "neeo-driver-example", // these are the name of npm modules, which are already installed locally
    "neeo-driver-example2"
  ],
}

But this adds some duplicate data since they will also be listed in dependencies.

Also regarding the Driver export location, i'm still wondering how or if we need to handle the local use case. Here's some options I thought of:

It's mainly relevant to the CLI and not necessarily to the driver export standard. Perhaps this is a discussion more specific to the CLI and it's use of neeoSdkOptions, other tools don't necessarily need to use this and should be able to do it differently.

I've updated the description driver export format at the top of the issue. @nklerk @Shepless do you think that setup makes it clear and flexible enough to work with drivers? (both the driver development and running driver sides should be clear now)

nklerk commented 6 years ago

I'll need to read it a couple of times and test some stuff to give a definitive feedback. I can however already comment on the struggles i had in adopting the CLI until now, Although i can code I'm not a developer that needs half the word to understand the bigger picture, the lack of experience will be the same or more so with users that like to fiddle around but have no clue about node, NPS etc.. For then a linear always the same approach is recommended. My struggles can help in finding an easy way for the user.

Now concrete, here are my struggles.

To a user, the most simple solution is:

  1. npm install neeo-sdk
  2. download driver from git
  3. extract driver folder in /drivers/
  4. exec npm start

The SDK would scan reclusively either package.json or more future proof neeodriver.json The json could look something like https://github.com/nklerk/nl.nielsdeklerk.neeo/blob/master/app.json

Sorry for not yet answering the question directly.

pfiaux commented 6 years ago

Thanks for the feedback @nklerk, good points!

Since we didn't do the split yet I'll just call it neeo-cli and outline how it might work for the advanced user who only wants to run drivers:

  1. Install the CLI globally
  2. Go to the folder where you want to install the drivers
  3. run neeo-cli init server – this creates the minimum setup
  4. run neeo-cli add neeo_driver-kodi – this downloads the driver from npm or github urls, extracts it (in node_modules) and installs the dependencies
  5. run neeo-cli start server – this starts with all the drivers that were added

Then the CLI will also have some tools for developers like

nklerk commented 6 years ago

This would be the perfect solution!

quintstoffers commented 6 years ago

I didn't plan on getting involved in the discussions that are taking place here, but seeing the way drivers are currently "forced" to be structured in a certain way (although there are workarounds) I wanted to hop in and give a nudge towards configuration as @bdauria already mentioned earlier.

I think there are two to go forward if we want to expose drivers through config:

Sadly we don't have default/unnamed exports yet, so for now we're forced to name our export. I'm not quite opposed to devices. If there is a "neeo" section in package.json I suppose the devices export name could be configured there, or the path could contain the export name as is commonly used in serverless config, see:

I'm completely in favour of @pfiaux suggestion of how the CLI would be used to add new drivers, as I'm not quite fond of having to clone drivers into a certain directory. I would like to suggest that individual drivers are spawned in separate processes to avoid a bad driver from bringing everything down. Im that case I think it would be wise to look at existing process managers to avoid reinventing the wheel.

carp3-noctem commented 6 years ago

I would like to join this discussion, beside i currently have no time for playing with the new SDK.

I definitly like the Point, that @nklerk mentioned. From side of a User it must be as simple as possible to load a driver to NEEO. Currently it is hard enough to do so because needed to Setup an external Device to run the Code.

But due to this will not be changed within the next Releases, i like the Following way: Let the User Download the NEEO-SDK and install it via NPM, then there need to be one Folder (as it currently is) for all Drivers that are available so no new Installation is required for each driver and no Additional Config needs to be done by enduser.

I know, that Github is not very good when merging Code to one repository, but i also don't like the way to search for each driver and implement it every time. Optimal solution (in my eyes) are: Programmers provide a single folder for an driver sorted the way he likes which can be copied to folder Devices inside the SDK the SDK Scans this folder and awaits a "overhand of the Name and Capability" of the Driver (e.g. Name: Enigma2, Type: Mediaplayer Type: Single / Multi) The Type needs to overhand if the Driver should run as single instance or can be run together with others! Then the SDK start a server instance and loads the driver to the brain(s) or it groups drivers by (max 5 or so) and provide this to the Brain(s) If one Driver crashes the SDK provide a Logfile into an folder (to be send to developer / NEEO) on request only. Automatic restaret of the Driver when it was crashed to provide control of the device when the crash is one time only.

There should not be to much related to the enduser, thereofr a simple as possible concept should be approched.

Later on when a HTTP control of the Brain is officialli supported, this can also lead to loading a Zip file into the Brain which is then extracted and stored under a single folder.

Shepless commented 6 years ago

Apologies @pfiaux @bdauria @nklerk I have been AFK for this week - life and stuff 😄

I think the convention of a package exporting a single driver makes the most sense to me as multi-export comes with big challenges in terms of process management and also installation/maintenance.

I'm not sure I'm fully understanding the discussion around location though? To me, this shouldn't be a problem. If you npm install 10 drivers and they all have a standard "main": "./path-to-entry.js" then this is fine? All you'd need to do it is require() the packages and you're up and running.

However, if we're talking about a use case where you have local and npm packages in the same project, then to me that doesn't make sense. As a developer, if I'm creating a new driver - that driver should be self-contained and not need to depend on another driver. If we want to "combine" drivers to run from a single server - we're moving out of the responsibilities of the SDK to build a driver and into the next "architectural" level - e.g. Driver Manager/CLI Server Start. I hope that makes sense. In short what I am saying is - I can't see a valid reason for a driver package to need a dependency on another driver package. If we treat that as the norm and best practise then the location discussion is redundant as it just leverages standard npm practises.

pfiaux commented 6 years ago

Let's split off the single vs multiple driver discussion to a separate issue #100 because there's other concerns with that change architecture wise. For now we'll keep the devices array which we recommend to only be used with 1 device (plus arrays make for easier functional programming).

After looking over the pros and cons for local vs npm packages here I think we're going to go with the npm setup and stop supporting local drivers directly. We lose the convention of drivers in devices/ but local drivers can still be supported differently and gain in flexibility. It will provide a better experience in all cases, especially when we add some helpers to the CLI.

Summary

Let's say we're in /neeo-sdk-server/, and we want to install packages:

This treats local and npm packages the same way so only 1 setup to support for driver managers and developers and avoids any drivers accidentally depending on another. Also it leaves it up to the driver managers (CLI, driver manager, ...) which drivers to load. In some cases automatically loading all compatible drivers makes sense, in others we might want an explicit whitelist or blacklist but that should be up to the manager code to handle as it wants.

It will also be easier for the advanced user running SDK devices:

mkdir neeo-sdk-server && cd neeo-sdk-server && echo '{}' > package.json
npm install --save neeo-sdk neeo-driver-example
./node_modules/bin/neeo-sdk start

No manual managing of folders, downloading/extracting and installing dependencies. As @Shepless mentioned the best practice would be to leverage npm.

It leaves 1 lose end, as a developer writing a driver how do easily I run the driver I'm currently working on for development. I don't think it needs a special kind of standard defined here, there's currently multiple options and we can add more via the CLI to handle this case.

nklerk commented 6 years ago

@pfiaux I'm fully with you on this. one way of installing and running drivers is the way to go.

For development i would like to keep the option to use MS visual code in debugging mode. I'm not sure how that is called but the feature where the dev can halt code and look inside variables etc. but I guess this is what you meant.

pfiaux commented 6 years ago

We've published updated examples including a a skeleton/minimal driver in the next branch: https://github.com/NEEOInc/neeo-sdk-examples/tree/next/neeo-driver-example

We'll update the readme in the SDK as part of #82 and we have a work in progress CLI which will be in a separate repository with the install documentation.