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

Issues with mode == auto for a Whirlpool protocol AC #1283

Closed
ayavilevich opened this issue Sep 26, 2020 · 7 comments · Fixed by #1284
Closed

Issues with mode == auto for a Whirlpool protocol AC #1283

ayavilevich opened this issue Sep 26, 2020 · 7 comments · Fixed by #1284
Assignees

Comments

@ayavilevich
Copy link

Version/revision of the library used

2.7.10 + Issue1275

Expected and Actual behavior

In continuation of #1275

After you addressed the "power toggle" issue for Airwell and Whirlpool ACs, we done some testing.
@the-mentor tested the new version on a Tornado branded AC using Whirlpool protocol. He wasn't able to turn the AC off.
We are using home-assistant (HA) + tasmota-ir.

In the initial state, the AC is working and HA is showing that the AC is working, so they are in sync.
Next, we click "off" in HA generating:

13:56:37 MQT: stat/ir_study/RESULT = {"IRHVAC":{"Vendor":"WHIRLPOOL_AC","Model":2,"Power":"Off","Mode":"Off","Celsius":"On","Temp":24,"FanSpeed":"Auto","SwingV":"Off","SwingH":"Off","Quiet":"Off","Turbo":"Off","Econo":"Off","Light":"On","Filter":"Off","Clean":"Off","Beep":"Off","Sleep":-1}}
13:56:37 MQT: tele/ir_study/RESULT = {"IrReceived":{"Protocol":"WHIRLPOOL_AC","Bits":168,"Data":"0x0x830604810000800000000000000500010000080009","Repeat":0,"IRHVAC":{"Vendor":"WHIRLPOOL_AC","Model":2,"Power":"Off","Mode":"Off","Celsius":"On","Temp":24,"FanSpeed":"Auto","SwingV":"Off","SwingH":"Off","Quiet":"Off","Turbo":"Off","Econo":"Off","Light":"On","Filter":"Off","Clean":"Off","Beep":"Off","Sleep":-1}}}

the AC doesn't beep in response and ignores the signal.

When turning the AC off with the remote in same state we get:

14:09:46 MQT: tele/ir_study/RESULT = {"IrReceived":{"Protocol":"WHIRLPOOL_AC","Bits":168,"Data":"0x0x830604820000861F00000000001F00010000080009","Repeat":0,"IRHVAC":{"Vendor":"WHIRLPOOL_AC","Model":2,"Power":"Off","Mode":"Off","Celsius":"On","Temp":24,"FanSpeed":"Auto","SwingV":"Off","SwingH":"Off","Quiet":"Off","Turbo":"Off","Econo":"Off","Light":"On","Filter":"Off","Clean":"Off","Beep":"Off","Sleep":-1}}}

comparing data:
0x0x830604810000800000000000000500010000080009 - bad
0x0x830604820000861F00000000001F00010000080009 - good

the cause of the issue is byte 3.
81 means mode=auto where 82 means mode=cool.

This AC doesn't have mode=auto as an option.
Sending IRHVAC with mode=auto will also cause the AC to ignore the command.

note that power toggle is applied correctly, see byte 2 == 04.

ps. data has 0x0x as prefix.

The issue seems to be that when IRremote is asked to sendAc where mode==off then it will send mode=auto.
To confirm the issue I changed IRWhirlpoolAc::convertMode to return kWhirlpoolAcCool as the default and it works fine now.
IRWhirlpoolAc::convertMode doesn't currently have a case for stdAc::opmode_t::kOff.

What would be the right way to fix this? I am not sure if convertMode should handle kOff. Should all AC implementations handle mode=off? how about mode=auto?

Please advise.

What brand/model IR demodulator are you using?

YTF IR Brifge

Steps to reproduce the behavior

See above. Need this specific AC.

Has this library/code previously worked as expected for you?

No

Other useful information

More information is always welcome. Be verbose.

@crankyoldgit
Copy link
Owner

This AC doesn't have mode=auto as an option.

@the-mentor What brand/model AC & remote is this please? We may need to create a new model number to enforce the "no auto mode" for your a/c.

The issue seems to be that when IRremote is asked to sendAc where mode==off then it will send mode=auto.
To confirm the issue I changed IRWhirlpoolAc::convertMode to return kWhirlpoolAcCool as the default and it works fine now.
IRWhirlpoolAc::convertMode doesn't currently have a case for stdAc::opmode_t::kOff.

What would be the right way to fix this? I am not sure if convertMode should handle kOff. Should all AC implementations handle mode=off? how about mode=auto?

