Irremoteesp8266: haier HSU07-HEA03 code error

[siriuslzx]

Version/revision of the library used

v2.7.9

as #404 says:
[40-41] Fan speed
00 - Auto
01 - 3 (max)
10 - 2
11 - 1 (min)

[42-47] Timer OFF MINS
0-59 minutes

Actual behavior

in ir_Haier.cpp:

/// Set the speed of the fan.
/// @param[in] speed The desired setting.
void IRHaierAC::setFan(const uint8_t speed) {
  uint8_t new_speed = kHaierAcFanAuto;
  switch (speed) {
    case kHaierAcFanLow:  new_speed = 3; break;
    case kHaierAcFanMed:  new_speed = 1; break;
    case kHaierAcFanHigh: new_speed = 2; break;
    // Default to auto for anything else.
    default:              new_speed = kHaierAcFanAuto;
  }

  if (speed != getFan()) setCommand(kHaierAcCmdFan);
  setBits(&remote_state[5], kLowNibble, kHaierAcSwingSize, new_speed);
}

/// Get the current fan speed setting.
/// @return The current fan speed.
uint8_t IRHaierAC::getFan(void) {
  switch (GETBITS8(remote_state[5], kLowNibble, kHaierAcSwingSize)) {
    case 1:  return kHaierAcFanMed;
    case 2:  return kHaierAcFanHigh;
    case 3:  return kHaierAcFanLow;
    default: return kHaierAcFanAuto;
  }
}

Expected behavior

in ir_Haier.cpp:

/// Set the speed of the fan.
/// @param[in] speed The desired setting.
void IRHaierAC::setFan(const uint8_t speed) {
  uint8_t new_speed = kHaierAcFanAuto;
  switch (speed) {
    case kHaierAcFanLow:  new_speed = 3; break;
    case kHaierAcFanMed:  new_speed = 2; break;
    case kHaierAcFanHigh: new_speed = 1; break;
    // Default to auto for anything else.
    default:              new_speed = kHaierAcFanAuto;
  }

  if (speed != getFan()) setCommand(kHaierAcCmdFan);
  setBits(&remote_state[5], 6, kHaierAcSwingSize, new_speed);
}

/// Get the current fan speed setting.
/// @return The current fan speed.
uint8_t IRHaierAC::getFan(void) {
  switch (GETBITS8(remote_state[5], 6, kHaierAcSwingSize)) {
    case 1:  return kHaierAcFanHigh;
    case 2:  return kHaierAcFanMed;
    case 3:  return kHaierAcFanLow;
    default: return kHaierAcFanAuto;
  }
}

or by using bit field:

in ir_Haier.h:

// Haier HSU07-HEA03 remote
union HaierProtocol{
  ///< The state in native IR code form
  uint8_t remote_state[kHaierACStateLength];
  struct {
    // Byte 0
    uint8_t Prefix;
    // Byte 1
    uint8_t Command:4;
    uint8_t Temp   :4;
    // Byte 2
    uint8_t CurrHours:5;
    uint8_t unknow   :1;  // value=1
    uint8_t Swing    :2;
    // Byte 3
    uint8_t CurrMins:6;
    uint8_t OffTimer:1;
    uint8_t OnTimer :1;
    // Byte 4
    uint8_t OffHours:5;
    uint8_t Health  :1;
    uint8_t :0;
    // Byte 5
    uint8_t OffMins:6;
    uint8_t Fan    :2;
    // Byte 6
    uint8_t OnHours:5;
    uint8_t Mode   :3;
    // Byte 7
    uint8_t OnMins:6;
    uint8_t Sleep :1;
    uint8_t :0;
    // Byte 8
    uint8_t Sum;
  };
};

in ir_Haier.cpp:

/// Set the speed of the fan.
/// @param[in] speed The desired setting.
void IRHaierAC::setFan(const uint8_t speed) {
  uint8_t new_speed = kHaierAcFanAuto;
  switch (speed) {
    case kHaierAcFanLow:  new_speed = 3; break;
    case kHaierAcFanMed:  new_speed = 2; break;
    case kHaierAcFanHigh: new_speed = 1; break;
    // Default to auto for anything else.
    default:              new_speed = kHaierAcFanAuto;
  }

  if (speed != getFan()) setCommand(kHaierAcCmdFan);
  _.Fan = new_speed;
}

