MITSUBISHI_AC: Added support for i-SAVE mode. #1666

ratnick · 2021-11-07T15:34:49Z

This mode puts the A/C in an energy saving mode, maintaining the temp at 10C.

…Z-FHnnVE A/C. Not yet tested if it can be sent to A/C.

No use of common Econo mode. Instead a new mode iSave10C is made.

crankyoldgit

Btw, thanks for doing this, and providing a PR for it. That's great.

Sorry about all the comments/feedback. This is typical for a first-time contributor to this project, so don't feel bad or discouraged. We have lots & lots of automated checks & tests to keep things consistent & keep everything working, hence why there are a tonne of things. The tests are there so we don't stuff anything up, and we ourselves trip over them all the time so it's completely normal.

Keep it up. You picked a fairly well established project to make your first ever Contribution/PR too. So this would be rougher than most people's first contribution. Sorry.

Also remove "tools/analysis of bytes from Mitshubishi MSZ-FHnnVE AC remote.xlsm" file from the PR. Feel free to add it to the conversation stream for the PR etc, or as a link to a documented publicly etc, but not in the code repo that everyone will be downloading.

Linter issues found:

src/IRac.cpp:2926:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:459:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.cpp:459:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:460:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:461:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:462:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:463:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:464:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.h:93:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.h:93:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_Mitsubishi.h:280:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_Mitsubishi.h:281:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.h:281:  Should have a space between // and comment  [whitespace/comments] [4]

Unit tests now fail to compile as you've adjusted the mitsubishi() function in IRac.
See: https://github.com/crankyoldgit/IRremoteESP8266/runs/4135829425?check_suite_focus=true#step:4:129

and a Doxygen error:
https://github.com/crankyoldgit/IRremoteESP8266/runs/4135834351?check_suite_focus=true#step:4:66

crankyoldgit · 2021-11-08T07:25:49Z

src/IRac.cpp

