Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various sensor fixes #34

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alex-w2
Copy link

@alex-w2 alex-w2 commented Sep 29, 2024

Correctly assign SensorEntityDescription to the correct attribute. This fixes long term statistics (state_class correctly recorded) and removes the need for overriding some properties.

With the fixed SensorEntityDescription we have a malconfigured power sensor however. Assuming the state class is supposed to be measurement, the sensor device class probably needs to be POWER with the unit being WATT.

For some reason, the energy sensor was constructed if the temperature is available int he state value. Probably it should depend on the power, not temperature (same as the power sensor).

The energy sensor also had two conflicting sensor states configured (total and total increasing). Stuck with the latter for now, removed the former.

Made the power calculation a bit more robust as well

Correctly assign SensorEntityDescription to the correct attribute.
This fixes long term statistics (state_class correctly recorded)
and removes the need for overriding some properties.

With the fixed SensorEntityDescription we have a malconfigured power
sensor however. Assuming the state class is supposed to be
measurement, the sensor device class probably needs to be POWER
with the unit being WATT.

For some reason, the energy sensor was constructed if the
temperature is available int he state value. Probably it should
depend on the power, not temperature (same as the power sensor).

The energy sensor also had two conflicting sensor states configured
(total and total increasing). Stuck with the latter for now,
removed the former.

Made the power calculation a bit more robust as well
@alex-w2
Copy link
Author

alex-w2 commented Sep 29, 2024

This change will conflict slightly with #31 (contains some of those changes).

Furthermore, I'm making some asusmptions here about the power sensor, which I would need help validating as my rctouch'es don't consume power. Since there's both a power and an energy sensor defined, I would assume the power should report power consumption and energy report energy consumption. As it was, both were device class energy. Chaning that, the unit also needed changing to watt. Is this correct? I have no idea..

However, with my changes the state_class is correctly registered and measurement is not compatible with watt hour. So it either needs to be watt and measurement (and probably energy), or watt hour and increasing/total increasing (and senor type power, but then we have two power sensors..).

The power sensor entity name also seems wrong, but changing it now would be a breaking change? So I did not change it.

Appreciate if someone with actual power consumption could validate this change and see if it makes sense or not.

@gunmalmg
Copy link

gunmalmg commented Sep 29, 2024

@alex-w2 Thank you for your respons.
I will test as soon as I come home from "høstferie".
But how can I test this ?. How do I get this update ??

vh Gunnar

@alex-w2
Copy link
Author

alex-w2 commented Sep 29, 2024

@gunmalmg I'm not sure how you installed the addon initially (throught HACS or directly from github?), but you'd have to manually install the version on this branch from my fork into custom_components and then restart the addon/home assistant.

If you want to give it a shot, make sure to do a backup beforehand, in case things go wrong.

@gunmalmg
Copy link

@alex-w2 I ‘not that skilled to safely do that kind of testing. I can manage to download beta versions through HACS.
But - if you are a Norwegian, I can give you(send you) some xComfort devices - so you can do direct testing yourself.
If so – you can send me an e-mail (in Norwegian) to [gunnar@malmgard.no] – and we take it from there.

Regards Gunnar

@alex-w2
Copy link
Author

alex-w2 commented Oct 1, 2024

We can perhaps use a custom hacs repository, let me look into it in the weekend.

@alex-w2
Copy link
Author

alex-w2 commented Oct 6, 2024

