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

Driver export format and location definitions #92

Closed
pfiaux opened this issue May 31, 2018 · 12 comments
Closed

Driver export format and location definitions #92

pfiaux opened this issue May 31, 2018 · 12 comments

Comments

@pfiaux
Copy link
Contributor

pfiaux commented May 31, 2018

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 format - how in code a driver should be exported
  • Driver export location - where within a package a driver should be exported

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

  • One single file should export the devices array defined above.
  • That file should be linked as the package's main property in the package.json:
{
  ...
  "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:

  • For devices local to workspace
    • devices/${driverName}/index.js
    • devices/index.js
  • For devices installed from npm, available in node_modules
    • node_modues/${moduleName}/devices/index.js

0.50.5 export location example:

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

  • some local drivers in development: localDeviceA, localDeviceB, localDeviceC
  • some external drivers installed via npm: externalDeviceA, externalDeviceB

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.

  • This made combining drivers in to 1 server difficult without writing code
  • This made running multiple servers difficult without managing ports
  • It also couples different concerns: device definition and handling with server

Driver export format

None.

Driver export location

None.

@pfiaux
Copy link
Contributor Author

pfiaux commented May 31, 2018

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.

  • external drivers could use the package.json main property to specify which file exports the devices
    ** it could be devices/ but doesn't have to be leaving the structure up to the developer

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
Copy link

bdauria commented May 31, 2018

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
Copy link
Contributor Author

pfiaux commented May 31, 2018

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:

  • CLI checks the package.json main property and loads devices from that file
  • CLI takes an additional parameter with a path to load devices from (neeo-sdk start devices/index.js)
  • CLI reads the path or paths to load devices from as an array from the package.json neeoSdkOptions
  • CLI reads paths from env variables

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
Copy link
Contributor

nklerk commented May 31, 2018

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.

  • The CLI is delivered with the SDK. but the SDK is included in the driver. a chicken and egg problem arises for me. So do i first need to install the SDK or first the driver.
  • I was trying to structure my driver (https://github.com/nklerk/neeo_driver-kodi/tree/next) i chose to include the full path as described. this will however overwrite files when other drivers are build the same way, so mi issue was how would i keep the package.json unieque while offering a simple solution without difficult structures.
  • NPM introduces a totally different way of adding drivers as modules. and added at least to me a lot to the confusion.

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
Copy link
Contributor Author

pfiaux commented May 31, 2018

Thanks for the feedback @nklerk, good points!

  • Agreed on the CLI vs SDK, we plan to split the CLI from the SDK (avoiding the other problem you mentioned above) in Decouple runtime and tools: brainstorm and roadmap #82
  • The structure of drivers and role of npm needs better documentation from us. If you install the driver via npm it will go to node modules (npm supports github urls as well). For example if I wanted to install your kodi driver i could run npm install --save git+https://github.com/nklerk/neeo_driver-kodi#next. The driver will the be loaded from ./node_modules/neeo_driver-kodi/devices/index.js automatically by the CLI. The advantages being:
    • You don't need to manage the files (download from github, move to correct location)
    • You don't have to worry about other drivers overriding your files since they'll be in separate folders
    • You don't need to run npm install multiple times (instead of going to each folder in devices to run npm install, you can run it once on the root)

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

  • run neeo-cli init driver – asks some question like driver name, device type, do you need discovery, and then generates some starting driver code you can complete

@nklerk
Copy link
Contributor

nklerk commented May 31, 2018

This would be the perfect solution!

@quintstoffers
Copy link

quintstoffers commented May 31, 2018

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:

  • Use" NPM's main property to point to the file drivers are exposed in.
  • Add a "neeo" section to package.json, containing devicesPath or something similar. This would also allow further configuration in the future.

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
Copy link

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
Copy link

Shepless commented Jun 3, 2018

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
Copy link
Contributor Author

pfiaux commented Jun 4, 2018

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:

  • an npm package: npm install neeo-driver-example
  • a git package with public url: git+https://github.com/nklerk/neeo_driver-kodi#next
  • a local package in /my-driver/: npm link ../my-driver

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
Copy link
Contributor

nklerk commented Jun 4, 2018

@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
Copy link
Contributor Author

pfiaux commented Jun 12, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants