skrollme / homebridge-eveatmo

Homebridge plugin which adds a Netatmo weatherstation as HomeKit device and tries to act like Elgato Eve Room/Weather
Apache License 2.0
69 stars 7 forks source link

Warnings since homebridge 1.0.2 #40

Closed nicoduj closed 4 years ago

nicoduj commented 4 years ago

Hi,

Since update, I got thoses warnings, about CarbonDioxideService I think :

HAP Warning: Characteristic 00000093-0000-1000-8000-0026BB765291 not in required or optional characteristics for service 0000008D-0000-1000-8000-0026BB765291. Adding anyway.

My config : { "platform": "eveatmo", "name": "NicoNetatmo", "extra_co2_sensor": false, "ttl": 900, "auth": { "client_id": "AAAA", "client_secret": "BBBB", "username": "TOTO@free.fr", "password": "AZERTY" }, "deviceTypes": [ "weatherstation" ], "whitelist": [ "02:0ggg0::44", "70:ggg42:d6", "03:hghgg3:0", "03:0:cggg9:10" ] },

nicoduj commented 4 years ago

Made an issue there : https://github.com/homebridge/HAP-NodeJS/issues/817, but it seems that the following service should be used :

export class CarbonDioxideSensor extends Service {

  static UUID: string = '00000097-0000-1000-8000-0026BB765291';

  constructor(displayName?: string, subtype?: string) {
    super(displayName, CarbonDioxideSensor.UUID, subtype);

    // Required Characteristics
    this.addCharacteristic(Characteristic.CarbonDioxideDetected);

    // Optional Characteristics
    this.addOptionalCharacteristic(Characteristic.StatusActive);
    this.addOptionalCharacteristic(Characteristic.StatusFault);
    this.addOptionalCharacteristic(Characteristic.StatusLowBattery);
    this.addOptionalCharacteristic(Characteristic.StatusTampered);
    this.addOptionalCharacteristic(Characteristic.CarbonDioxideLevel);
    this.addOptionalCharacteristic(Characteristic.CarbonDioxidePeakLevel);
    this.addOptionalCharacteristic(Characteristic.Name);
  }
}

Air Qaulity does not declare it anymore, as mentioned by @Supereg

Supereg commented 4 years ago

As described in the issue. There wasn't any change there. 🤔

nicoduj commented 4 years ago

As described in the issue. There wasn't any change there. 🤔

this plugin is using an AirQuality sensor with Characteristic.CarbonDioxideLevel . It is not declared anymore. I was just pointing that it should expose this service instead, but was not clear enough. I have edited my post, sorry

skrollme commented 4 years ago

Hello @nicoduj , I'm aware that the service/characteristic-combinations are not fully compliant with the standards but the aim of this plugin was to mimic the EVE-Devices as close as possible, to be used inside the EVE app. There were some tradeoffs to keep both apps - Apple's home.app and eve.app - both usable.

Are there still some points to discuss here? I did not update to homebridge 1.x so far, so I have no experience how this plugin works with this version.

nicoduj commented 4 years ago

Hi,

thanks for answering. I understand your point, I was just reporting a warning in homekit since 1.0.0 (evrything else is working fine),and i am questioning myself about this option "extraCO2 sensor", especially since netatmo is compatible for some version with homekit. Eve app is compatible also with homekit, so I am wondering if it would not be compliant ot expose CO2 always as another sensor instead of added to the airquality sensor, which in my opinion is not the way netatmo is doing it (both for home and eve app), but I might be wrong :) Or maybe make it exclusive : if we add a CO2 sensor, not adding it to the ariquality sensor at all.

You can off course close if you do not agree with this kind of change, just a reflexion :)

skrollme commented 4 years ago

Hello @nicoduj , I will keep this open as a reminder, but as mentioned before some of the decisions were made on purpose. And as long as it works like expected I think I will keep it this way. The extra CO2-Sensor was added for more info in apple's home.app. But I also see the warnings in the log after updating to homebridge 1.0

