h4de5 / home-assistant-toshiba_ac

Toshiba AC integration into home-assistant.io
GNU General Public License v3.0
113 stars 17 forks source link

Entity descriptors, 8 °C mode, fireplace, outdoor unit silent #124

Closed juliekoubova closed 1 year ago

juliekoubova commented 1 year ago

⚠️ Includes changes from #123

Concern: Unavailable Switches

SUMMARY: Going with unavailable, seems safer and less weird if the switch sometimes did multiple things. It shouldn't be a problem to set the correct mode and then the switch in automations and scripts, I hope.

operinko commented 1 year ago

After a quick test, 8C mode seems to work perfectly for me. My device doesn't, however, have the AirPurifier/PURE mode.

As for the concerns: I don't know how the Toshiba AC mobile app or remote deal with this situation, but I'd think this should work in the same way as they've set it up. In the app, if the device is set to Dry or Cool, 8C mode is unavailable and cannot be toggled. If 8C mode is on when the device is moved to any other state that can't co-exist with it, it turns off but is remembered when going back to a supported operating mode (Dry => Heat, 8C is back on). The remembering part might be done on the AC unit too, since the same seems to be true when using the actual IR remote.

Can't test how Ion/Pure mode works with that, though.

operinko commented 1 year ago

I'd rather not have the switch do multiple things sometimes (as in, if we're on Dry mode and I toggle the 8C switch, I wouldn't want it to turn off Dry mode and then turn on 8C Mode)

juliekoubova commented 1 year ago

image

tetienne commented 1 year ago

@juliekoubova Can you change the base branch of this PR to your other one? So we can see only the changes related to this feature?

labr3y commented 1 year ago

I've tested this branch, everything is working fine from the few tests I did.

The only thing not working (but probably because it's not yet implemented is merit_a_feature: HIGH_POWER.

juliekoubova commented 1 year ago

The only thing not working (but probably because it's not yet implemented is merit_a_feature: HIGH_POWER.

yup I haven't added HIGH_POWER and ECO yet. Should we add it as another select? That would be easiest and it would make most sense IMO. You can have Power 50 + HIGH_POWER, for example, even though I have no idea what it ends up doing.

operinko commented 1 year ago

Hmh, had some issues with the PyPI library not getting properly installed for some reason, but a quick uninstall, reboot and reinstall later it's up.

Outdoor temperature is reported as it should be, outdoor unit silent mode, fireplace mode and 8C mode all work as they should and the switches and select lists get disabled when the climate control is in a non-supported mode for them like "Dry".

Thumbs up from me!

labr3y commented 1 year ago

The only thing not working (but probably because it's not yet implemented is merit_a_feature: HIGH_POWER.

yup I haven't added HIGH_POWER and ECO yet. Should we add it as another select? That would be easiest and it would make most sense IMO. You can have Power 50 + HIGH_POWER, for example, even though I have no idea what it ends up doing.

On my unit : High Power, ECO and Outdoor Silent Unit disable each other. You can only select one of those (but you can select Power 50% + High Power).

8°C and Fireplace are also working on the same level, you can only have one or the other.

juliekoubova commented 1 year ago

@juliekoubova Can you change the base branch of this PR to your other one? So we can see only the changes related to this feature?

I'm not sure if it's possible to change the PR, that branch is in my fork. The diff from that branch is here:

https://github.com/juliekoubova/home-assistant-toshiba_ac/compare/move-to-base...juliekoubova:home-assistant-toshiba_ac:entity-desc

h4de5 commented 1 year ago

sorry i am still not familar with how github presents PRs. i see some of the comments are still unresolved. in case you are waiting for me to click merge, please let me know ;)

operinko commented 1 year ago

The only unresolved conversation is regarding class-typing, and the way it is is fine in my opinion. And regardless, I think that's the kind of change that would be better off in it's own branch.

juliekoubova commented 1 year ago

Yep if @tetienne is okay with this, I think we can merge it :)

h4de5 commented 1 year ago

thank you very much for the contribution and your time that you put into this. i'll try it out as soon as possible

juliekoubova commented 1 year ago

thank you!