@@ -1539,6 +1539,7 @@ void IRac::mirage(IRMirageAc *ac,
 /// @note Clock can only be set in 10 minute increments. i.e. % 10.
 void IRac::mitsubishi(IRMitsubishiAC *ac,
                      const bool on, const stdAc::opmode_t mode,
+                      const bool iSave10C,


Rename to "econo", and move it to after the "quiet" parameter.
You also need the special comment line above the function for doxygen to document it.
e.g.

/// @param[in] econo Toggle the device's economical mode.

crankyoldgit · 2021-11-08T07:26:04Z

src/IRac.cpp

@@ -1553,6 +1554,8 @@ void IRac::mitsubishi(IRMitsubishiAC *ac,
  ac->setVane(ac->convertSwingV(swingv));
  ac->setWideVane(ac->convertSwingH(swingh));
  if (quiet) ac->setFan(kMitsubishiAcFanSilent);
+  ac->setiSave10C(iSave10C);
+


Remove the blank line.

Please remove the blank line at line 1557

crankyoldgit · 2021-11-08T07:26:27Z

src/IRac.cpp

@@ -2920,7 +2923,7 @@ bool IRac::sendAc(const stdAc::state_t desired, const stdAc::state_t *prev) {
    case MITSUBISHI_AC:
    {
      IRMitsubishiAC ac(_pin, _inverted, _modulation);
-      mitsubishi(&ac, send.power, send.mode, degC, send.fanspeed, send.swingv,
+      mitsubishi(&ac, send.power, send.mode, send.econo, degC, send.fanspeed, send.swingv,


You will need to move this parameter per earlier comment.

crankyoldgit · 2021-11-08T07:29:05Z

src/ir_Mitsubishi.cpp

+/// Normal minimum temp is 16C, and i-SAVE mode works as a gate to enable AC to use 10C as setting. However, when Remote control shows 10C, it still emits 16C on the "Temp" bits, 
+/// and instead it uses other bits to indicate a target temp of 10C. Slightly strange, but I guess it's to keep compatibility to systems without i-SAVE.
+/// i-SAVE only has this 10C functionality when the AC is already in Heat mode. In all other modes, minimum temp is 16C.
+/// I have found no other difference between normal Heat mode and i-SAVE other than the ability to go to 10C.
+/// In this implementation, i-SAVE mode is ONLY used to enable the AC temperature setting to 10C. Therefore "Temp" is set to 16 disregarding what the remote shows, and mode is set to Heat.


Start the block of comments with /// @note
e.g.

/// @note Normal minimum temp is 16C, ...

Plus the linter will complain that these lines are over 80 chars long. Please line wrap them so they don't go over 80 chars.

crankyoldgit · 2021-11-08T07:30:12Z

src/ir_Mitsubishi.cpp

+/// I have found no other difference between normal Heat mode and i-SAVE other than the ability to go to 10C.
+/// In this implementation, i-SAVE mode is ONLY used to enable the AC temperature setting to 10C. Therefore "Temp" is set to 16 disregarding what the remote shows, and mode is set to Heat.
+/// 
+void IRMitsubishiAC::setiSave10C(bool iSave10C) {


void IRMitsubishiAC::setiSave10C(const bool iSave10C) {

crankyoldgit · 2021-11-08T07:31:35Z

src/ir_Mitsubishi.cpp

+  // Supported by MSZ-FH series
+//  result.iSave10C = getiSave10C();


Replace with:

result.econo = getiSave10C();

crankyoldgit · 2021-11-08T07:32:49Z

src/ir_Mitsubishi.h

@@ -88,7 +89,9 @@ union Mitsubishi144Protocol{
    // Byte 14
    uint8_t          :8;
    // Byte 15
-    uint8_t          :8;
+    uint8_t          :5;
+    uint8_t iSave10C :1;  //i-SAVE: 1 if in Heat mode AND iSave os on, and temperature THEN set to 10C


Needs a space after comment chars.
e.g.
uint8_t iSave10C :1; // i-SAVE: 1 if in Heat mode AND iSave os on, and temperature THEN set to 10C

crankyoldgit · 2021-11-08T07:33:16Z

src/ir_Mitsubishi.h

+  bool getiSave10C(void) const;    //1 = in i-Save mode and temp = 10C
+  void setiSave10C(bool);          //1 = set to i-Save mode when temp = 10C on remote. "Temp" byte is not affected by this(!). 10C temp is implied through other bits


Ditto previous comment. Space after the // chars.

crankyoldgit · 2021-11-08T07:34:02Z

src/ir_Mitsubishi.h

+  bool getiSave10C(void) const;    //1 = in i-Save mode and temp = 10C
+  void setiSave10C(bool);          //1 = set to i-Save mode when temp = 10C on remote. "Temp" byte is not affected by this(!). 10C temp is implied through other bits


Line to long (> 80 chars)
Please wrap the comments etc.

ratnick · 2021-11-08T08:20:24Z

I like the comments, and I take it as learning. in fact, I think your comments are very thorough - so thanks for that.
It's been more than 20 years since I developed professionally, and there was no Github or anything like that (at my work). We modified huge warehouse management systems straight on the production servers with only minimum testing:-). Some customers suffered pretty much from that strategy :-(.
So I'm enjoying leaning about this "new" world, and hope you can bear with my newbie mistakes.
I will have a look at the comment over the next couple of weeks.

…sting in other series): - I-See mode - Absense Detect mode (MZS-FH series, unknown if existing in other series) - Ecocool mode (MZS-FH series, unknown if existing in other series) - Natural Flow mode (MZS-FH series, unknown if existing in other series) Corrected LINT errors and warnings from last commit Cleaned up order of setWideVane and getWideVane Removed test tool (excel)

ratnick · 2021-11-14T19:40:43Z

I have now corrected the errors and warnings.
I also added 4 more modes supported by the A/C.

I am unsure if I should create a new Pull Request or if what I have done here is fine?

ratnick · 2021-11-14T19:41:37Z

PS: How do I run the build myself, so I can check the build output myself (before submitting to you)?

crankyoldgit · 2021-11-14T20:42:49Z

PS: How do I run the build myself, so I can check the build output myself (before submitting to you)?

Read this:
https://github.com/crankyoldgit/IRremoteESP8266/wiki/Library-Maintainers-Guide

crankyoldgit · 2021-11-15T04:22:56Z

src/ir_Mitsubishi.cpp

-/// i-SAVE only has this 10C functionality when the AC is already in Heat mode. In all other modes, minimum temp is 16C.
-/// I have found no other difference between normal Heat mode and i-SAVE other than the ability to go to 10C.
-/// In this implementation, i-SAVE mode is ONLY used to enable the AC temperature setting to 10C. Therefore "Temp" is set to 16 disregarding what the remote shows, and mode is set to Heat.
+/// Normal minimum temp is 16C, and i-SAVE mode works as a gate to enable AC


/// @note Normal minimum temp is 16C, and i-SAVE mode works as a gate to enable AC

Not sure what you are requesting here?

What I'm saying is, please add @note to the start of that line.
i.e.
Change: /// Normal minimum temp is 16C, and i-SAVE mode works as a gate to enable AC to /// @note Normal minimum temp is 16C, and i-SAVE mode works as a gate to enable AC

crankyoldgit · 2021-11-15T04:24:28Z

src/ir_Mitsubishi.cpp

+    if (state) setMode(kMitsubishiAcHeat);
+    if (state) setTemp(kMitsubishiAcMinTemp);  
+    _.iSave10C = state;


if (state) { setMode(kMitsubishiAcHeat); setTemp(kMitsubishiAcMinTemp); } _.iSave10C = state;

crankyoldgit · 2021-11-15T04:25:26Z

src/ir_Mitsubishi.cpp

+void IRMitsubishiAC::setISee(const bool state) {
+    _.ISee = state;
+}
+
+bool IRMitsubishiAC::getISee(void) const {
+    return _.ISee;
+}
+
+void IRMitsubishiAC::setEcocool(const bool state) {
+    _.Ecocool = state;
+}
+
+bool IRMitsubishiAC::getEcocool(void) const {
+    return _.Ecocool;
+}
+
+void IRMitsubishiAC::setAbsenseDetect(const bool state) {
+    _.AbsenseDetect = state;
+}
+
+bool IRMitsubishiAC::getAbsenseDetect(void) const {
+    return _.AbsenseDetect;
+}
+
+/// Set the requested Direct/Indirect mode. Only works if I-See mode is ON.
+/// @param[in] The requested Direct/Indirect mode..
+void IRMitsubishiAC::setDirectIndirect(const uint8_t mode) {
+    if (_.ISee) {
+        _.DirectIndirect = std::min(mode, kMitsubishiAcDirect);  // bounds check
+    }
+    else {
+        _.DirectIndirect = 0;
+    }
+}
+
+/// Get the LDirect/Indirect mode of the A/C.
+/// @return The native mode setting.
+uint8_t IRMitsubishiAC::getDirectIndirect(void) const { 
+    return _.DirectIndirect;
+}
+
+void IRMitsubishiAC::setNaturalFlow(const bool state) {
+    _.NaturalFlow = state;
+}
+
+bool IRMitsubishiAC::getNaturalFlow(void) const{
+    return _.NaturalFlow;
+}
+
+


Style-guide-nitpick: Only two (not four) spaces for each level of indentation please.

crankyoldgit · 2021-11-15T04:25:48Z

src/ir_Mitsubishi.cpp

+    }
+}
+
+/// Get the LDirect/Indirect mode of the A/C.


LDirect? typo?

crankyoldgit · 2021-11-15T04:28:08Z

src/ir_Mitsubishi.cpp

+    if (_.ISee) {
+        _.DirectIndirect = std::min(mode, kMitsubishiAcDirect);  // bounds check
+    }
+    else {
+        _.DirectIndirect = 0;
+    }


_.DirectIndirect = _.ISee ? std::min(mode, kMitsubishiAcDirect) : kMitsubishiAcDirectOff;

crankyoldgit · 2021-11-15T04:33:55Z

src/ir_Mitsubishi.h

+  /// 1 = set to i-Save when temp=10C on remote.
+  /// "Temp" byte is not affected by this; 
+  /// 10C temp is implied through other bits.


Move those comments in/near the procedures in the .cpp file. e.g. Doxygen will automatically turn the comments into documentation for us.

crankyoldgit · 2021-11-15T04:37:58Z

src/locale/defaults.h

+#ifndef D_STR_DIRECTINDIRECTMODE
+#define D_STR_DIRECTINDIRECTMODE "Direct / Indirect mode"
+#endif  // D_STR_DIRECTINDIRECTMODE
+#ifndef D_STR_DIRECT
+#define D_STR_DIRECT "Direct"
+#endif  // D_STR_DIRECT
+#ifndef D_STR_INDIRECT
+#define D_STR_INDIRECT "Indirect"
+#endif  // D_STR_INDIRECT


If we re-order them, we can use the sub-words for the compound term. This can help for internationalisation/translations.

#ifndef D_STR_DIRECT #define D_STR_DIRECT "Direct" #endif // D_STR_DIRECT #ifndef D_STR_INDIRECT #define D_STR_INDIRECT "Indirect" #endif // D_STR_INDIRECT #ifndef D_STR_DIRECTINDIRECTMODE #define D_STR_DIRECTINDIRECTMODE D_STR_DIRECT" / " D_STR_INDIRECT " mode" #endif // D_STR_DIRECTINDIRECTMODE

crankyoldgit · 2021-11-15T04:39:17Z

src/locale/defaults.h

+#ifndef D_STR_NATURALFLOW
+#define D_STR_NATURALFLOW "Natural Flow"
+#endif  // D_STR_NATURALFLOW


[Q] What does this feature do? If we can, we want to reuse an existing term/word/name etc to reduce translation effort and to reduce the space required for the strings.

I used Mitsubishis terminology from the remote and from the user’s manual. In this way it’s consistent for the next (Mitsubishi) user in line.
The functionality is that it just open the airflow from outside without heating or cooling.

Would kFreshStr be an acceptable alternative? If not, leaving this as is fine. Same argument as Econo vs Ecocool, except "Natural Flow" is even larger/more space etc.

OK, done for both Ecocool and Natural Flow

src/locale/defaults.h

crankyoldgit · 2021-11-15T04:40:16Z

src/locale/defaults.h

@@ -66,6 +66,9 @@
 #ifndef D_STR_ECONO
 #define D_STR_ECONO "Econo"
 #endif  // D_STR_ECONO
+#ifndef D_STR_ECOCOOL
+#define D_STR_ECOCOOL "Ecocool"


[Q] What does this feature do? If we can, we want to reuse an existing term/word/name etc to reduce translation effort and to reduce the space required for the strings.
e.g. Would Econo be enough?

I used Mitsubishis terminology from the remote and from the user’s manual. In this way it’s consistent for the next (Mitsubishi) user in line.
Not sure of the exact functionality, but my guess is that it lowers the power consumption when cooling at the expense of cooling speed. To me, “Econo” can be used for both heating and cooling, and is thus a broader term.

Please (re)use the "Econo" / D_STR_ECONO / kEconoStr string in your code for this, instead of a new string.
I understand the vendor has a special name for it, but if we did this for every vendor, we would have a dozen names for basically the same mode/setting. That would chew up valuable storage/memory space. e.g. Using kEconoStr instead of the new string, will save at least 8 to 12 bytes of flash. It might not seem like much, but it adds up.

crankyoldgit · 2021-11-15T04:43:20Z

FYI: Linter issues:

src/IRac.h:340:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/IRtext.cpp:86:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/IRtext.cpp:87:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.cpp:403:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.cpp:479:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.cpp:482:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.cpp:522:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
src/ir_Mitsubishi.cpp:522:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]
src/ir_Mitsubishi.cpp:529:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.cpp:537:  Missing space before {  [whitespace/braces] [5]
src/ir_Mitsubishi.h:98:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Mitsubishi.h:98:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_Mitsubishi.h:291:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/ir_Mitsubishi.h:305:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Mitsubishi.h:305:  At least two spaces is best between code and comments  [whitespace/comments] [2]

And the unit tests need to be updated too:
See https://github.com/crankyoldgit/IRremoteESP8266/runs/4205298502?check_suite_focus=true

- removed spaces at end of line - no lines longer than 80 chars - changed from 4 to 2 indents - space between // and comment - Doxygen comments for all functions - reordered and reused some strings (defaults.h) as per suggestion - Updated unit test, expected output

ratnick · 2021-11-22T12:13:21Z

Hi David, Thanks for your comments. I gave up on installing Linux (Ubuntu) on my Windows PC to run the tests locally. There were too many “detours” and things I don’t know enough about to just make work, and I will end up spending many hours (days). Unfortunately, it’s time I don’t have. Instead, I modified the test scripts where they failed from my best guess, and I hope they run now. On your questions below:

+#ifndef D_STR_NATURALFLOW

+#define D_STR_NATURALFLOW "Natural Flow" +#endif // D_STR_NATURALFLOW [Q] What does this feature do? If we can, we want to reuse an existing term/word/name etc to reduce translation effort and to reduce the space required for the strings. [A] I used Mitsubishis terminology from the remote and from the user’s manual. In this way it’s consistent for the next (Mitsubishi) user in line. The functionality is that it just open the airflow from outside without heating or cooling.

@@ -120,6 +123,9 @@

#ifndef D_STR_IFEEL #define D_STR_IFEEL "IFeel" #endif // D_STR_IFEEL +#ifndef D_STR_ISEE +#define D_STR_ISEE "ISee" [Q] What does this feature do? If we can, we want to reuse an existing term/word/name etc to reduce translation effort and to reduce the space required for the strings. [A] I used Mitsubishis terminology from the remote and from the user’s manual. In this way it’s consistent for the next (Mitsubishi) user in line. I guess it’s a trademarked term, and to mee, it would be confusing to use something else that what the manufacturer uses. That would mean, I would have to look into the code to check the functionality. The functionality is that it uses a (infrared, I guess) sensor to “see” if there are persons in the room. In combination with other settings, that can be used to adjust heat depending on whether there are persons present. It can also be used for providing direct or indirect airflow towards the persons in the room.

@@ -66,6 +66,9 @@

#ifndef D_STR_ECONO #define D_STR_ECONO "Econo" #endif // D_STR_ECONO +#ifndef D_STR_ECOCOOL +#define D_STR_ECOCOOL "Ecocool" [Q] What does this feature do? If we can, we want to reuse an existing term/word/name etc to reduce translation effort and to reduce the space required for the strings. e.g. Would Econo be enough? [A] I used Mitsubishis terminology from the remote and from the user’s manual. In this way it’s consistent for the next (Mitsubishi) user in line. Not sure of the exact functionality, but my guess is that it lowers the power consumption when cooling at the expense of cooling speed. To me, “Econo” can be used for both heating and cooling, and is thus a broader term. All the best, Nikolaj From: David Conran ***@***.***> Sent: 15. november 2021 05:41 To: crankyoldgit/IRremoteESP8266 ***@***.***> Cc: Nikolaj Nøhr-Rasmussen ***@***.***>; Author ***@***.***> Subject: Re: [crankyoldgit/IRremoteESP8266] MITSUBISHI_AC: Added support for i-SAVE mode. (PR #1666) @crankyoldgit requested changes on this pull request.