Olen / homeassistant-plant

Alternative Plant component of home assistant
218 stars 23 forks source link

Use default values if min / max is missing in openPlantBook #152

Closed ChristophCaina closed 2 weeks ago

ChristophCaina commented 3 months ago

Hi, I am creating some new plants that are currently not available in OpenPlant Book. As of now, I don't have the min/max values for illumination and the other sensors, so I kept them empty until I can provide some information for those.

Plant-Monitor can find the plant, but is now showing that the information are missing.

image

Unfortunately, this means, I cannot create the plant. If the plant would not be available in OpenPlantBook, the integration is providing default values.

I can then use the default values (and hopefully they would be updated when the information will be available in Open Plant Book)....

Right now, it would be great, if the integration could show an information that the values are missing in OpenPlantBook - but would then use the default values instead, to allow the creation of the plant in HA

Olen commented 3 months ago

I don't understand?

Can't you just input whatever values you want and add the plant?

I guess openplantbook returns blank values for empty?
I believe the best solution here is to have OPB return some more or less sane default values for empty data.

ChristophCaina commented 3 months ago

I don't think so. Without using OpenPlantBook, the Integration does provide some "default" min / max values already. I would consider, that these should also be used, if OpenPlantBook would deliver NULLs if the information is missing.

OpenPlantBook should not fall back to defaults, as I would like to update those information in OpenPlantBook rather than having some (maybe wrong) information there.

The HA integration otherwise should allow me to setup the plant even if the information are missing in OPB.

ChristophCaina commented 3 months ago

If I create the plant without using opb - it is using the defaults - and then, I can change the integration to use opb again. This "step in between" would be obsolete if the integration would use its defaults directly as a fall back

Olen commented 3 months ago

But those fields are just plain input-fields? You can just insert your own numbers if the ones from OPB does not suit you (even if they are empty, I guess, I don't use custom plants so I have no idea what that looks like).

Can't you just type a number there, or use the small arrows to increase/decrease the value?

ChristophCaina commented 3 months ago

ok, following use cases:

I use OPB because I want to use the values reported from there. -> Updating / Changing the values provided by OPB with my own ones would mean, they would never be fetched again from OPB - in case they would change. In my eyes, that's not correct behave. If I don't want to use the values from OPB - I should not use the OPB integration. OPB should asure, that changes in the values will be updated in my integration.

  1. sure - I can setup my own default values... but in case that I don't sync with OPB - the integration does use default values. Why not also so, when OPB provides NULL?
Olen commented 3 months ago

Please feel free to add a PR that fixes this (and possibly explains it better, because I still fail to see that this is a big problem).

And there is no way for the integration to know if a value was originally fetched from OPB, if it was input manually by the user or of it was the "default" value. And you can easily change these values by updating the "number.xxxx" entries, and of course the integration would not know anything about that either.

My point is that the sentence "I cannot create the plant." does not make any sense. Just add the plant, if OPB does not provide numbers, invent your own. After all - the "default" values were just invented by me at some point.

Yes, maybe it would be convenient in the odd case where OPB does not provide correct values to not have to type in a few numbers, but in my head, there is a bug in OPB if it does not provide all the data it promises.

There is already code in place that ensures that if the value is not retuned at all from OPB, it uses the default value:

            max_moisture = opb_plant.get(
                CONF_PLANTBOOK_MAPPING[CONF_MAX_MOISTURE], DEFAULT_MAX_MOISTURE
            )
etc.

But if OPB returns "something", but it's not a number, then the (correct) error message "expected int" is shown.

ChristophCaina commented 3 months ago

ok, thanks for the explanation about the last topic. I will have a look if I can change that to fall back to the defaults then

Olen commented 3 months ago

Again, In my opinion, it is a bug in OPB if it returns something that is not a number (including an empty string) for those values. It should either return a number or not return anything.

ChristophCaina commented 3 months ago

honestly, even if it is a bug in opb - which should be fixed there, I think it would make sense to catch some of those cases also in the integration.

Talking about convenience - I am using already 15 or 16 devices ... 11 more should be added now, in some cases with some more exotic plants that aren't available in big numbers - so they are not yet available in opb at all.

therefore, just having the defaults would be very convenient, rather than providing all required information for each of the plants in a row... setting up all those sensors is already a bit time consuming ^^

Don't get me wrong - I am just want to help, making this integration even better ;)

Olen commented 3 months ago

Yes, sure. But as I said. The integration expects OPB to return numbers or nothing. So catching real empty values is alrady somthing that is done (for convenience). You could ofcourse add some try/expect or cast to int or strip anything that is not a number, but OPB should really not allow itself to send that kind of data, so a general error for "this is neither an int nor 'none'" to catch all kinds of possible bad values and warn the user that OPB returns the wrong data feels better than trying to guess what OPB really ment.