/// Get the current fan speed setting.
/// @return The current fan speed.
uint8_t IRHaierAC::getFan(void) const {
  switch (_.Fan) {
    case 1:  return kHaierAcFanHigh;
    case 2:  return kHaierAcFanMed;
    case 3:  return kHaierAcFanLow;
    default: return kHaierAcFanAuto;
  }
}

[NiKiZe]

Reading issue #404 I agree, I wonder if @kuzin2006 can provide any input here, and if the fan speed was tested or not?

[siriuslzx]

in ir_Haier_test.cpp,from line369

  EXPECT_EQ(
      "Command: 8 (Sleep), Mode: 1 (Cool), Temp: 21C, Fan: 3 (High), "
      "Swing: 3 (Change), Sleep: On, Health: On, "
      "Clock: 10:15, On Timer: Off, Off Timer: Off",
      haier.toString());
  haier.setOnTimer(800);    // 1:20pm
  haier.setOffTimer(1125);  // 6:45pm
  haier.setCommand(kHaierAcCmdOn);

  EXPECT_EQ(
      "Command: 1 (On), Mode: 1 (Cool), Temp: 21C, Fan: 2 (Medium), "
      "Swing: 3 (Change), Sleep: On, Health: On, "
      "Clock: 10:15, On Timer: 13:20, Off Timer: 18:45",
      haier.toString());

haier.setOffTimer(1125) // 6:45pm
will change Fan speed,because in current library,Fan speed use[46-47], Timer OFF MINS use[42-47], or Fan speed use remote_state[5] [0-1],Timer OFF MINS use remote_state[5] [0-5],so haier.setOffTimer(1125) set Timer OFF MINS to 45(0b101101),thus set Fan speed to 0b01, then getFan() will return kHaierAcFanMed

[crankyoldgit]

I agree. I think it's stuffed. I think I overlapped the bits.

@siriuslzx Can you "fix" it in your (assuming) pending PR. Please limit the PR to just Haier, in case we need to roll it back etc because we found out that my horrible code was actually right when a user confirms it on way or the other.

I'm sure this won't be the last time we find something like this. :-/

[siriuslzx]

I agree. I think it's stuffed. I think I overlapped the bits.

@siriuslzx Can you "fix" it in your (assuming) pending PR. Please limit the PR to just Haier, in case we need to roll it back etc because we found out that my horrible code was actually right when a user confirms it on way or the other.

I'm sure this won't be the last time we find something like this. :-/

should I change the ir_Haier_test? I don't know how to write it

[crankyoldgit]

should I change the ir_Haier_test?

Yes, you should update it.

I don't know how to write it

All you'll need to do is update it with the correct text.

[siriuslzx]

should I change the ir_Haier_test?

Yes, you should update it.

I don't know how to write it

All you'll need to do is update it with the correct text.

the codes {0xA5, 0x91, 0x20, 0x00, 0x0C, 0xC0, 0x20, 0x00, 0x42}
in #668, it means: Command: 1 (On), Mode: 0 (AUTO), Temp: 25C, Fan: 0 (AUTO)
in ir_Haier_test,it means: Command: 1 (On), Mode: 1 (Cool), Temp: 25C, Fan: 0 (Auto)
and I will change it to: Command: 1 (On), Mode: 1 (Cool), Temp: 25C, Fan: 1 (Low)
how crazy! it is not serious, but I don't have a HSU07-HEA03 remote (>﹏<)

[crankyoldgit]

Just change the ir_haier.cpp to the bit field style, and we can deal with the errors produced in ir_haier_test.cpp separately if you don't feel confident.

I don't have access to the remote either. Welcome to my world.

{0xA5, 0x91, 0x20, 0x00, 0x0C, 0xC0, 0x20, 0x00, 0x42}

Sadly not everyone checks the results carefully to see if they match the remote correctly. So, don't feel to worried if it changes slightly.
Especially when you've identified an obvious problem. ie. a bit range overlap. That's clearly wrong and needs to be fixed.

[crankyoldgit]

@siriuslzx This is fixed now isn't it?

[siriuslzx]

yes,it is

[crankyoldgit]

The code changes mention have now been included in the newly released v2.7.10 of the library.