Add config flow for selecting precision of DS18B20 devices by droberg · Pull Request #64315 · home-assistant/core

droberg · 2022-01-17T18:24:18Z

Breaking change

Proposed change

Add config flow for selecting temperature precision of Maxim DS18B20 1-wire temperature sensor.

Note that there are still things that needs fixing:

Remove personal debug-logging
Use config setting in the entities paths for reading the values
Make sure constants etc are put in the constants file
Add test code for reading the temperature
Add test code for the config flow
Verify that the strings.json and translation is used correctly
Make config trigger "re-init" of device automatically
Do not break SysBus implementation while it's still supported

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-01-17T18:24:24Z

Hey there @garbled1, @epenet, mind taking a look at this pull request as it has been labeled with an integration (onewire) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

thecode · 2022-01-17T19:18:13Z

@droberg please note that using this integration with a direct IO connection on a Raspberry PI is deprecated from next release, it will only be allowed to be used via owserver.

epenet · 2022-01-17T21:15:24Z

I think the device registry would be best to get the device name.

epenet · 2022-01-18T10:47:53Z

I see that @emontnemery is working on #64350. That might make this PR redundant...
Let's wait for his feedback before spending too much time on this PR

epenet · 2022-01-18T14:47:02Z

I looks like the work from emontnemery doesn't solve the issue here, so we'll still need the option flow.

droberg · 2022-01-18T14:56:44Z

Ok, I haven't had time to read up on #64350 but according to the title it looked exactly like the thing we wanted. I will change the implementation to use friendly names as soon as I have time. I'm still putting in more time learning the structure than actually implementing code so no harm done if this becomes redundant at this point.

I was also thinking that I'd rather have the configuration as drop down selectors or using enums etc but this was the best I could do after this level of digging around in the code.

droberg · 2022-01-18T20:15:19Z

I have now read up on #64350 and as I read it it adds storage of options in the entity structures, with the comment that there is no solution of how to expose this to the frontend yet.

When/if this is exposed to the frontend it should cover the "type" option in the same manner as other options as I understand it. Or am I missing something?

epenet · 2022-01-19T08:17:55Z

I opened this architecture discussion to see what would be possible: home-assistant/architecture#713

I believe that since virtual entities are forbidden (for now) then the OptionsFlow as you are implementing here is the best way to provide a UI.

Regarding the storage of the options, I would also keep storing it in the config entry options - it should be a small enough tweak if the policy changes down the line.

emontnemery · 2022-01-21T13:55:57Z

The next logical step after #64366 is to allow customizing precision in terms of number of digits.
Could you use that to infer the appropriate precision of the DS18B20, although that is configured in bits and not in decimal digits?

epenet · 2022-01-21T14:13:39Z

@emontnemery this PR would serve as a base for other types of sensors:

DS2760 to select the thermocouple type (B, E, J, K, N, R, S and T)
DS2438 to select the humidity sensor type (HIH-4000, HIH-5030, HIH-5031, etc.)

emontnemery · 2022-01-21T14:41:35Z

Oh, OK.
In that case, go ahead with the config flow 👍

droberg · 2022-01-22T20:59:59Z

It now uses the names from the device-registry together with the device id's. This felt like the only way of getting it to work nicely with the config flow multi-select, but it also felt like a good idea to have both the user assigned name and the actual id in the description.

droberg · 2022-01-26T19:30:20Z

Rebased

homeassistant/components/onewire/sensor.py

droberg · 2022-01-26T20:55:06Z

Not sure if the added test actually tests something new

epenet · 2022-01-26T21:04:43Z

Not sure if the added test actually tests something new

You need to adjust the mock config entry to add the options for the new sensor

tests/components/onewire/conftest.py

homeassistant/components/onewire/sensor.py

droberg · 2022-01-30T12:44:14Z

Rebased

droberg · 2022-01-30T12:47:45Z

@epenet : I'm having some troubles with my environment here. It seems that pyownet and pi1wire is no longer available in the dev-container.

I will install them manually but is this because direct access is being deprecated ?

epenet · 2022-01-30T19:32:47Z

@epenet : I'm having some troubles with my environment here. It seems that pyownet and pi1wire is no longer available in the dev-container.

I will install them manually but is this because direct access is being deprecated ?

It's fine on my side - I'm not having any issues.
Direct access (with pi1wire library) is being deprecated but it is still supported and shouldn't get removed before 2022.6 (I think). OWServer (with pyownet) is not deprecated and is fully supported...

droberg · 2022-01-30T21:00:58Z

Oups, messed up when trying a git amend

droberg · 2022-02-22T10:12:29Z

Thanks!
I will have a look at this in a few hours.

epenet

LGTM, but I'd like a second pair of eyes

homeassistant/components/onewire/config_flow.py

balloob

Looks ok if last comment addressed.

droberg · 2022-02-23T20:30:30Z

Cat on keyboard

droberg · 2022-03-02T19:36:10Z

Sorry for the delay. Will get on with this as soon as possible.

droberg · 2022-03-02T20:30:38Z

FIxed review comment, rebased, tested locally etc.

epenet

Latest changes look good to me.
@balloob do you agree?

epenet · 2022-03-03T19:52:27Z

Thanks @droberg 👍

As a follow-up I suggest that you add one for the Thermocouple type (family 30)...

MartinHjelmare

