1technophile / OpenMQTTGateway

MQTT gateway for ESP8266 or ESP32 with bidirectional 433mhz/315mhz/868mhz, Infrared communications, BLE, Bluetooth, beacons detection, mi flora, mi jia, LYWSD02, LYWSD03MMC, Mi Scale, TPMS, BBQ thermometer compatibility & LoRa.
https://docs.openmqttgateway.com
GNU General Public License v3.0
3.6k stars 793 forks source link

MQTT Discovery value key definitions #690

Closed jmw6773 closed 4 years ago

jmw6773 commented 4 years ago

Describe the bug The value key definitions for temperature (BME, HTU, AHT) and presence (HCSR501 sensor) do not align with the keys that these sensors use. This causes the mqttDiscovery to fail for these devices. (It is a partial fail for the temperature sensor as the 'tempf' and 'hum' works as is.)

Temperature https://github.com/1technophile/OpenMQTTGateway/blob/0e257a58c137124c81967086970353640ec411c8/main/config_mqttDiscovery.h#L80

PIR https://github.com/1technophile/OpenMQTTGateway/blob/0e257a58c137124c81967086970353640ec411c8/main/config_mqttDiscovery.h#L83

Expected behavior The following value keys work for the (BME, HTU, AHT) temperature sensors and the HCSR501.

#  define jsonTemp     "{{ value_json.tempc | is_defined }}"
#  define jsonPresence "{{ value_json.hcsr501 | is_defined }}"

The DHT sensor would require the following (or a change to the ZsensorDHT.ino file to change the published JSON key to match other keys).

#  define jsonTemp     "{{ value_json.temp | is_defined }}"
jmw6773 commented 4 years ago

The Presence is used for Bluetooth, so this key shouldn't be changed.

https://github.com/1technophile/OpenMQTTGateway/blob/0e257a58c137124c81967086970353640ec411c8/main/config_mqttDiscovery.h#L83

Instead the HCSR501 should update it's key to use the 'presence' key for its data value instead of 'hcsr501'. https://github.com/1technophile/OpenMQTTGateway/blob/0e257a58c137124c81967086970353640ec411c8/main/ZsensorHCSR501.ino#L51-L57

1technophile commented 4 years ago

Thanks for pointing this.

Currently we have modules pushing celsius temperature with the tem and tempc keys, we should choose one of them and apply the change in the code I think. Another possibility could be to consider the use of tempc and tempf keys for the modules that send both and tem for the modules that send only celsius. What is your opinion on this? Unfortunately this will be a breaking change, so we have to take this into account also.

I agree for the hcsr501.

jmw6773 commented 4 years ago

The DHT module is pushing temp as its key, meaning we have at least three sensors using different keys.

We could also replace the tem key with the tempc key and always send the 'tempc' and 'tempf' keys. This would require updates to add code to convert C -> F (or F -> C) for at least the DHT and DS18B20 sensors and the mqttDiscovery definitions, but shouldn't be difficult.

This would very slightly increase the function's runtime and the MQTT payload, but would make it easier from the users' perspective as they wouldn't need to determine if the OMG is sending C or F values and then convert the values in the receiving system. (HA or Node-Red)

If we add macro functions, like the ones below, to the User_config.h file or other common header file, the conversions wouldn't take much code space. But this may disorganize the file structure.

#define CtoF(c) ((c * 1.8) + 32)
#define FtoC(f) ((f - 32) * 5/9)

What do you think?

1technophile commented 4 years ago

Good idea the macro ! I agree that having both keys tempc and tempf will be easier to understand and integrate. The most used gateway using temperarure is the BT one. So if we replace tem by tempc we will sure break integrations. I'm not sure what is the best way to manage this change. Keep tem for the next version with also tempc and tempf, add a macro to come back to tem, or remove tem. Do you have some ideas about how to manage the change ?

jmw6773 commented 4 years ago

I think you're right about keeping the current key, and adding tempc and tempf for all of the sensors until 0.9.5 is released. We can then remove the tem key from the BT and the temp key from DHT for the 0.9.6 release. This way users will get an announcement about the change for 0.9.5 and a Breaking Change for the 0.9.6 release.

1technophile commented 4 years ago

ok let's do it this way

jmw6773 commented 4 years ago

Sorry to break topic, but are you experiencing any compile issues?

I've been unable to compile OMG since Sunday evening on PlatformIO (Windows - v4.3.4) I've re-pulled the repo, deleted the .platformio directory, and reinstalled PlatformIO , but I keep getting an error that PlatformIO isn't able to find the libraries with versions specified for the RFM69 and BMP180 libraries.

UndefinedPackageVersion: Could not find a version that satisfies the requirement '4a6f77ad47' for your system 'windows_amd64':

If I remove the specified version for the RFM69 and BMP280 libraries, it pulls the current version, but PlatformIO does not pull the dependencies for the RFM69 library. (SPIFlash, Low-Power) If I manually download these dependancies or put them in the lib_deps field the ENV I'm building in platformio.ini file, OMG compiles correctly.

I don't think its OMG's configuration that is causing this error. I'm not sure if there is something wrong with the PlatformIO library manager.

Is anyone else seeing these errors, or is it just my computers?

1technophile commented 4 years ago

Is anyone else seeing these errors, or is it just my computers?

I reproduce the error with RFM69 library, I'm investigating

1technophile commented 4 years ago

Seems that there is an issue with the PIO versions identification of the RFM69 lib image

The versions available doesn't match with the ones available on github

1technophile commented 4 years ago

Just opened a thread here in platformio forum.

1technophile commented 4 years ago

I had to replace the @ by # before the sha of the 2 libraries.

jmw6773 commented 4 years ago

@1technophile , Just read the issue again realized I didn't make any changes to fix the HCSR501 sensor's MQTTDiscovery key. It's an easy change, but do you want to publish the old and new keys for one release like we're doing for the temperature sensors, then remove it for 0.9.6?

i.e. {"hcsr501":"true", "presence":"true"}

1technophile commented 4 years ago

I don't think this module is a lot used, so I would proceed directly with the change.