-
-
Notifications
You must be signed in to change notification settings - Fork 679
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
New thermostat tile for Wear OS #4959
base: master
Are you sure you want to change the base?
New thermostat tile for Wear OS #4959
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Martreides
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Here's a tip: running Don't forget to mark the PR as ready to review when you're done. |
Thanks for the tip! Before my last commit I had one issue left, which should be fixed in my latest commit. Good to know for future commits though! |
So just a thought but maybe we should call this a Climate Tile instead of Thermostat tile to be more precise? Users can add any climate entity but does not specifically need to be a thermostat? |
|
||
serverManager.integrationRepository().callAction( | ||
entityStr.split(".")[0], | ||
"set_temperature", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we need to do a state check to prevent a possible error?
https://www.home-assistant.io/integrations/climate/#action-climateset_temperature
New target temperature for climate device (commonly referred to as a setpoint). Do not use if hvac_mode is heat_cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I wasn't aware of that. I will add a state check later today to handle that. Any best practice on what to do if that hvac_mode is heat_cool? From the docs I understand that in that case both target_temp_high and target_temp_low should be provided, but that is not supported by the tile currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm might be best to skip that state for now 🤔 and maybe just hide controls? Dont think it makes sense at all to set both low and high to the same value right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any device myself that has a high and low. But, I can imagine that it might not be good to set those to the same value. I guess it would always be either cooling or heating because it's never between the min and max 😅.
That's something I have been thinking about as well. In the end I went with Thermostat Tile since it only functions as a thermostat. The docs write that climate entities can "control temperature, humidity, or fans, such as A/C systems and humidifiers." Since this tile only controls temperature and nothing else I was afraid that Climate Tile might be too broad. One reason I can think of for going with Climate Tile is to accommodate future additions. Note that in the code I did try to be clear that the entity being used is the climate entity. |
Thanks for clarifying it does make sense in the current use case however one thing to consider is that later on changing the name may lead to a breaking change as the tile path is in the manifest. Changing that later may have the undesired effect of removing the tile as the path can no longer be referenced. With that said I can see that multiple tiles are allowed to be added for this tile. Maybe we should consider showing the entity friendly name or icon to help distinguish between multiple tiles? Also did you consider showing the state? Might be helpful to know if the thermostat is on for example? Thanks for the PR as well 🙏 , looks like this will let us close #2555 based on current functionality. |
Looks like we also need to check if the climate entity supports changing the temp We do already have some of that logic in our device controls implementation you may refer to |
I guess we can combine your earlier comment into this as well right? We do one check to determine whether the entity can be controlled by this tile and use that throughout the code. I will have to dive into this for a bit. I see my own climate entity reports as 385, which is target_temperature, turn_off and turn_on. Since the ClimateControl code that implements this is part of the phone app I guess we cannot use any part of it directly right?
I don´t entirely follow what you mean here. Do you mean to show the name or icon on the tile?
Ideally I would like to show it the same as in the browser, it shows the state and glows if heating. The glowing is a bit too complicated for me at this point, but I think the state should be quite easy to add.
Nice, I wasn't aware that issue existed! |
Yup that sounds like a good approach
So not directly but you can move some of that logic to our Entity extensions and then reuse that elsewhere. Helps keep all that business logic in one place to easily reference :)
Yes something to help differentiate between multiple tiles being added otherwise its a guessing game :) Imagine a user adding a tile for upstairs and downstairs thermostats how do they tell them apart quickly
Hmm maybe instead of glow we match the frontend color? They seem to use orange for heat, blue for cool and white for idle/off. We typically try to match frontend behavior to help meet user expectations |
…ing) based on which the font changes color. Also includes the friendly name of the entity at the op.
The last commits contains changes to the layout of the tile. It now shows the friendly name of the entity at the top. I'm not completely sure on whether each entity has a friendly name though. It also shows the state of the entity (Idle, Heating or Cooling), based on the state the color of the state and the actual temperature changes (this reflects the behaviour of the card). I think it would be nice as well if we add the unit (C or F) to the tile. However, I am struggling with figuring out how to determine whether to use C or F. In the docs I do see a required field temperature_unit, but at least my climate entity does not have it. Edit: also changed the colour of the buttons to use the color from the HA theme. |
Latest screenshot looks great! Does make you wonder how closely the tile should try to match the frontend here. Hard to consider things here too because we cant scroll so we have limited space
We actually handle that by defaulting to entity ID if not provided
hmm mine does not either, maybe for the unit it could be best to go by the config response because on my end i have no control over the unit like we can with sensors |
Indeed! I think this looks good, but we should not make it more crowded.
Great! I am thinking about adding a setting so users can choose whether to show the name. Myself I would prefer it without as I only have a single climate entity. That should be quite easy to add though.
Thanks! Will have a look at that and the other changes tomorrow evening. |
I agree however I cannot escape the thought of users asking about controlling the state too 🙈 . For this PR not necessary as the MVP solves a good use case, but in the future we can consider a button to toggle through the states just like we do with device controls. Mentioning this here so others can see we discussed it. Given the state of the app its not required for this tile as users can still get by using scripts and such on the watch for state control Should we consider updating the UI to match the frontend when the device is off? State control seems to only be allowed when it is Side note: we can probably adapt the changes here for other domains like media player, number etc... for additional tiles in the future :) Great work!! |
I had to reshuffle the code a bit. If no entity was selected the code would fail before reaching the point at which it would output the "select entity" content. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have time tomorrow I would like to do some test on emulated devices with different screens (sizes and shapes).
Did you end up testing on other screen sizes? Mainly interested in small screens because you are hardcoding quite a lot of units for spacing and the screenshot looks like it just fits and is not the smallest screen.
wear/src/main/java/io/homeassistant/companion/android/home/views/SelectThermostatTileView.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/home/views/SetThermostatTileView.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/home/views/SettingsView.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
Realistically no one is going to have a square Wear OS watch right now so the current layout looks fine to me. |
) | ||
).build() | ||
} catch (e: Exception) { | ||
Log.e(TAG, "Unable to fetch entity ${tileConfig?.entityId}", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log.e(TAG, "Unable to fetch entity ${tileConfig?.entityId}", e) | |
Log.e(TAG, "Unable to fetch entity ${tileConfig.entityId}", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't really see at what point it becomes okay to stop using safe calls. Android Studio keep giving me a lot of warning with the current code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok once you're sure that the variable is not null
. In this case: because a null
tileConfig would be handled by the if statement in line 86.
Not sure why your Android Studio isn't showing you these and is actively showing warnings, I can see how it might be unintuitive then. Search results online suggest power save mode should be off and/or invalidating caches might help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding this, but isn't it so that if tileConfig is null the if statement would immediately skip to the else part? If so, it could still be null in the else clause of the if/else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, isNullOrBlank()
also applies if tileConfig
is null. Here's a simplified example that shows it: https://pl.kotl.in/zyrFZF9_c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it! I was under the impression isNullOrBlank()
only applied to entityId
. Thanks!
) | ||
.build() | ||
) | ||
if (tileConfig.showEntityName == true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to show when loading the entity data failed? Either one is fine in my opinion.
- Nothing: this should also check that
entity != null
. If the entity isnull
the entire text arc will be skipped. - Entity ID: tshould also check that
tileConfig.entityId != null
, so you can use that as a fallback in line 278 when the entity couldn't be loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I see it correctly, the changes made in 35d6fd5 make sure that this part of the code is never reached when the entity is null.
val entity = tileConfig.entityId?.let { | ||
serverManager.integrationRepository().getEntity(it) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're fetching the entity again, while it was already fetched in line 100-102. Try passing in the received entity to the function to avoid another network call. Maybe you can even say it is never null
in this function which would clean up the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could I pass the entity to the timeline function? I'm struggling a bit with the type of the parameter. The following seems to not work:
private suspend fun timeline(tileConfig: ThermostatTile, entity: Entity
I also tried entity: Entity<T>
which I saw somewhere else but that also does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to add it all to the current line 189 you can do:
private suspend fun timeline(tileConfig: ThermostatTile, entity: Entity<Map<String, Any>>, targetTemperature: Float): Timeline = Timeline.fromLayoutElement(
(but given the length adding a newline after = is probably nice/ktlint might reformat)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now also use check(entity != null)
to be able to say entity is never null in the timeline function.
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
wear/src/main/java/io/homeassistant/companion/android/tiles/ThermostatTile.kt
Outdated
Show resolved
Hide resolved
Latest changes looking good here, no crashes on various states of my thermostat and the controls work as expected. |
val updatedTargetTemp = targetTemp + if (lastId == TAP_ACTION_UP) +stepSize.toFloat() else -stepSize.toFloat() | ||
|
||
serverManager.integrationRepository().callAction( | ||
entityStr.split(".")[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use entity.domain
(+ import io.homeassistant.companion.android.common.data.integration.domain
) instead of splitting the string manually
entityStr.split(".")[0], | ||
"set_temperature", | ||
hashMapOf( | ||
"entity_id" to entityStr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use entity.entityId
directly, the extra variable isn't needed
var hvacAction = entity.attributes["hvac_action"].toString() | ||
val state = entity.state | ||
val hvacActionColor = when (hvacAction) { | ||
"heating" -> getColor(R.color.colorDeviceControlsThermostatHeat) | ||
"cooling" -> getColor(R.color.colorDeviceControlsDefaultOn) | ||
else -> 0x00000000 | ||
} | ||
|
||
hvacAction = when (state) { | ||
"off" -> "off" | ||
"unavailable" -> "off" | ||
else -> hvacAction | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing hvacAction
to off
when unavailable
, also makes it look like unavailable climate entities are off (see: friendlyHvacAction
), that seems unwanted to me? Instead you should just edit line 259/264 to also disable the buttons when unavailable
. (The code suggestion assumes you'll edit those lines.)
Try to avoid a var
if you immediately reassign it.
var hvacAction = entity.attributes["hvac_action"].toString() | |
val state = entity.state | |
val hvacActionColor = when (hvacAction) { | |
"heating" -> getColor(R.color.colorDeviceControlsThermostatHeat) | |
"cooling" -> getColor(R.color.colorDeviceControlsDefaultOn) | |
else -> 0x00000000 | |
} | |
hvacAction = when (state) { | |
"off" -> "off" | |
"unavailable" -> "off" | |
else -> hvacAction | |
} | |
val hvacAction = entity.attributes["hvac_action"].toString() | |
val hvacActionColor = when (hvacAction) { | |
"heating" -> getColor(R.color.colorDeviceControlsThermostatHeat) | |
"cooling" -> getColor(R.color.colorDeviceControlsDefaultOn) | |
else -> 0x00000000 | |
} |
val entityStr = entity.entityId | ||
var stepSize = entity.attributes["target_temp_step"] | ||
|
||
stepSize = if (stepSize == null && temperatureUnit == "°F") { | ||
1.0f | ||
} else if (stepSize == null) { | ||
0.5f | ||
} else { | ||
stepSize.toString().toFloat() | ||
} | ||
|
||
val updatedTargetTemp = targetTemp + if (lastId == TAP_ACTION_UP) +stepSize.toFloat() else -stepSize.toFloat() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid a var
if you immediately reassign it, and don't convert floats to floats again ;)
val entityStr = entity.entityId | |
var stepSize = entity.attributes["target_temp_step"] | |
stepSize = if (stepSize == null && temperatureUnit == "°F") { | |
1.0f | |
} else if (stepSize == null) { | |
0.5f | |
} else { | |
stepSize.toString().toFloat() | |
} | |
val updatedTargetTemp = targetTemp + if (lastId == TAP_ACTION_UP) +stepSize.toFloat() else -stepSize.toFloat() | |
val attrStepSize = (entity.attributes["target_temp_step"] as? Number)?.toFloat() | |
val stepSize = attrStepSize ?: if (temperatureUnit == "°F") 1.0f else 0.5f | |
val updatedTargetTemp = targetTemp + if (lastId == TAP_ACTION_UP) +stepSize else -stepSize |
I'm away for the week, will have a look at the last comments as soon as I'm back. |
Summary
This pull request is intended to add a thermostat tile to the Wear OS app. It shows the target and current temperature, and allows a user to change the target temperature. The high level set up of the implementation is based on the existing tiles, mostly the camera tile.
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#1155
Any other notes
This is my first contribution to an Android project. A good review of the PR is appreciated.