convertMode() only handles modes supported by the A/Cs in question. It will however return a mode (default) when it is passed a mode the A/C doesn't support. This mode is typically kAuto because that often is the best choice/decision.
Perhaps the best solution(hack) is to make kCool the default for IRWhirlpoolAc::convertMode. (Ah, I see you've already done that).
Longer term is to forbid an Auto mode for this particular A/C model.

As you've indicated, this stems from Home Assistant insisting that climates have an "Off" mode via MQTT. The IR library doesn't natively support an "Off" mode, as that's done via the power setting. (I think we've only ever hit one remote with an "Off" mode) That means the "glue layer" joining HA to the IR library needs to handle this conversion. e.g. Tasmota, IRMQTTServer, etc. I'm happy to assist in making the library to do the best thing when the "glue layer" stuffs up (e.g. Default to cool instead for this A/C), but the library won't be taking on mapping any/all home automation systems seamlessly. That is best done in the middle-ware layer, and not cluttering up the library with code most users don't need.

ps. data has 0x0x as prefix.

That's Tasmota issue I believe, not this library's.

Anyway. I'll look into it now to see what I can do. But, the better fix is going to require setting a model for this device to enforce/limit it from producing bad codes, if the user insists on producing them. :)

crankyoldgit added a commit that referenced this issue Sep 27, 2020
Apparently some models of Whirlpool don't have Auto mode, and don't respond to commands with Auto mode set. So change behaviour of `convertMode()` when handling unexpected modes to return Cool. e.g. If passed `kOff`.

* Add unit test to confirm behaviour.
* Update unit test style issue. (kGpioUnused)
* Add missing Housekeeping unit tests.
* make `convertFan()` & `convertMode()` to static.

Fixes #1283
@crankyoldgit crankyoldgit added enhancement Pending Confirmation Waiting for confirmation from user labels Sep 27, 2020
@crankyoldgit
Copy link
Owner

@ayavilevich @the-mentor Can you please try PR #1284 to see if that solves your immediate problem?

@ayavilevich
Copy link
Author

@crankyoldgit
agree with what you wrote above.

It would be better designed to have a separate model but I don't see any difference in the code that will allow detection of the model. This will mean this model will remain "secret" to the user. So user will think he is model=2 when receiving but in fact he will have to select a different one for sending.

The PR you made is good balance. It is simple change but should make it work. I will make a build and we will test it again.

Regarding the AC, we were not been able to find a model number such as DG11J191. The remote says nothing.
This is a Tornado branded, multi-inverter, central AC with several indoor units. Tornado don't make their own, so this is a white-label product.
If you have an idea on how we can find more info, please let us know.

@ayavilevich
Copy link
Author

@the-mentor
Copy link

@the-mentor please try build https://github.com/ayavilevich/Tasmota/releases/tag/v0.3

@ayavilevich @crankyoldgit I can confirm that everything is working as expected in the test build.

-DM

@crankyoldgit crankyoldgit removed the Pending Confirmation Waiting for confirmation from user label Sep 27, 2020
@crankyoldgit
Copy link
Owner

It would be better designed to have a separate model but I don't see any difference in the code that will allow detection of the model. This will mean this model will remain "secret" to the user. So user will think he is model=2 when receiving but in fact he will have to select a different one for sending.

That's not strictly true. It could be made visible.
Assuming mode 2 is the model with "auto" mode, and model 3 is "Model 2 without Auto".
I can change everything currently model 2 to 3 and if it detects the code for "Auto" report that as Model 2.

Or alternatively, it could be ... in the documentation/src file etc. ;-)

Regarding the AC, we were not been able to find a model number such as DG11J191. The remote says nothing.
This is a Tornado branded, multi-inverter, central AC with several indoor units. Tornado don't make their own, so this is a white-label product.
If you have an idea on how we can find more info, please let us know.

The model number could appear different.
For the remote, try looking in the battery compartment, or if your super-game the PCB inside the remote.
As for the A/C unit. Normally there is a compliance plate somewhere on the "external" unit. For the internal units, sometimes it behind the door/cover you where you change/clean the filter.
Other than that ... sales invoice?!

Not a real drama. We've got a solution for now, but if you find it. We could at least make it harder for someone to produce an invalid code.

I can confirm that everything is working as expected in the test build.

@the-mentor Thanks for the confirmation.

crankyoldgit added a commit that referenced this issue Sep 27, 2020
Apparently some models of Whirlpool don't have Auto mode, and don't respond to commands with Auto mode set. So change behaviour of `convertMode()` when handling unexpected modes to return Cool. e.g. If passed `kOff`.

