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

Allow sensors to have custom settings #939

Merged
merged 5 commits into from Sep 18, 2020

Conversation

dshokouhi
Copy link
Member

@dshokouhi dshokouhi commented Sep 15, 2020

This PR allows any sensor to have a custom setting to fit its need. A table identical to attributes is created and most of the logic around the sensor detail fragment is the same too with the exception that we use a EditTextPreference to get the value for the manager to handle. I chose to keep it separate from attributes since these are really more app specific but I think we can allow the managers to provide them as an attribute if we see the need.

With this PR each sensor manager is responsible for:

  • Updating its BasicSensor.description to describe each setting
  • Creating the setting in the table and in proper order
  • Handling null or unwanted values
  • Setting valueType: "number" or valueType: "string" to handle a dynamic keyboard change
  • Setting valueType: "toggle" if they require a toggle to enable a setting
  • Setting a sensible default if required.

For this PR I have chosen wifi_bssid to have a custom setting. The user must first enable the toggle get_next_bssid then and only then will we create a new field to rename the current BSSID. The user will need to enable the toggle again if they want to rename another BSSID, the toggle will turn itself off after creating one field. In my house I have 2 AP's so I have chosen to send Den or Bedroom depending on the connected mac address. This will negate the need to use a template sensor and without the need to use secret values to mask it.

Things to be worked in a future PR would be to allow a user to hide a setting and possibly reset them. Later on we can use valueType to also dictate if instead of a EditTextPreference or SwitchPreference to use something like Mult-select List or Checkbox etc... I think for now we should stick to some small basics for an MVP. In the current state it should allow a wide variety of things to be done including #890

image

image

image

@anyuta1166
Copy link
Contributor

anyuta1166 commented Sep 15, 2020

Any mac address found by this sensor will have a setting in place so the user can replace the mac address with something thats more meaningful to them

Is there an ability to delete a setting? If a user connects to a large number of BSSIDs, he will end up with dozens of settings.

For example, I often use public Wi-Fi network. SSID is always the same, but each bus, bus station itself, train and subway cars have their own BSSID. So each ride through the city will end up in a dozen of new settings.

@anyuta1166
Copy link
Contributor

In the current state it should allow a wide variety of things to be done including #890

Great, but I prefer my own solution. Your solution means there will be 3 different settings (for background location, zone location and geocoded location), hidden deeper inside the sensor settings.

@anyuta1166
Copy link
Contributor

anyuta1166 commented Sep 15, 2020

I advise to make a setting "Collect BSSIDs" or like that, which can be turned off (or better be off by default - because we already have SSID sensor and BSSID -> custom name translation should be for advanced users).

Even if there will be an ability to delete collected BSSIDs, it will be very inconvenient to regularly make a manual cleanup of settings.

@skynetua
Copy link
Contributor

Why do we need to collect them? As for me, we should store only user-added things into the one single string, like pairs - BSID1:ALIAS1, BSID2:ALIAS2. Or even better like json array.

@dshokouhi
Copy link
Member Author

Is there an ability to delete a setting?

I mentioned that we should look into doing this after this PR, for a MVP PR that will add to the complexity of things and we should keep it simple first. We need to think of a way that works for all sensors and their needs. Not all sensors will want a setting to be deleted, every sensor has a unique need.

Great, but I prefer my own solution. Your solution means there will be 3 different settings (for background location, zone location and geocoded location), hidden deeper inside the sensor settings.

This was already addressed in #890 and it was mentioned that they should be separated. Logically speaking inside sensor details, where you enable a sensor, is the best place to keep the settings as the description is there and we can properly translate it. These settings are meant to have a default value that is sensible so in most cases a user won't need to touch them.

(or better be off by default - because we already have SSID sensor and BSSID -> custom name translation should be for advanced users).

This sensor is already disabled by default and SSID is only enabled if the user grants location permissions, users have to explicitly enable this sensor. Many users would still consider BSSID, by itself, an advanced sensor so I don't see many enabling it.

Why do we need to collect them?

I mentioned the why in my post, its to help reduce the need for template sensors and secret values making the automation aspect more user friendly. Anyone who wants to automate off this will need to use a secret value or a secret inside a template sensor just to get the proper names displayed. I don't expect people to remember MAC addresses but I do expect them to know which ones they care about and to want to name them. In a household with multiple AP's it can be very helpful to see where a person is.

As for me, we should store only user-added things into the one single string, like pairs - BSID1:ALIAS1, BSID2:ALIAS2. Or even better like json array.

This greatly adds to the complexity for first release I was thinking only simple text fields so sensors can do what they need to do without getting too crazy initially. There won't be too many BSSID usage here only really for people who have multiple AP's and want to track them. Normal users only need to stick to the Wifi connection sensor as that contains the SSID name. Also creating a JSON array in a text field would not be user friendly.

@dshokouhi
Copy link
Member Author

Ok I have now added some more logic here to address the concerns raised around too many entries, thank you for that. I will update the original post but here is whats new:

Managers can now define valueType: "toggle" so they can create a toggle as needed. Managers are responsible for creating the toggle, creating the settings in a proper order that makes sense so if the toggle should be the first option it should be created first. The default should be false.

The sensor detail fragment will handle creating the toggle based on the valueType and also will handle setting the value to "true" and storing it so the manager can continue with its logic.

So for wifi_bssid the flow will be user enables the toggle, next update we set the toggle to off and create one new field for the current BSSID. User can then rename the BSSID as they see fit. Then the next time they encounter a BSSID they want to rename they will need to enable the toggle again to grab the current value and store it.

@JBassett JBassett merged commit e652696 into home-assistant:master Sep 18, 2020
@dshokouhi dshokouhi deleted the sensor_settings branch March 24, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants