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

Add PM10, PM2, PM1 sensors to AC-device #613

Merged
merged 10 commits into from
Oct 18, 2023

Conversation

skinkie
Copy link
Contributor

@skinkie skinkie commented Oct 15, 2023

This is my first copy-paste code for this project. I have not ran the code, as I first have to figure out how to do so in the HACS situation.

Fix #606

@skinkie skinkie marked this pull request as ready for review October 15, 2023 17:12
@skinkie
Copy link
Contributor Author

skinkie commented Oct 15, 2023

image

@skinkie
Copy link
Contributor Author

skinkie commented Oct 15, 2023

Given this state below, I wonder what I should implement to get "airState.quality.airMon = 1" (as being: measurements always on) working.

image

Copy link
Owner

@ollo69 ollo69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR.
Please attach your integration diagnostic so that I can test this feature.

custom_components/smartthinq_sensors/climate.py Outdated Show resolved Hide resolved
custom_components/smartthinq_sensors/wideq/const.py Outdated Show resolved Hide resolved
custom_components/smartthinq_sensors/wideq/devices/ac.py Outdated Show resolved Hide resolved
@skinkie
Copy link
Contributor Author

skinkie commented Oct 16, 2023

@ollo69 where can I find the integration diagnostic?

@ollo69
Copy link
Owner

ollo69 commented Oct 16, 2023

Here:

image

@skinkie
Copy link
Contributor Author

skinkie commented Oct 16, 2023

config_entry-smartthinq_sensors-f2602f0d26f4390e50358d85290c71ff.json.txt

@skinkie
Copy link
Contributor Author

skinkie commented Oct 16, 2023

I think what would be nice is if #614 is added, to make the sensor availability dependent on ('airMon' or not 'poweroff'), so there wouldn't be a false sense of security based on the minimum value.

@ollo69
Copy link
Owner

ollo69 commented Oct 16, 2023

I think what would be nice is if #614 is added, to make the sensor availability dependent on ('airMon' or not 'poweroff'), so there wouldn't be a false sense of security based on the minimum value.

It is better to proceed by step. When this PR is completed and merged you can rebase #614 to provide additional features.

@skinkie skinkie requested a review from ollo69 October 16, 2023 08:51
Comment on lines 1257 to 1259
support_key = self._get_state_key(SUPPORT_AIR_POLUTION)
if self._device.model_info.enum_value(support_key, "@PM1_0_SUPPORT") is None:
return None
Copy link
Owner

@ollo69 ollo69 Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest here to create a new function at device level, eg. is_pm_supported (similar, for example to is_mode_airclean_supported), to avoid to scan model_info every status refresh.

You could start creating a new constant:

SUPPORT_PM = [SUPPORT_AIR_POLUTION, ["@PM1_0_SUPPORT", "@PM10_SUPPORT", "@PM2_5_SUPPORT"]]

In the new function you should loop the second array and createa new boolean array (and store this in a new device attribute) that represent supported PM type.

Then here you can just call the device method that will scan model_info only on the first call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commited in 7cb81de.

Do you have any suggestion how to debug this component outside homeassistant?

Copy link
Owner

@ollo69 ollo69 Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Install Docker Desktop and Visual studio code
  • Open the project with Visual studio code
  • When the project is open, select the option clone in a new volume
  • A new environment will be created, and you can run a copy of HA with integration for test (command palette -> run task -> run home assistant core).

Do you want that I wait for your test result before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I test this code now by every time copying it to my 'live' HA. But there was an issue yesterday where I made a mistake and only saw in the evening what the issue actually was. I wondered if there was an option just to run this part of the code, for example in pycharm, so that debugging options are available.

Comment on lines +697 to +717
@property
def is_pm10_supported(self):
"""Return if PM sensors are supported."""
if self._is_pm_supported is None:
self._is_pm_supported = self._is_mode_supported(SUPPORT_PM)
return self._is_pm_supported[0]

@property
def is_pm25_supported(self):
"""Return if PM sensors are supported."""
if self._is_pm_supported is None:
self._is_pm_supported = self._is_mode_supported(SUPPORT_PM)
return self._is_pm_supported[2]

@property
def is_pm1_supported(self):
"""Return if PM sensors are supported."""
if self._is_pm_supported is None:
self._is_pm_supported = self._is_mode_supported(SUPPORT_PM)
return self._is_pm_supported[1]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that I like to have 3 different properties, because I'm planning to replace cache attributes with cached_property and I couldn't use in this case.
Apart this I like this approach, I'll think about this later.
Maybe should be better to order the array in different way (PM1, PM25 and PM10)? Or you use this order for a specific reason (I'm not a PM expert)?

Copy link
Contributor Author

@skinkie skinkie Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because from a human readability standpoint I liked:
1 = PM1
2 = PM2
0 = PM10 ;-)

I initially modelled this as a dictionary, with a key lookup, but for some reason, I could not get def is_pm_supported(self, key): working.

@ollo69
Copy link
Owner

ollo69 commented Oct 17, 2023

Please fix linting. If you use dev environment as explained above, black formatting is applied by Visual Studio on save.

@skinkie
Copy link
Contributor Author

skinkie commented Oct 18, 2023

Please fix linting. If you use dev environment as explained above, black formatting is applied by Visual Studio on save.

For this project I am using vim. Any thing I can run from the commandline?

@ollo69
Copy link
Owner

ollo69 commented Oct 18, 2023

For this project I am using vim. Any thing I can run from the commandline?

Vim, the programming environment for people who don't have to ask 😉.
Of course you can run black from command line, or just see the suggestion in test details and apply on your code. Or don't worry, I'll fix for you before merging.
But as future solution I suggest you to create a dev environment,

@skinkie
Copy link
Contributor Author

skinkie commented Oct 18, 2023

For this project I am using vim. Any thing I can run from the commandline?

Vim, the programming environment for people who don't have to ask 😉. Of course you can run black from command line, or just see the suggestion in test details and apply on your code. Or don't worry, I'll fix for you before merging. But as future solution I suggest you to create a dev environment,

Oh you actually now ran the tests. To be honest I have never heard about "black" before. Isn't gg=G enough ;-)

@skinkie
Copy link
Contributor Author

skinkie commented Oct 18, 2023

@ollo69 model_info.py is changed by black too.

@ollo69
Copy link
Owner

ollo69 commented Oct 18, 2023

Probably because you are not using right parameters for formatting

@skinkie
Copy link
Contributor Author

skinkie commented Oct 18, 2023

I ran black --line-length 88 ., from git workflow.

@ollo69
Copy link
Owner

ollo69 commented Oct 18, 2023

Probably you are using wrong black version. Check requirement.txt

@skinkie
Copy link
Contributor Author

skinkie commented Oct 18, 2023

Probably you are using wrong black version. Check requirement.txt

Indeed. 20 vs 23.

@ollo69 ollo69 merged commit 1b28764 into ollo69:master Oct 18, 2023
4 checks passed
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.

PM10, PM25, etc. sensors
2 participants