AdyRock / com.misol

GNU General Public License v3.0
0 stars 1 forks source link

measure_rain "rain intensity" does not work as intended. measure_rain and measure_rain.event mixed up #34

Closed mech78 closed 1 month ago

mech78 commented 1 month ago

While translating https://github.com/mech78/com.misol/tree/german_translations I was wondering why I had mostly changed translations of "measure_rain.*" to "Regen", but I kept having the word "Niederschlag" in one capability. Actually that capability that is "Niederschlag" is "measure_rain". This is apparently a built-in capability of Homey. Compare: https://apps-sdk-v3.developer.homey.app/tutorial-device-capabilities.html having it's own translations and unit already (click on the three dots) in the overview. That's were the German word "Niederschlag" (German for a general form of rain; "Niederschlag" ist a generic term for "rain" ("Regen") which can mean any kind like snow, (watery rain), hail, ...) is taken from.

So basically that means that the mapping of the rain intensity with mm/hr does not work as intended and the following code in driver.compose.json of those devices is silently ignored and the Homey default capability setting appears to be used:

"capabilitiesOptions": { "measure_rain": { "title": { "en": "Rain Rate", "nl": "Regenintensiteit", "de": "Regenintensität" }, "units": { "en": "mm/hr", "nl": "mm/u", "de": "mm/h" } },

So actually both drivers should have a new capability "measure_rain.rate" instead getting those values. Then one can configure mm/hr as intended.

Leaves the question open: What should "measure_rain" be instead? - I'd suggest using the "measure_rain.event" data for this. Because as far as I understand the explanations in https://www.wetterstationsforum.info/viewtopic.php?t=241 or in English https://osswww.ecowitt.net/uploads/20210930/WN1900%20Manual.pdf the "rain event" is defined as: "Rain event is defined as continuous rain, and resets to zero if rainfall accumulation is less than 1 mm (0.039 in) in a 24-hour period."

I'd say this would be perfect for the current rain event's "mm" rainfall and would be what Homey's "measure_rain" default capability is intended for. A new custom "measure_rain.rate" would then actually reflect the "mm/h".

Actually if this is done I'd suggest also rephrasing all "Regen täglich/wöchentlich/monatlich/jährlich/gesamt" to "Niederschlag täglich/wöchentlich/monatlich/jährlich/gesamt" as it's a bit awkward to use two different translations if Homey's default translation for measure_rain is "Niederschlag" instead of "Regen". - One could argue though that Homey's translation is not best, because I doubt that my rain sensors would measure generic types like "snow" until it melts to water... - But at least it's more consistent then mixing the translations. I didn't do it in my PR as I didn't understand till know where "measure_rain" was actually obtaining it's translation from and was wondering why "Regenintensität" etc. didn't appear anywhere... https://github.com/AdyRock/com.misol/pull/33 - So I'd adjust the PR to reflect that.

Let me know if I can assist in fixing this or should create a PR myself for changing the mapping of the rain rate & rain event. Cheers, Michael

mech78 commented 1 month ago

Compare data from EcoWitt WS2910 via GW1100:

grafik

[ { "stationtype": "GW1100A_V2.3.2", "runtime": "1953401", "heap": "24872", "dateutc": "2024-05-20+10:02:07", "tempinf": "73.22", "humidityin": "51", "baromrelin": "28.039", "baromabsin": "28.039", "tempf": "73.22", "humidity": "48", "winddir": "243", "windspeedmph": "0.67", "windgustmph": "1.12", "maxdailygust": "4.47", "solarradiation": "678.47", "uv": "6", "rainratein": "0.000", "eventrainin": "0.472", "hourlyrainin": "0.000", "dailyrainin": "0.000", "weeklyrainin": "0.303", "monthlyrainin": "2.252", "yearlyrainin": "11.094", "totalrainin": "11.094", "temp1f": "73.04", "humidity1": "50", "temp2f": "75.56", "humidity2": "45", "soilmoisture1": "70", "soilad1": "332", "tf_ch1": "92.30", "wh65batt": "0", "batt1": "0", "batt2": "0", "soilbatt1": "1.7", "tf_batt1": "1.74", "freq": "868M", "model": "GW1100A", "interval": "16" } ]

IHMO "eventrainin" should be mapped to "measure_rain" and not to a dedicated capability. "rainratein"(afaik in 'in/h') should be a dedicated capability instead to show up.

AdyRock commented 1 month ago

I agree in principle that it should have been that way, so I will investigate that change to see what impact it would have on existing users Flows. I might have to just add a new measure_rain.rate and keep the measure_rain.event to reduce the likelihood of breaking too many Flows.

Also, thanks for the translation. What name shall I add to the app.json for the translations section?

mech78 commented 1 month ago

Probably https://github.com/mech78/com.misol/tree/fix_rain_capabilities gives some clues... Not yet checked what it could do to backwards compatibility though.

mech78 is just fine for the translation section.

AdyRock commented 1 month ago

I have updated the code to use measure_rain.rate. That fixes the title with the smallest change.

mech78 commented 1 month ago

Thx. Found a few issues, have a look at https://github.com/AdyRock/com.misol/pull/35