Discussion: Consistency

[jnovack]

I'd like to submit a pull-request, and considering other people are as well, I'd like to standardize some conventions.

Here are my ideas and suggestions (to be discussed and applied individually), please discuss:

1. Name of the accessory. "Http" just looks funny, I propose "HTTP" as other projects are doing the same (homebridge-ssh is "SSH"). This is a breaking change.
2. Gettin' crazy with config. Rather than setting many top level variables, make use of objects within the config. This is a breaking change.
3. Google's JavaScript Style Guide. I prefer 4 spaces, but the rest of the world prefers 2. Since you are already using 2 (in most places...), I'm willing to concede this point.

Example for Item 2

  "brightness": {
      "get_url": string,
      "set_url": string,
      "method": string
  }

You can quickly check if (this.brightness) ...

[rudders]

I like it all except for the impact of the breaking changes and there is another contributor @NovaGL who's thought's I'd like also as he and I are discussing a number of changes at the moment. I honestly think we should fork a new HTTP plugin. Homebridge-advanced-HTTP perhaps. And leave this one as a simple one for people to use and fork for specific uses. Perhaps @nfarina might have some thoughts also.

[nfarina]

This plugin works best as sample code in my opinion - simpler is better. A separate, expanded "advanced" HTTP plugin would probably be welcomed by many though.

[NovaGL]

My thought was this, any breakable changes should not be included, since this project has been forked 11 times it would be unfair to them.

My initial thought when adding other services is just like @nfarina said a sample.

I wanted to add all the services but just the required characteristics, that way people could look at it and go, ooh thats how you make a thermostat using Http or that's how I can control my door.

The accepted values for all the services would always be 0 or 1 to avoid confusion.

So I leave it up to you guys I am happy to put it in another advanced plugin, I would like to see how @jnovack would implement point 2.

[jnovack]

To be fair, this is what major release versions are for.

Structure:

{
    "accessory": "HTTP",
    "name": string,
    "service": string,

    "http_method": string-optional,
    "username": string-optional,
    "password": string-optional,
    "sendImmediately": string-optional,

    "switch": {
        "status": url-optional,
        "powerOn": string-or-object,
        "powerOff": {
            url: string,
            body: string
        }
    },

    "brightness": string-or-object,

    "color": {
        "status": url-status,
        "url": url-optional,
        "brightness": boolean,
        "http_method": string-optional
    }
}

Example:

"accessories": [
    {
        "accessory": "HTTP",
        "name": "RGB Led Strip",
        "service": "Light",

        "switch": {
            "status": "http://localhost/api/v1/status",
            "powerOn": "http://localhost/api/v1/on",
            "powerOff": "http://localhost/api/v1/off"
        },

        "color": {
            "status": "http://localhost/api/v1/set",
            "url": "http://localhost/api/v1/set/%s"
        }
    }
]

I have this working right now. https://github.com/jnovack/homebridge-http

And depending on how much I drink this evening, I could get the LockMechanism changes from @NovaGL in too for a full rewrite of the project.

As far as forks go, @TG908 has made the only other progress (other than @NovaGL) with a full re-write for Volume/Channel.

[NovaGL]

I find the word colour ambiguous. It would be best to stick to words that are characteristics look here.

So instead of the would "colour" , I would use one of the following global names

• Brightness
• Hue
• Saturation

I integrated @TG908's volume changes in my other repo, but I don't believe custom characteristics should be added in this repo as it only causes confusion as Siri doesn't understand them.

I am going to be adding sensors soon, so an easy why to monitor smoke alarms and smoke alarms. I think thermostats would be useful too but I don't have one.

While you are doing the rewrite can you change the if (this.service == "Switch") elseif etc to case statements, I think it will be much neater if we want to add many services.

switch (this.service) {
    case "Switch":
        var switchService = new Service.Switch(this.name);

        if (this.switchHandling == "yes") {
            switchService
            .getCharacteristic(Characteristic.On)
            .on('get', this.getPowerState.bind(this))
            .on('set', this.setPowerState.bind(this));
        } else {
            switchService
            .getCharacteristic(Characteristic.On)   
            .on('set', this.setPowerState.bind(this));
        }
        return [switchService];
        break;
    case "Light":
        var lightbulbService = new Service.Lightbulb(this.name);

        if (this.switchHandling == "yes") {
            lightbulbService
            .getCharacteristic(Characteristic.On)
            .on('get', this.getPowerState.bind(this))
            .on('set', this.setPowerState.bind(this));
        } else {
            lightbulbService
            .getCharacteristic(Characteristic.On)   
            .on('set', this.setPowerState.bind(this));
        }

        if (this.brightnessHandling == "yes") {

                lightbulbService
                .addCharacteristic(new Characteristic.Brightness())
                .on('get', this.getBrightness.bind(this))
                .on('set', this.setBrightness.bind(this));
        }

           return [informationService, lightbulbService];
           break;
    case "Door":
        var lockService = new Service.LockMechanism(this.name);

        lockService 
        .getCharacteristic(Characteristic.LockCurrentState)
        .on('get', this.getLockCurrentState.bind(this))
        .on('set', this.setLockCurrentState.bind(this));

        lockService 
        .getCharacteristic(Characteristic.LockTargetState)
        .on('get', this.getLockTargetState.bind(this))
        .on('set', this.setLockTargetState.bind(this));

        return [lockService];
        break;
    case "Smoke":
        var smokeService = new Service.SmokeSensor(this.name);

        smokeService 
        .getCharacteristic(Characteristic.SmokeDetected)
        .on('set', this.getSmokeDetected.bind(this));

      return [smokeService];
      break;
    case "Motion":
        var motionService = new Service.MotionSensor(this.name);

        motionService 
        .getCharacteristic(Characteristic.MotionDetected)
        .on('get', this.getMotionDetected.bind(this));

         return [motionService];
         break;
    } 

[jnovack]

That whole section was from this project, yes, it's on the list, only so many hours in the day to code. I just wanted to prove working. I want to also JSDOC all the functions as well.

But at this point, if we're going to discuss forking the project officially, I feel that you have already taken the "1-800" of names (homebridge-http), and if this is going to be used as a simple, potentially forkable-as-an-example, then I would ask that this project use homebridge-http-simple, and the full-featured project be homebridge-http. Rather than the other way around.

[NovaGL]

Just an update all that code is now committed to my repo. So there is nothing you need to do with it.

Just trying to make the code easier. Also changed "getPowerState" to "getStatusState" so it can be used for more than one service.

Naming the repo is not upto me I would prefer @rudders to have the final say.

[jnovack]

Naming the repo is not upto me I would prefer @rudders to have the final say.

Yes, that's why I was asking him, on his project issues page.

Additionally, package.json has a license of "ISC", but your License file is the Apache license, which license is this software licensed under?

[rudders]

Hi guys, been offline most of the day.

First thoughts,

• Homebridge-http stays as is and we don't break it - purification of names etc is overkill for Homebridge I believe.
• Someone creates homebridge-http-advanced - not precious who!
• open to advice on licence - not precious, I only extracted the module from legacy.

[NovaGL]

Just tell me the address of the new repo when created an I will add to it.

[jnovack]

@rudders Before I officially fork this project and publish, I'd like to nail down the license so there is no issue with separation.

[rudders]

@jnovack - Homebridge has this same licence inconsistency - package.json states ISC and point to his Apache V2 LICENCE file you'll notice and the code was based on legacy so really is an issue for @nfarina to wade in on I believe. Thoughts mate?

[NovaGL]

Closing this, due to to lack of responses.