diff --git a/esphome/components/mitsubishi_cn105/mitsubishi_cn105.cpp b/esphome/components/mitsubishi_cn105/mitsubishi_cn105.cpp index 6683a9a25bc..3d30d1a25f0 100644 --- a/esphome/components/mitsubishi_cn105/mitsubishi_cn105.cpp +++ b/esphome/components/mitsubishi_cn105/mitsubishi_cn105.cpp @@ -49,6 +49,13 @@ void MitsubishiCN105::initialize() { this->set_state_(State::CONNECTING); } bool MitsubishiCN105::update() { switch (this->state_) { + case State::DEFERRED_STATUS_REQUEST: + // Defer the next request to a later loop iteration; some units might not respond if a request is sent + // immediately after a response. See https://github.com/esphome/esphome/issues/18099. No minimum RX-to-TX delay + // is enforced. + this->set_state_(State::UPDATING_STATUS); + return false; + case State::WAITING_FOR_SCHEDULED_STATUS_UPDATE: if (this->pending_updates_.any()) { this->status_update_wait_credit_ms_ = @@ -101,12 +108,14 @@ bool MitsubishiCN105::should_transition(State from, State to) { return from == State::CONNECTING; case State::UPDATING_STATUS: - return from == State::CONNECTED || from == State::STATUS_UPDATED || - from == State::WAITING_FOR_SCHEDULED_STATUS_UPDATE; + return from == State::DEFERRED_STATUS_REQUEST || from == State::WAITING_FOR_SCHEDULED_STATUS_UPDATE; case State::STATUS_UPDATED: return from == State::UPDATING_STATUS; + case State::DEFERRED_STATUS_REQUEST: + return from == State::CONNECTED || from == State::STATUS_UPDATED; + case State::SCHEDULE_NEXT_STATUS_UPDATE: return from == State::STATUS_UPDATED || from == State::SETTINGS_APPLIED; @@ -114,7 +123,7 @@ bool MitsubishiCN105::should_transition(State from, State to) { return from == State::SCHEDULE_NEXT_STATUS_UPDATE; case State::APPLYING_SETTINGS: - return from == State::WAITING_FOR_SCHEDULED_STATUS_UPDATE || from == State::STATUS_UPDATED; + return from == State::WAITING_FOR_SCHEDULED_STATUS_UPDATE; case State::SETTINGS_APPLIED: return from == State::APPLYING_SETTINGS; @@ -122,9 +131,10 @@ bool MitsubishiCN105::should_transition(State from, State to) { case State::READ_TIMEOUT: return from == State::UPDATING_STATUS || from == State::APPLYING_SETTINGS || from == State::CONNECTING; - default: + case State::NOT_CONNECTED: return false; } + return false; } void MitsubishiCN105::did_transition_(State to) { @@ -135,7 +145,7 @@ void MitsubishiCN105::did_transition_(State to) { case State::CONNECTED: this->current_status_msg_type_ = STATUS_MSG_SETTINGS; - this->set_state_(State::UPDATING_STATUS); + this->set_state_(State::DEFERRED_STATUS_REQUEST); break; case State::UPDATING_STATUS: @@ -143,11 +153,14 @@ void MitsubishiCN105::did_transition_(State to) { break; case State::STATUS_UPDATED: { - if (this->pending_updates_.any() && this->is_status_initialized()) { - this->set_state_(State::APPLYING_SETTINGS); - } else if (this->current_status_msg_type_ == STATUS_MSG_SETTINGS && this->should_request_telemetry_()) { + // When present, pending settings are applied from WAITING_FOR_SCHEDULED_STATUS_UPDATE during the next update(), + // deferring transmission to a later loop iteration; some units might not respond if a request is sent + // immediately after a response, causing the request to time out. + const bool should_apply_pending_settings = this->pending_updates_.any() && this->is_status_initialized(); + if (!should_apply_pending_settings && this->current_status_msg_type_ == STATUS_MSG_SETTINGS && + this->should_request_telemetry_()) { this->current_status_msg_type_ = STATUS_MSG_TELEMETRY; - this->set_state_(State::UPDATING_STATUS); + this->set_state_(State::DEFERRED_STATUS_REQUEST); } else { this->set_state_(State::SCHEDULE_NEXT_STATUS_UPDATE); } @@ -175,7 +188,9 @@ void MitsubishiCN105::did_transition_(State to) { this->set_state_(State::CONNECTING); break; - default: + case State::NOT_CONNECTED: + case State::DEFERRED_STATUS_REQUEST: + case State::WAITING_FOR_SCHEDULED_STATUS_UPDATE: break; } } @@ -359,6 +374,8 @@ const LogString *MitsubishiCN105::state_to_string(State state) { return LOG_STR("UpdatingStatus"); case State::STATUS_UPDATED: return LOG_STR("StatusUpdated"); + case State::DEFERRED_STATUS_REQUEST: + return LOG_STR("DeferredStatusRequest"); case State::SCHEDULE_NEXT_STATUS_UPDATE: return LOG_STR("ScheduleNextStatusUpdate"); case State::WAITING_FOR_SCHEDULED_STATUS_UPDATE: diff --git a/esphome/components/mitsubishi_cn105/mitsubishi_cn105.h b/esphome/components/mitsubishi_cn105/mitsubishi_cn105.h index 4d3f899dee9..0fee90dfc11 100644 --- a/esphome/components/mitsubishi_cn105/mitsubishi_cn105.h +++ b/esphome/components/mitsubishi_cn105/mitsubishi_cn105.h @@ -101,6 +101,7 @@ class MitsubishiCN105 { CONNECTED, UPDATING_STATUS, STATUS_UPDATED, + DEFERRED_STATUS_REQUEST, SCHEDULE_NEXT_STATUS_UPDATE, WAITING_FOR_SCHEDULED_STATUS_UPDATE, APPLYING_SETTINGS, diff --git a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp index 669345f5763..10d935a775e 100644 --- a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp +++ b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_climate_tests.cpp @@ -15,11 +15,10 @@ struct MitsubishiCN105ClimateTestContext { TEST(MitsubishiCN105ClimateTests, CelsiusTemperatureMappingAndTraitsMatchExpectedValues) { MitsubishiCN105ClimateTestContext context; - const auto mapping = TemperatureMapping(); for (int temperature = 16; temperature <= 31; ++temperature) { - EXPECT_EQ(mapping.to_mitsubishi(temperature), temperature); - EXPECT_EQ(mapping.from_mitsubishi(temperature), temperature); + EXPECT_EQ(context.component.get_temperature_mapping().to_mitsubishi(temperature), temperature); + EXPECT_EQ(context.component.get_temperature_mapping().from_mitsubishi(temperature), temperature); } const auto traits = context.sut.traits(); @@ -32,8 +31,6 @@ TEST(MitsubishiCN105ClimateTests, CelsiusTemperatureMappingAndTraitsMatchExpecte TEST(MitsubishiCN105ClimateTests, FahrenheitTemperatureMappingAndTraitsMatchExpectedValues) { MitsubishiCN105ClimateTestContext context; - auto mapping = TemperatureMapping(); - mapping.set_use_fahrenheit(true); context.component.set_use_fahrenheit(true); const std::array cases{ @@ -46,8 +43,8 @@ TEST(MitsubishiCN105ClimateTests, FahrenheitTemperatureMappingAndTraitsMatchExpe }; for (const auto &[fahrenheit, mitsubishi_celsius] : cases) { - EXPECT_FLOAT_EQ(mapping.to_mitsubishi(fahrenheit), mitsubishi_celsius); - EXPECT_FLOAT_EQ(mapping.from_mitsubishi(mitsubishi_celsius), fahrenheit); + EXPECT_FLOAT_EQ(context.component.get_temperature_mapping().to_mitsubishi(fahrenheit), mitsubishi_celsius); + EXPECT_FLOAT_EQ(context.component.get_temperature_mapping().from_mitsubishi(mitsubishi_celsius), fahrenheit); } const auto traits = context.sut.traits(); EXPECT_EQ(traits.get_temperature_unit(), TemperatureUnit::FAHRENHEIT); diff --git a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_tests.cpp b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_tests.cpp index 3bc6d5b2b8e..fcd09bb18e7 100644 --- a/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_tests.cpp +++ b/tests/components/mitsubishi_cn105/climate/mitsubishi_cn105_tests.cpp @@ -42,11 +42,17 @@ TEST(MitsubishiCN105Tests, ConnectAndUpdateStatus) { // All bytes from UART should be consumed EXPECT_TRUE(ctx.uart.rx.empty()); - // After successful connect we request status, first settings (0x02) + // Defer the first settings request (0x02) until the next update. + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::DEFERRED_STATUS_REQUEST); + EXPECT_TRUE(ctx.uart.tx.empty()); + + ctx.sut.set_current_time(201); + ASSERT_FALSE(ctx.sut.update()); + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::UPDATING_STATUS); EXPECT_THAT(ctx.uart.tx, ::testing::ElementsAre(0xFC, 0x42, 0x01, 0x30, 0x10, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x7B)); - EXPECT_EQ(ctx.sut.operation_start_ms_, 200); + EXPECT_EQ(ctx.sut.operation_start_ms_, 201); // Clear TX bytes. ctx.uart.tx.clear(); @@ -75,15 +81,24 @@ TEST(MitsubishiCN105Tests, ConnectAndUpdateStatus) { EXPECT_EQ(ctx.sut.status().vane_mode, MitsubishiCN105::VaneMode::POSITION_4); EXPECT_EQ(ctx.sut.status().wide_vane_mode, MitsubishiCN105::WideVaneMode::SWING); - // Now fetch telemetry (0x03) + // Defer the telemetry request (0x03) until the next update. + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::DEFERRED_STATUS_REQUEST); + EXPECT_TRUE(ctx.uart.tx.empty()); + + ctx.sut.set_current_time(301); + ASSERT_FALSE(ctx.sut.update()); + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::UPDATING_STATUS); EXPECT_THAT(ctx.uart.tx, ::testing::ElementsAre(0xFC, 0x42, 0x01, 0x30, 0x10, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x7A)); - EXPECT_EQ(ctx.sut.operation_start_ms_, 300); + EXPECT_EQ(ctx.sut.operation_start_ms_, 301); // Clear TX bytes. ctx.uart.tx.clear(); + // Queue a setting while waiting for telemetry. + ctx.sut.set_power(true); + // Telemetry response ctx.uart.push_rx({0xFC, 0x62, 0x01, 0x30, 0x10, 0x03, 0x00, 0x00, 0x0B, 0x00, 0x00, 0xAA, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xA5}); @@ -103,6 +118,13 @@ TEST(MitsubishiCN105Tests, ConnectAndUpdateStatus) { EXPECT_TRUE(ctx.uart.tx.empty()); EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::WAITING_FOR_SCHEDULED_STATUS_UPDATE); EXPECT_EQ(ctx.sut.operation_start_ms_, 400); + + // Apply the pending setting on the next update, outside RX processing. + ctx.sut.set_current_time(401); + ASSERT_FALSE(ctx.sut.update()); + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::APPLYING_SETTINGS); + EXPECT_FALSE(ctx.uart.tx.empty()); + EXPECT_EQ(ctx.sut.operation_start_ms_, 401); } TEST(MitsubishiCN105Tests, NoResponseTriggersReconnect) { @@ -469,6 +491,36 @@ TEST(MitsubishiCN105Tests, WriteInterruptsWaitingForNextStatusUpdate) { EXPECT_EQ(ctx.sut.status_update_wait_credit_ms_, 0); } +TEST(MitsubishiCN105Tests, PendingSettingsTakePriorityOverDueTelemetry) { + MitsubishiCN105TestsContext ctx; + + ctx.sut.status_.target_temperature = 24.0f; + ctx.sut.status_.room_temperature = 21.0f; + ASSERT_TRUE(ctx.sut.is_status_initialized()); + + ctx.sut.state_ = TestableMitsubishiCN105::State::STATUS_UPDATED; + ctx.sut.set_state(TestableMitsubishiCN105::State::SCHEDULE_NEXT_STATUS_UPDATE); + ctx.sut.set_current_time(1000); + ASSERT_FALSE(ctx.sut.update()); + ASSERT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::UPDATING_STATUS); + ctx.uart.tx.clear(); + + ctx.sut.set_power(true); + ctx.uart.push_rx({0xFC, 0x62, 0x01, 0x30, 0x10, 0x02, 0x00, 0x00, 0x00, 0x08, 0x07, + 0x00, 0x04, 0x00, 0x00, 0x0C, 0x00, 0x00, 0x00, 0x00, 0x00, 0x3C}); + + ctx.sut.set_current_time(1001); + ASSERT_TRUE(ctx.sut.update()); + EXPECT_TRUE(ctx.uart.tx.empty()); + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::WAITING_FOR_SCHEDULED_STATUS_UPDATE); + + ctx.sut.set_current_time(1002); + ASSERT_FALSE(ctx.sut.update()); + EXPECT_EQ(ctx.sut.state_, TestableMitsubishiCN105::State::APPLYING_SETTINGS); + EXPECT_THAT(ctx.uart.tx, ::testing::ElementsAre(0xFC, 0x41, 0x01, 0x30, 0x10, 0x01, 0x01, 0x00, 0x01, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x7B)); +} + TEST(MitsubishiCN105Tests, SetAndClearRemoteRoomTemp) { MitsubishiCN105TestsContext ctx;