Please address the comments in a new PR. Thanks!

homeassistant/components/onewire/__init__.py

homeassistant/components/onewire/config_flow.py

MartinHjelmare · 2022-03-04T08:28:54Z

tests/components/onewire/test_options_flow.py

@@ -0,0 +1,237 @@
+"""Tests for 1-Wire config flow."""


These tests should move to the test_config_flow.py module.

The reason they are separate is that they needed different mock-setups. This may perhaps sort itself later, when the device-registry comment is handled. (But I can't say for sure )

MartinHjelmare · 2022-03-04T08:30:56Z

tests/components/onewire/test_options_flow.py

+
+    # Verify that first config step comes back with a selection list of all the 28-family devices
+    with patch(
+        "homeassistant.helpers.device_registry.DeviceRegistry.async_get_device",


We shouldn't patch the device registry. Create a device in the device registry before starting the test instead.

MartinHjelmare · 2022-03-04T08:32:37Z

tests/components/onewire/test_options_flow.py

+    config_entry: ConfigEntry,
+):
+    """Test that SysBus options flow aborts on init."""
+    hass.data[DOMAIN] = {config_entry.entry_id: FakeOWHubSysBus()}


Don't interact with integration details in the tests. Set up the integration instead with a MockConfigEntry, while patching appropriately to avoid I/O.

homeassistant added the cla-signed label Jan 17, 2022

probot-home-assistant bot added integration: onewire new-feature labels Jan 17, 2022

probot-home-assistant bot assigned epenet Jan 17, 2022

droberg mentioned this pull request Jan 17, 2022

Add support for DS2760 1-Wire TypeK Thermocouple #63222

Merged

22 tasks

droberg mentioned this pull request Jan 18, 2022

Add entity options to entity registry #64350

Merged

22 tasks

epenet mentioned this pull request Jan 26, 2022

Move onewire raw_value attribute to separate sensor #61093

Closed

22 tasks

droberg force-pushed the onewire-precision-config-ds18b20 branch from 00275ac to 8742148 Compare January 26, 2022 19:27

epenet reviewed Jan 26, 2022

View reviewed changes

epenet reviewed Jan 27, 2022

View reviewed changes

droberg force-pushed the onewire-precision-config-ds18b20 branch from 3beac32 to 20e7d69 Compare January 30, 2022 12:43

droberg force-pushed the onewire-precision-config-ds18b20 branch from 88a8729 to 28273f2 Compare February 22, 2022 20:51

epenet approved these changes Feb 23, 2022

View reviewed changes

epenet added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 23, 2022

balloob reviewed Feb 23, 2022

View reviewed changes

homeassistant/components/onewire/config_flow.py Outdated Show resolved Hide resolved

balloob removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 23, 2022

balloob approved these changes Feb 23, 2022

View reviewed changes

droberg closed this Feb 23, 2022

droberg reopened this Feb 23, 2022

balloob modified the milestone: 2022.3.0 Feb 23, 2022

droberg and others added 9 commits March 2, 2022 20:25

Use lamda expression for override_key in sensor config

b2a8a09

Add config flow for selecting precision of DS18B20 devices

15dae1a

Adjust config flow

46aafdd

Adjust description

9bc731a

Remove print

1490f8a

Fix missing get-call in get_current_setting

db51c29

Fix json-file and english translation

a472873

Remove extra call to dict.keys

9cf696b

Switch key<->value order in precision options mapping

8bb3066

droberg force-pushed the onewire-precision-config-ds18b20 branch from a62280b to 8bb3066 Compare March 2, 2022 20:27

epenet approved these changes Mar 3, 2022

View reviewed changes

balloob merged commit cfed1ff into home-assistant:dev Mar 3, 2022

MartinHjelmare reviewed Mar 4, 2022

View reviewed changes

github-actions bot locked and limited conversation to collaborators Mar 5, 2022

droberg deleted the onewire-precision-config-ds18b20 branch April 16, 2022 14:53

Uh oh!

Conversation

droberg commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant bot commented Jan 17, 2022

Uh oh!

thecode commented Jan 17, 2022

Uh oh!

epenet commented Jan 17, 2022

Uh oh!

epenet commented Jan 18, 2022

Uh oh!

epenet commented Jan 18, 2022

Uh oh!

droberg commented Jan 18, 2022

Uh oh!

droberg commented Jan 18, 2022

Uh oh!

epenet commented Jan 19, 2022

Uh oh!

emontnemery commented Jan 21, 2022

Uh oh!

epenet commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emontnemery commented Jan 21, 2022

Uh oh!

droberg commented Jan 22, 2022

Uh oh!

droberg commented Jan 26, 2022

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

droberg commented Jan 26, 2022

Uh oh!

epenet commented Jan 26, 2022

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

droberg commented Jan 30, 2022

Uh oh!

droberg commented Jan 30, 2022

Uh oh!

epenet commented Jan 30, 2022

Uh oh!

droberg commented Jan 30, 2022

Uh oh!

droberg commented Feb 22, 2022

Uh oh!

epenet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

droberg commented Feb 23, 2022

droberg commented Jan 17, 2022 •

edited

Loading

epenet commented Jan 21, 2022 •

edited

Loading

droberg commented Mar 2, 2022 •

edited

Loading

MartinHjelmare Mar 4, 2022 •

edited

Loading