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

[PLUGIN DEVELOPMENT] Centralising HTTP plugins #2160

Closed
phenotypic opened this issue Feb 20, 2019 · 23 comments
Closed

[PLUGIN DEVELOPMENT] Centralising HTTP plugins #2160

phenotypic opened this issue Feb 20, 2019 · 23 comments

Comments

@phenotypic
Copy link

Hi there,

As the title suggests, I am currently in the process of working on some 'HTTP type' plugins for homebridge. More specifically, I've recently done a fair bit of work on the homebridge-thermostat plugin attempting to streamline it. However, I've run into an issue which stretches a bit further than my JavaScipt knowledge, despite all my attempts!

Here is a snippet of example code taken from the original index.js:

  _httpRequest: function (url, body, method, callback) {
      request({
          url: url,
          body: body,
          method: this.http_method,
          timeout: this.timeout,
          rejectUnauthorized: false,
          auth: this.auth
      },
          function (error, response, body) {
              callback(error, response, body);
          });
  },
  
  getTargetTemperature: function(callback) {
    this.log("[+] getTargetTemperature from:", this.apiroute+"/status");
    var url = this.apiroute+"/status";
    this._httpRequest(url, '', 'GET', function (error, response, responseBody) {
        if (error) {
          this.log("[!] Error getting targetTemperature: %s", error.message);
  	  callback(error);
        } else {
  	  var json = JSON.parse(responseBody);
  	  this.targetTemperature = parseFloat(json.targetTemperature);
  	  this.log("[*] targetTemperature: %s", this.targetTemperature);
  	  callback(null, this.targetTemperature);
        }
    }.bind(this));
  },

  setTargetTemperature: function(value, callback) {
    this.log("[+] setTargetTemperature from %s to %s", this.targetTemperature, value);
    var url = this.apiroute+"/targetTemperature/"+value;
    this._httpRequest(url, '', 'GET', function (error, response, responseBody) {
        if (error) {
          this.log("[!] Error setting targetTemperature", error.message);
  	  callback(error);
        } else {
          this.log("[*] Sucessfully set targetTemperature to %s", value);
  	  callback();
        }
    }.bind(this));
  },

As you can see, for the sake of simplicity, I have removed numerous get and set functions but have kept two (getTargetTemperature and setTargetTemperature) for the sake of illustrating my point.

To summarise the what happens with the Thermostat: When the state is refreshed (by opening the Home app), numerous (4-6) get functions are called, all of which contact the HTTP device simultaneously and attempt to retrieve the same JSON (containing data like the target temperature, current state etc.), they then proceed to extract just 1 field each; meaning that whilst the JSON only needs to be retrieved once, it is retrieved multiple times, which is extremely wasteful and can cause Homebridge to hang while this takes place. It is also worth noting that the set functions are not an issue because they are only called individually once you update a field like the target temperature.

I have tried seeking a solution to the issue. My first thought was creating a centralised function called something like _getStatuses which would be called by each of the get functions. If it was being called for the first time in a while, it would request the JSON once, parse each of the fields and store them ready for the other incoming requests, reducing the number of requests greatly.

However, the problem is that I have tried multiple approaches to this problem and none of them have worked to a satisfactory level for a variety of reasons, including the fact that JavaScript is asynchronous and makes it hard to work step-by-step. Thus, I am hoping that someone may have an idea or solution which will solve the problem. I would be really grateful if someone could provide an idea, some sample code or a pre-existing plugin which relates to the issue.

Thank you in advance for your help,

Kind regards, Tom

@ebaauw
Copy link
Contributor

ebaauw commented Feb 20, 2019

You’ll want to implement polling the http server from your plugin, calling Characteristic.updateValue() when the corresponding value has changed. Don’t use a handler for the ”get” event and let homebridge return the last value from cache. This has added advantage that changes will be visible in HomeKit, without having to issue a refresh from the Home app.

@phenotypic
Copy link
Author

phenotypic commented Feb 20, 2019

@ebaauw Thank you for your reply, that sounds like a good solution. Would you be able to link a plug-in which makes use of this characteristic, or attach some sample code showing how to implement it? Best, Tom

@ebaauw
Copy link
Contributor

ebaauw commented Feb 20, 2019

All my plugins use Characteristic.updateValue() instead of Characteristic.on('get', ...).

homebridge-hue and homebridge-music setup a heartbeat for polling (periodically querying the device). homebridge-zp and homebridge-p1 use notifications from the device, so they don't need to implement polling.

@phenotypic
Copy link
Author

phenotypic commented Feb 20, 2019

Ah I see, I don't understand how that would help with the issue of 'wasteful' HTTP requests though, because surely you would still need to individually get each of the values from each function. Do you mean that the plugin stores these values from the polling so they're not all requested once when you open the app? I had a look at some of your plugins but found them rather hard to use as a template due to their comparative complicity; would you be able to expand a bit more on Characteristic.updateValue() and how you propose this might fix the 'issue'? Thanks again!

@ebaauw
Copy link
Contributor

ebaauw commented Feb 20, 2019

because surely you would still need to individually get each of the values from each function

No - you (or rather: homebridge) simply return the value from cache (memory), without doing any HTTP GET at that moment. Periodically, you do one HTTP GET and update all characteristic values from the single JSON response, causing homebridge to update its cache and send notifications to each connected HomeKit client.

Do you mean that the plugin stores these values from the polling so they're not all requested once when you open the app?

Yes, so they're not requested (from the HTTP server) at all, when the app requests them. Actually homebridge already stores the value, when you call updateValue(). I still keep a copy in my plugin to detect changes, calling updateValue() only when the value has actually changed.

@phenotypic
Copy link
Author

Periodically, you do one HTTP GET and update all characteristic values from the single JSON response

Wow, that sounds great! Would you be able to provide an example snippet of code (like the one I attached above) which would do this, as I'm slightly struggling to understand how exactly to implement it to take all the values from one get request.

The solution as a whole sounds very interesting and I'm hopeful that if I'm able to get a grasp of it, its something I could implement into some other plugins.

@NorthernMan54
Copy link
Contributor

@Tommrodrigues Take a look at this plugin

https://github.com/NorthernMan54/homebridge-bme280

It is a simple homekit accessory to read a loca lBME280 Temperature sensor, and display it in the Home App. It does not implement the .on('GET' interface as it uses a polling loop to read the sensor every 60 seconds and update Homebridge with the results.

@phenotypic
Copy link
Author

@NorthernMan54 Thanks, I've had a look at the plugin and made some notes but I'm still a little stuck.

Here is a snippet of code:

getServices: function() {

    this.informationService = new Service.AccessoryInformation();
    this.informationService
		  .setCharacteristic(Characteristic.Manufacturer, this.manufacturer)
		  .setCharacteristic(Characteristic.Model, this.model)
		  .setCharacteristic(Characteristic.SerialNumber, this.serial);

    this.service
		  .getCharacteristic(Characteristic.CurrentHeatingCoolingState)
		  .on('get', this.getCurrentHeatingCoolingState.bind(this));

    this.service
	          .getCharacteristic(Characteristic.TargetHeatingCoolingState)
	          .on('get', this.getTargetHeatingCoolingState.bind(this))
	          .on('set', this.setTargetHeatingCoolingState.bind(this));
}

Do you see a way I might be able to have all the 'gets' set up to call the same function (a central HTTP get function) and for this to be called once, wait for it to finish then let it be called again, this time returning the cached values?

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Feb 20, 2019 via email

@phenotypic
Copy link
Author

@NorthernMan54 Just joined slack right now, so not quite sure how it works but I've got it set up with the Homebridge workspace (yet I can't seem to see any 'chat'). It'd be great if we could 1-1 message about it at some point, thanks! Do you need my email or something?

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Feb 20, 2019 via email

@NorthernMan54
Copy link
Contributor

@ebaauw Tks for your input on this and if you want to take a look at the final result take a look at this

https://github.com/Tommrodrigues/homebridge-web-thermostat

In the final design, we went with a 1 minute polling loop, removed all .on('get' and used updateValue for everything

@phenotypic
Copy link
Author

@ebaauw Indeed, thank you for all your advice. Following @NorthernMan54's help and guidance yesterday, I was able to implement similar functionality in my homebridge-web-rgb plugin as well as my homebridge-web-thermostat plugin.

Thank you to you both!

@graanco
Copy link

graanco commented Dec 10, 2019

I see you had the same issue as I am having.... And I see that you decide to do a interval poll instead of using .on('get'. The reason why I am going after on get is I don't want to use the polling (which does work). I would rather have the system only call when the user does. I this the wrong thought? And if I do it with on get instead of polling will this cause an issue with automation services with Homekit.h the screen fast enough

--edit---
In my programming the only time I have an issue is when I call the host website and the async seems to have to long of a delay? And sometime it looks like the sensor loose their callback too. If I refresh the screen fast enough the cache calls seem to work from the website.

Anyone thoughts?

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Dec 10, 2019 via email

@graanco
Copy link

graanco commented Dec 10, 2019

So the only real solution then is to use the polling as described in this thread. That is a shame since I would like to not have to call the API as much. It just seems unwarranted traffic. But if that is the way it works then I will go back to to the polling.

My rant against .on(‘get’ 1) Using .on(‘get’ causes the home app to behave slow on loading, as homebridge needs to get current status from the device, rather than using the cached value. 2) Using automations with .on(‘get’ will not work as the status is not updated in real time, and only update when looking at the page in the home app 3) Device status does not change in real time when viewing in the home app, a change in room is required to update the status

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Dec 10, 2019 via email

@graanco
Copy link

graanco commented Dec 12, 2019

Yes, the system supports events and I use setchara... on the events and update char... on the web scraps.

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Dec 12, 2019 via email

@graanco
Copy link

graanco commented Dec 12, 2019

Well it is a partial events on the system I am working on... the socket events only fire for the sensor when the system is armed. So my thought is do a set for the socket events and update during the scrapping from the JSON returns from the API. but that is when I get the no response.

@graanco
Copy link

graanco commented Dec 12, 2019

I posted the question on reddit with code. https://www.reddit.com/r/homebridge/comments/e8ro6l/looking_for_some_help/

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Dec 12, 2019 via email

@graanco
Copy link

graanco commented Dec 12, 2019

I would publish to fit as a branch but I don’t see how i can make another branch of my code.

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

No branches or pull requests

4 participants