Profile library path for models case sensitive?!

[dagobert]

I am still continuing to create custom models of multiple devices to get ready for submission to you official library.

I ran into some problem which is connected to the libraries path naming. I have a few devices of the same manufacturer where the model character case differes from most others. This is due to the integration maintainer accidentally miss-typing it. E.g

HMIP-PSM instead of HmIP-PSM

Powercalc did not recognize this device until I changed the path to this "wrong" way of writing it.
The path that does not work is powercalc/profiles/eq-3/HmIP-PSM/model.json, the one that works is powercalc/profiles/eq-3/HMIP-PSM/model.json.

Her the model.json content:

{
  "measure_description": "Manually measured",
  "measure_method": "manual",
  "measure_device": "From manufacturer specifications",
  "name": "HmIP-PSM",
  "aliases": [
    "HMIP-PSM"
  ],
  "standby_power": 0.3,
  "standby_power_on": 0.3,
  "sensor_config": {
    "power_sensor_naming": "{} Device Power",
    "energy_sensor_naming": "{} Device Energy"
  },
  "device_type": "smart_switch",
  "calculation_strategy": "fixed",
  "fixed_config": {
    "power": 0
  }
}

As you can see, even the aliases do not help to identify the device when using the upper-lower case version of the path.

If I the path is case sensitive, this will lead to unintentional problems. If the maintainer is changing the character case in the future, I am already trying to create a pull request, the device suddenly will not be discovered anymore if I am using the only-upper-case path.

Unless there is a particular reason I cannot see it is probably better to change the path-to-model identification case-insensitive. I tried to find the parts in the code but failed until now.

[bramstroker]

Need to have a further look into this. I agree it is error prone.
But it's not easy to solve as most OS's which HA runs on are case sensitive.
So HmIP-PSM is another directory than HMIP-PSM

[dagobert]

I am very much willing to support you with this but maybe you can hint me into the richt point in your code.

In general I would traverse though the directories matching case insensitive the directory first of the manufacturer and when matched to the model directory.
On the other hand, since you allow aliases which is different to the model directory anyway, I guess it would be more like the following:
Match case-insensitive to the manufacturer directory than load all the model.json files, filter them by matching case-insensitive the list of names and aliases. Than you can either use either the path or the json-object in your further code.

I the present code with the case sensitive matching I would not even be able to guess how you would implement the alias matching.

What do you think?

[bramstroker]

Essentially there are 2 type of loaders:

• https://github.com/bramstroker/homeassistant-powercalc/blob/master/custom_components/powercalc/power_profile/loader/local.py

This one is responsible for loading models from local file system. Used for loading custom models.

• https://github.com/bramstroker/homeassistant-powercalc/blob/master/custom_components/powercalc/power_profile/loader/remote.py

This one is responsible for loading models from the powercalc build in library at https://library.powercalc.nl. It uses a custom build api at https://api.powercalc.nl and downloads profiles from github.

For your use case the local loader is applicable.

Related code is also here: https://github.com/bramstroker/homeassistant-powercalc/blob/master/custom_components/powercalc/power_profile/loader/local.py#L110.

To do development and fix the issue you should really work with running the unit tests. The whole component is 100% covered by tests. So whenever we do any changes tests can be added.

I will dedicate some topic in documentation how to setup the tests.
But following should work:

• poetry install
• poetry run pytest tests/

When you run with some Python IDE you can also use a debugger to run test and set breakpoints. I use PyCharm.

I might have a look into this issue later, somewhere coming weeks. Currently working on finalizing big development #2557, and busy with work.

[bramstroker]

Will be fixed with #2573

Also added some docs for integration development instructions.
https://docs.powercalc.nl/contributing/integration-development/

[dagobert]

The "name" key inside model.json is not used for model matching and autodiscovery at all. It's only for displaying purposes in the library at https://library.powercalc.nl Only the directory name is used and optionally configured aliases. The local loader does not support aliases yet, but I figured it does not make a lot of sense to add support for that as it's for custom models anyway, and you can just name the directory correctly. The remote loader does support aliases fully.

I understand. I was using the custom modeles to prepare them for submission. Having a difference in handling them local and remote I was not aware of. Maybe I missed it in the documentation.
If you think about it in that way maybe you are willing to agree to a patch. Otherwise let me know and I will use my time elsewhere...

Did you test my changes? As these should just work.

I tested your changes and they work as you intended. As soon as I have a folder matching the model name (independent of the case) It is discovered.

So, if I understand correctly when I have a folder name matching the model powercalc will use it independent of name and aliases.

Secondly, are the model.json files in the HA-storage folder considered as local or remote?

[bramstroker]

I understand. I was using the custom modeles to prepare them for submission. Having a difference in handling them local and remote I was not aware of. Maybe I missed it in the documentation. If you think about it in that way maybe you are willing to agree to a patch. Otherwise let me know and I will use my time elsewhere...

You mean to support aliases for local loader as well? That would be nice, to have similar functionality in the local loader. You can go forward an make a PR for that if you want.

So, if I understand correctly when I have a folder name matching the model powercalc will use it independent of name and aliases.

name is never considered as mentioned above, it's not used for model matching at all.
To see if a model is supported powercalc will look first in the local loader, than the remote.
Next it wil first try to match a found model in HA device info against the model_id in powercalc (this is the directory name).
When that not matches it will check any known aliases which are defined in model.json. Currently this only works for the remote loader.

Secondly, are the model.json files in the HA-storage folder considered as local or remote?

These are handled by the remote loader. Essentially they are some kind of first level cache to prevent downloading the profile again each time. It will only download when the profile has been updated or the files in .storage directory are missing.

[dagobert]

You mean to support aliases for local loader as well? That would be nice, to have similar functionality in the local loader. You can go forward an make a PR for that if you want.

I have implemented the first version. It is not too pretty because I have to keep to your general loader calls. Hence I am re-reading the directory over and over. :(

But I think I have a solution for this:
If this is fine to you, I will create a dictionary like follows within the find_model function since I figure that this is the first function called by library.py whenever you are looking for a new model. Therefore, whenever afterwards load_model is called I can get my information from there insteadof searching for the alias again. This is due to the fact that load_model does not get the directory because until now you ignored aliases.

lib = {
  "manufacturer" : {
    "model" : "directory"
  }
}

[bramstroker]

You can create a PR, than we can discuss further when I see the code.
Only the local loader should change imo.

[dagobert]

You can create a PR, than we can discuss further when I see the code. Only the local loader should change imo.

I have created a pull request with my preferred version of three I have implemented: #2610