Supereg commented 4 years ago

Did the warning not show in previous releases? Just curious. As this warning is in place since a long time 🤔 It's just a save guard inside HAP-NodeJS to show that you probably building a service incompatible with the official HAP spec (and possible incompatible with the Home App. Could be that Home App still displays it fine). When you intentionally creating custom services, you may want to call addCharacteristics (the first time) instead of relying on getCharacteristic to add it. This will tell HAP-NodeJS that you are intentionally adding the characteristic to the service and will thus get rid of the warning.

nicoduj commented 4 years ago

@Supereg : the warning was not here before homebridge 1.0 since those characteristics were allowed I think in hap before . But like you said, it is not in the spec, even if supported by home app. addCharacteristic : "This is the way" 👍

Latest HAP :

export class AirQualitySensor extends Service {

  static UUID: string = '0000008D-0000-1000-8000-0026BB765291';

  constructor(displayName?: string, subtype?: string) {
    super(displayName, AirQualitySensor.UUID, subtype);

    // Required Characteristics
    this.addCharacteristic(Characteristic.AirQuality);

    // Optional Characteristics
    this.addOptionalCharacteristic(Characteristic.StatusActive);
    this.addOptionalCharacteristic(Characteristic.StatusFault);
    this.addOptionalCharacteristic(Characteristic.StatusTampered);
    this.addOptionalCharacteristic(Characteristic.StatusLowBattery);
    this.addOptionalCharacteristic(Characteristic.Name);
    this.addOptionalCharacteristic(Characteristic.OzoneDensity);
    this.addOptionalCharacteristic(Characteristic.NitrogenDioxideDensity);
    this.addOptionalCharacteristic(Characteristic.SulphurDioxideDensity);
    this.addOptionalCharacteristic(Characteristic.PM2_5Density);
    this.addOptionalCharacteristic(Characteristic.PM10Density);
    this.addOptionalCharacteristic(Characteristic.VOCDensity);
  }
}

HAP in 0.4.X

Service.AirQualitySensor = function(displayName, subtype) {
  Service.call(this, displayName, '0000008D-0000-1000-8000-0026BB765291', subtype);

  // Required Characteristics
  this.addCharacteristic(Characteristic.AirQuality);

  // Optional Characteristics
  this.addOptionalCharacteristic(Characteristic.StatusActive);
  this.addOptionalCharacteristic(Characteristic.StatusFault);
  this.addOptionalCharacteristic(Characteristic.StatusTampered);
  this.addOptionalCharacteristic(Characteristic.StatusLowBattery);
  this.addOptionalCharacteristic(Characteristic.Name);
  this.addOptionalCharacteristic(Characteristic.OzoneDensity);
  this.addOptionalCharacteristic(Characteristic.NitrogenDioxideDensity);
  this.addOptionalCharacteristic(Characteristic.SulphurDioxideDensity);
  this.addOptionalCharacteristic(Characteristic.PM2_5Density);
  this.addOptionalCharacteristic(Characteristic.PM10Density);
  this.addOptionalCharacteristic(Characteristic.VOCDensity);
  this.addOptionalCharacteristic(Characteristic.CarbonMonoxideLevel);
  this.addOptionalCharacteristic(Characteristic.CarbonDioxideLevel);
};
Supereg commented 4 years ago

It seems like it just went missing in the Typescript rewrite of HAP-NodeJS. Don't know if this was on purpose. But the spec clearly states something different, so we probably gonna leave it that way. You may want to to ensure that your addCharacteristic doesn't break old homebridge instances running in the case you wanna maintain compatibility

skrollme commented 4 years ago

Thanks for all your information. As mentioned above I'm aware, that the current-state does not follow the specifications. If this decision will break the functionality in the future, I will / have to change the plugin's structure. Until this I will keep it like it is at the moment. I can live with the knowledge that my homebridge log contains some warnings :D