* Add unit test to confirm behaviour.
* Update unit test style issue. (kGpioUnused)
* Add missing Housekeeping unit tests.
* make `convertFan()` & `convertMode()` to static.

Fixes #1283
crankyoldgit added a commit that referenced this issue Oct 2, 2020
_v2.7.11 (20200902)_

**[Features]**
- Transcold: Add detailed support. (#1256 #1278)
- Airwell/Whirlpool: Add handling of previous state to `.toCommon()` (#1275 #1276)
- IRMQTTServer: Change how MQTT packet/buffer size is set. (#1271)
- Fujitsu: Add support for timers. (#1255 #1261 #1262)
- Neoclima: Add Economy & Fahrenheit support (#1260 #1265)
- Technibel: Cleanup and code fixes/improvements. (#1259 #1266)
- Technibel: Add detailed A/C support (#1259)
- Transcold: Add basic support. (#1256 #1258)

**[Misc]**
- refactor ir_Delonghi (#1285)
- Whirlpool: Change default mode in `convertMode()` (#1283 #1284)
- SamsungAC: Unit tests to help debug poor signal (#1277 #1280)
- Add question & note about VS1838b use to issue template. (#1281)
- rewrite ir_Corona (#1274)
- tools/mkkeywords: Fix minor parsing issue. (#1272)
- Add Zhongxian Li to Contributers.md (#1270)
- rewrite Carrier (#1269)
- rewrite ir_Argo by using bit field (#1264)
- rewrite ir_Amcor by using bit field (#1263)
- Update Fujitsu supported model info.
- Clarify the scope of the LittleFS breaking change.
crankyoldgit added a commit that referenced this issue Oct 2, 2020
_v2.7.11 (20201002)_

**[Features]**
- Transcold: Add detailed support. (#1256 #1278)
- Airwell/Whirlpool: Add handling of previous state to `.toCommon()` (#1275 #1276)
- IRMQTTServer: Change how MQTT packet/buffer size is set. (#1271)
- Fujitsu: Add support for timers. (#1255 #1261 #1262)
- Neoclima: Add Economy & Fahrenheit support (#1260 #1265)
- Technibel: Cleanup and code fixes/improvements. (#1259 #1266)
- Technibel: Add detailed A/C support (#1259)
- Transcold: Add basic support. (#1256 #1258)

**[Misc]**
- refactor ir_Delonghi (#1285)
- Whirlpool: Change default mode in `convertMode()` (#1283 #1284)
- SamsungAC: Unit tests to help debug poor signal (#1277 #1280)
- Add question & note about VS1838b use to issue template. (#1281)
- rewrite ir_Corona (#1274)
- tools/mkkeywords: Fix minor parsing issue. (#1272)
- Add Zhongxian Li to Contributers.md (#1270)
- rewrite Carrier (#1269)
- rewrite ir_Argo by using bit field (#1264)
- rewrite ir_Amcor by using bit field (#1263)
- Update Fujitsu supported model info.
- Clarify the scope of the LittleFS breaking change.
siriuslzx pushed a commit that referenced this issue Oct 4, 2020
* Regenerate Doxygen documentation

* v2.7.11 release
_v2.7.11 (20201002)_

**[Features]**
- Transcold: Add detailed support. (#1256 #1278)
- Airwell/Whirlpool: Add handling of previous state to `.toCommon()` (#1275 #1276)
- IRMQTTServer: Change how MQTT packet/buffer size is set. (#1271)
- Fujitsu: Add support for timers. (#1255 #1261 #1262)
- Neoclima: Add Economy & Fahrenheit support (#1260 #1265)
- Technibel: Cleanup and code fixes/improvements. (#1259 #1266)
- Technibel: Add detailed A/C support (#1259)
- Transcold: Add basic support. (#1256 #1258)

**[Misc]**
- refactor ir_Delonghi (#1285)
- Whirlpool: Change default mode in `convertMode()` (#1283 #1284)
- SamsungAC: Unit tests to help debug poor signal (#1277 #1280)
- Add question & note about VS1838b use to issue template. (#1281)
- rewrite ir_Corona (#1274)
- tools/mkkeywords: Fix minor parsing issue. (#1272)
- Add Zhongxian Li to Contributers.md (#1270)
- rewrite Carrier (#1269)
- rewrite ir_Argo by using bit field (#1264)
- rewrite ir_Amcor by using bit field (#1263)
- Update Fujitsu supported model info.
- Clarify the scope of the LittleFS breaking change.
@crankyoldgit
Copy link
Owner

FYI, the changes mentioned above have now been included in the new v2.7.11 release of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants