swttt / com.swttt.homekit

Homekit for Homey
https://apps.athom.com/app/com.swttt.homekit
GNU Affero General Public License v3.0
35 stars 20 forks source link

ACs show up in Apple Home but with limited features #213

Closed OlivierZal closed 2 years ago

OlivierZal commented 2 years ago

Hi,

I use HomeyKit to control my AC (from MELCloud app) through Apple Home, but many features which are available in Homey does not appear in Apple Home: although I can set the temperature and the heat/cool mode, I cannot set the fan power or vertical/horizontal position.

Do you know how to make all those features available from Apple Home?

Regards

robertklep commented 2 years ago

The MELCloud app uses custom capabilities, which means they are specific to that particular app. HomeyKit cannot handle custom capabilities.

OlivierZal commented 2 years ago

Hi @robertklep, thanks for your quick answer.

I contacted MELCloud app creator, who answered that solution should rather be on HomeyKit’s side than on its side: https://github.com/XattSPT/com.melcloud/issues/30#issuecomment-1014832682

What would be the solution to make HomeyKit benefit of all the features proposed by MELCloud app, that is, how to transform custom into standard capabilities?

Regards

robertklep commented 2 years ago

It's not possible to transform custom capabilities to standard capabilities, that's why they are called "custom" 😊

The main issue is that Homey's "standard" library lacks a lot of capabilities, which means that each developer has to invent their own (custom) ones. So the solution isn't on HomeyKit's side either, it's on Athom's side to add more standard capabilities.

In theory I could add support for the specific MELCloud capabilities to HomeyKit, but I don't want to add such specific dependencies to HomeyKit.

OlivierZal commented 2 years ago

I understand.

OlivierZal commented 2 years ago

If I well understand:

Your code seems to handle the fan power but not the position.

robertklep commented 2 years ago

All the capabilities you mention are custom and specific to the MELCloud app. Like I said, theoretically I can add support for them, but because they are custom I'm not going to because that creates dependencies on the MELCloud app.

OlivierZal commented 2 years ago

Hi @robertklep, would you agree with this? Would it be possible to use/test it locally?

robertklep commented 2 years ago

@OlivierZal thanks for the effort, but still, these changes depend on custom MELCloud capabilities which means that by adding them, we would be creating a dependency on that app in terms of future maintenance, which I don't want to do (not only that, but because they are custom, I don't know if there are other apps that happen to be using capabilities with the same name but with different values, which could cause issues for users with those apps).

Would it be possible to use/test it locally?

I would have assumed that you already did, how else could you create a PR? πŸ˜…

OlivierZal commented 2 years ago

Thanks @robertklep for having a look 😊 Indeed I made a PR but it was more for a design discussion basis πŸ˜…

I guess we could restrict the capability conditions to make sure it belongs to MELCloud and then not break devices using the same capability names, but I understand the maintenance argument (I saw a dependency to Philips Hue elsewhere in the device libs, but I think it's not the same popularity than MELCloud!).

So I would be glad if you can teach me (or provide me some material) how I can use my fork with my Homey locally.

robertklep commented 2 years ago

Nah, this is fine, and useful for others too πŸ‘πŸ»

Do you already have a version of your fork sources locally? If so, it's just a matter of installing the homey command line tool and running it from your fork's directory:

homey app run

This will start the app until you stop it, but it won't install it (so it'll be started after a reboot). You can do that using:

homey app install

If you don't already have the sources locally, you have to install Git and clone your fork repository:

git clone -b 'ozal/melcloud/fan_support' https://github.com/OlivierZal/com.swttt.homekit.git
OlivierZal commented 2 years ago

Thank you very much @robertklep.

I can install and run locally.

OlivierZal commented 2 years ago

@robertklep, I made some tests, and I succeed in making it work. However, I don't really get the use cases where we need to use addCharacteristics versus getCharacteristics (both seem to work). Any clue about the usage difference?

robertklep commented 2 years ago

However, I don't really get the use cases where we need to use `addCharacteristics versus getCharacteristics (both seem to work). Any clue about the usage difference?

I can't remember for certain, but some services in the Homekit protocol have optional characteristics, I think that we're using addCharacteristic on those to set them on the service (and getCharacteristic for non-optional characteristics, because they are already added to the service by the hap-nodejs library).

OlivierZal commented 2 years ago

Yes, everything is defined in node_modules/hap-nodejs/lib/gen/HomeKitTypes.js and also saw the method definition talking about the need to create an instance sometimes, but actually I don't see any difference of behaviour.

HomeKit types does not seem up-to-date with the latest HomeKit tools by the way.

OlivierZal commented 2 years ago

and the HomeyKit code does not always respect this rule, e.g. https://github.com/swttt/com.swttt.homekit/blob/master/lib/devices/thermostat.js#L66, although we have:

Service.Thermostat = function(displayName, subtype) {
  Service.call(this, displayName, '0000004A-0000-1000-8000-0026BB765291', subtype);

  // Required Characteristics
  this.addCharacteristic(Characteristic.CurrentHeatingCoolingState);
  this.addCharacteristic(Characteristic.TargetHeatingCoolingState);
  this.addCharacteristic(Characteristic.CurrentTemperature);
  this.addCharacteristic(Characteristic.TargetTemperature);
  this.addCharacteristic(Characteristic.TemperatureDisplayUnits);

  // Optional Characteristics
  this.addOptionalCharacteristic(Characteristic.CurrentRelativeHumidity);
  this.addOptionalCharacteristic(Characteristic.TargetRelativeHumidity);
  this.addOptionalCharacteristic(Characteristic.CoolingThresholdTemperature);
  this.addOptionalCharacteristic(Characteristic.HeatingThresholdTemperature);
  this.addOptionalCharacteristic(Characteristic.Name);
};
robertklep commented 2 years ago

I inherited most of the code of the project, so I can't speak as to why the original developer used one or the other. I probably use both interchangeably too πŸ˜…

OlivierZal commented 2 years ago

Hi @robertklep, so now I understand and manage better how it works... so that I found a bug: https://github.com/OlivierZal/com.swttt.homekit/commit/34c0f54a750b4be7d6e1645585c9462489f5c14e#diff-ab23aceaf01d9610b779f2ff02399f98c76b4a54185cc38254500e6e97e70ef5R273

it should be thermostat.getCharacteristic(Characteristic.TargetHeatingCoolingState).update(state2value(value)) since:

robertklep commented 2 years ago

It should be CurrentHeatingCoolingState because it's reflecting the thermostat mode that's currently set on Homey. Homey has no concept of a "target heating/cooling state".

OlivierZal commented 2 years ago

Ok @robertklep, but still I think it should be state2value(value) instead of value

OlivierZal commented 2 years ago

And auto could be handled based on a target and current temperature comparison.

robertklep commented 2 years ago

Yes, I agree on the state2value(), fixed it in 3fd9c8f1

OlivierZal commented 2 years ago

Thanks @robertklep, here is a successfully tested solution to handle better AUTO mode: https://github.com/OlivierZal/com.swttt.homekit/commit/446e9a7e4a4c31dbcec96831f2fed9a67e4e4fdb

I'm open to any comment or suggestion.

robertklep commented 2 years ago

The issue I have is I don't want to implement such logic in Homeykit. It should be a minimal "translation layer" between Homey and Homekit, and by adding additional logic the state of the thermostat in Homey ("auto") is decoupled from the state of the thermostat in Homekit ("cooling/heating/off").