@gunmalmg you should be able to add my repository (https://github.com/alex-w2/ha-xcomfort-bridge/) as a custom repository in HACS now, then install the latest version from there. This version contains both PRs #34 and #35.

@gunmalmg
Copy link

gunmalmg commented Oct 7, 2024 via email

@olegje
Copy link

olegje commented Oct 8, 2024

Hello!
I am working on making my Xcomfort system work together with HA.
I have several RCtouch devices that draw power and can be used for testing.
However, I have some challenges making the alex-w2 branch work.
I first set up the master branch, and it's working, importing 43 devices and 79 entities. Of course, I then get all the errors and bugs with the RCT device.
Then I rolled back (rolled back the entire VM to before the Xcomfort installation) and installed the Alex-W2 branch. But then I get an error when I try to initialize the integration. "Error setting up entry 192.168.10.247 for xcomfort_bridge"

I hope we will get all these latest PRs working and merged into the master branch as soon as possible.
If there's anything I can do to help or test something, let me know.
Cheers to the people who have put their time into this!

*Edit: Simple error to fix, just missing a , in line 24 of sensor.py, this was on the version that i installed via HACS.
Maybe fix this and update the readme om how to install via HACS?

@alex-w2
Copy link
Author

alex-w2 commented Oct 8, 2024

Sorry about that, fixed and pushed a new release now at https://github.com/alex-w2/ha-xcomfort-bridge

@alex-w2
Copy link
Author

alex-w2 commented Oct 8, 2024

I can cleanup the readme a bit later perhaps, however, not really planning for my repo to become the main repo here. Goal is to merge upstream to here 😄

@olegje
Copy link

olegje commented Oct 9, 2024

This change will conflict slightly with #31 (contains some of those changes).

Furthermore, I'm making some asusmptions here about the power sensor, which I would need help validating as my rctouch'es don't consume power. Since there's both a power and an energy sensor defined, I would assume the power should report power consumption and energy report energy consumption. As it was, both were device class energy. Chaning that, the unit also needed changing to watt. Is this correct? I have no idea..

However, with my changes the state_class is correctly registered and measurement is not compatible with watt hour. So it either needs to be watt and measurement (and probably energy), or watt hour and increasing/total increasing (and senor type power, but then we have two power sensors..).

The power sensor entity name also seems wrong, but changing it now would be a breaking change? So I did not change it.

Appreciate if someone with actual power consumption could validate this change and see if it makes sense or not.

Yes, your changes to power sensor makes sense. It shows power as Watt, and it it the same value as the Eaton app shows.
The energy sensor make sence, but i cant guarante the value. 3,5 Kwh in 24 hours on my bathroom.
bad1

And yes, it would be nice if the sensors had a more describabale name.

@javydekoning
Copy link

javydekoning commented Oct 19, 2024

Sorry about that, fixed and pushed a new release now at https://github.com/alex-w2/ha-xcomfort-bridge

Tried adding it, but returns:

image
2024-10-19 08:00:47.661 ERROR (MainThread) [homeassistant.config_entries] Error occurred loading flow for integration xcomfort_bridge: No module named 'xcomfort'
2024-10-19 08:01:13.290 ERROR (MainThread) [homeassistant.config_entries] Error occurred loading flow for integration xcomfort_bridge: No module named 'xcomfort'
2024-10-19 08:01:24.717 ERROR (MainThread) [homeassistant.config_entries] Error occurred loading flow for integration xcomfort_bridge: No module named 'xcomfort'

Looks like this depends on https://github.com/jankrib/xcomfort-python, so just installing through HACS without incl dependencies wont work

@alex-w2
Copy link
Author

alex-w2 commented Oct 19, 2024

@javydekoning Did you install via HACS? It does depend on https://github.com/jankrib/xcomfort-python, but this should be handled automatically if installed through HACS (it's listed in the manifest). At least it worked when I test installed from HACS.

If you downloaded the integration manually you'll need to satisfy the dependency yourself (install it or download it to somewhere in the Python modules path).

@javydekoning
Copy link

javydekoning commented Oct 19, 2024

@javydekoning Did you install via HACS? It does depend on https://github.com/jankrib/xcomfort-python, but this should be handled automatically if installed through HACS (it's listed in the manifest). At least it worked when I test installed from HACS.

If you downloaded the integration manually you'll need to satisfy the dependency yourself (install it or download it to somewhere in the Python modules path).

Yes, I did install via HACS. Dependency didn't install.

@alex-w2
Copy link
Author

alex-w2 commented Oct 19, 2024

@javydekoning Not sure what the issue is, I tested again on a fresh HA instance (container) by installing HACS and then installing the xComfort bridge integration from my custom repository. It worked fine. I can tell that the xcomfort module was installed by running "pip list" in a terminal.

Can you try manually installing the xcomfort via pip in a terminal?

@javydekoning
Copy link

javydekoning commented Oct 20, 2024

I'm using ghcr.io/home-assistant/home-assistant:2024.10.3

Tried both the master branch and v0.1.2-alex-w2-r3, both facing the same issue.

/config # pip list | grep -i pycryptodome
pycryptodome                     3.21.0
pycryptodomex                    3.21.0
/config # pip list | grep -i rx
pyRFXtrx                         0.31.1
Rx                               3.2.0
rxv                              0.7.0
/config # pip list | grep -i xcomfort
/config #

If I manually pip install xcomfort things work, but given that this is a container the dependency is gone after a container restart or a hass upgrade.

@alex-w2
Copy link
Author

alex-w2 commented Oct 20, 2024

@javydekoning I'm out of ideas, this is not really my area of expertise unfortunately. I just threw something together quickly to let people test while we wait.

@alex-w2
Copy link
Author

alex-w2 commented Oct 20, 2024

@olegje, @gunmalmg Is it working for you?

@gunmalmg
Copy link

gunmalmg commented Oct 22, 2024 via email

@javydekoning
Copy link

I have tested this in the devcontainer. It works there and installs the dependency. Not sure why it's different in my running install, but I'll keep digging.

@javydekoning
Copy link

javydekoning commented Nov 9, 2024

Had some more time to spend on this, still face the same with a completely vanilla install of home-assistant.

Using: ghcr.io/home-assistant/home-assistant:latest

Debug logs enabled:

2024-11-09 07:58:10.887 INFO (SyncWorker_50) [homeassistant.util.package] Attempting install of xcomfort==0.1.2
2024-11-09 07:58:10.888 DEBUG (SyncWorker_50) [homeassistant.util.package] Running uv pip command: args=['/usr/local/bin/python3', '-m', 'uv', 'pip', 'install', '--quiet', 'xcomfort==0.1.2', '--index-strategy', 'unsafe-first-match', '--upgrade', '--constraint', '/usr/src/homeassistant/homeassistant/package_constraints.txt', '--target', '/config/deps']
2024-11-09 07:58:12.317 INFO (SyncWorker_50) [homeassistant.util.package] Attempting install of rx==3.0.1
2024-11-09 07:58:12.317 DEBUG (SyncWorker_50) [homeassistant.util.package] Running uv pip command: args=['/usr/local/bin/python3', '-m', 'uv', 'pip', 'install', '--quiet', 'rx==3.0.1', '--index-strategy', 'unsafe-first-match', '--upgrade', '--constraint', '/usr/src/homeassistant/homeassistant/package_constraints.txt', '--target', '/config/deps']
2024-11-09 07:58:12.631 INFO (SyncWorker_50) [homeassistant.util.package] Attempting install of pycryptodome==3.15.0
2024-11-09 07:58:12.631 DEBUG (SyncWorker_50) [homeassistant.util.package] Running uv pip command: args=['/usr/local/bin/python3', '-m', 'uv', 'pip', 'install', '--quiet', 'pycryptodome==3.15.0', '--index-strategy', 'unsafe-first-match', '--upgrade', '--constraint', '/usr/src/homeassistant/homeassistant/package_constraints.txt', '--target', '/config/deps']
2024-11-09 07:58:13.114 DEBUG (MainThread) [homeassistant.loader] Importing platforms for xcomfort_bridge executor=['config_flow'] loop=[] took 0.00s
2024-11-09 07:58:13.115 ERROR (MainThread) [homeassistant.config_entries] Error occurred loading flow for integration xcomfort_bridge: No module named 'xcomfort'

Config file used:

default_config:

http:
  server_host: 0.0.0.0
  trusted_proxies:
    - 10.0.0.0/8
  use_x_forwarded_for: true
  base_url: <my-dev-env>

logger:
  default: info
  logs:
    homeassistant.loader: debug
    homeassistant.setup: debug
    homeassistant.requirements: debug
    homeassistant.util.package: debug
    pip: debug

xcomfort is installed in /config/deps, but not loaded. Likelt not in $PYTHONPATH.

ls custom_components/ deps/
custom_components/:
xcomfort_bridge

deps/:
aiohappyeyeballs                  attrs                       multidict-6.1.0.dist-info      Rx-3.0.1.dist-info
aiohappyeyeballs-2.4.3.dist-info  attrs-24.2.0.dist-info      multidict.libs                 xcomfort
aiohttp                           Crypto                      propcache                      xcomfort-0.1.2.dist-info
aiohttp-3.10.10.dist-info         frozenlist                  propcache-0.2.0.dist-info      yarl
aiohttp.libs                      frozenlist-1.5.0.dist-info  propcache.libs                 yarl-1.17.1.dist-info
aiosignal                         idna                        pycryptodome-3.15.0.dist-info  yarl.libs
aiosignal-1.3.1.dist-info         idna-3.10.dist-info         pycryptodome.libs
attr                              multidict                   rx

I've added a logging statement in the custom component __init__.py and found that indeed /deps/config is not in the Python path.

2024-11-09 08:55:13.136 INFO (ImportExecutor_0) [custom_components.xcomfort_bridge] Python path: ['/config/deps/lib/python3.12/site-packages', '/config', '/usr/local/lib/python312.zip', '/usr/local/lib/python3.12', '/usr/local/lib/python3.12/lib-dynload', '/usr/local/lib/python3.12/site-packages', '__editable__.homeassistant-2024.11.1.finder.__path_hook__']

Not sure how this should work for customer components.

Reached out on ha forums for support: https://community.home-assistant.io/t/custom-component-development-loading-python-dependencies/792381

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants