Skip to content

Commit b59fed3

Browse files
dan-s-githubclaude
andcommitted
fix(crow_alarm_panel): address Copilot review comments on PR #6
Decouple last_confirmed_acp_state_/alarm_control_panel_ updates in the ARMED_STATE handler from the armed_state_ text-sensor null check, so watchdog-abort recovery works even when only alarm_control_panel: is configured. Reword the field's comment to note it's also set heuristically from zone activity, not only ARMED_STATE broadcasts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 2b65358 commit b59fed3

2 files changed

Lines changed: 36 additions & 30 deletions

File tree

‎components/crow_alarm_panel/crow_alarm_panel.cpp‎

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -384,35 +384,39 @@ void CrowAlarmPanel::loop() {
384384
ESP_LOGW(TAG, "[%-*s] Armed state too short, discarding", this->keypad_label_width_, CONTROLLER_LABEL);
385385
break;
386386
}
387-
if (armed_state_ != nullptr) {
388-
if (data[0] == 0x00 && data[1] == 0x01) {
387+
if (data[0] == 0x00 && data[1] == 0x01) {
388+
if (this->armed_state_ != nullptr) {
389389
this->armed_state_->publish_state("arming");
390-
ESP_LOGD(TAG, "[%-*s] Arming [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
391-
format_hex_pretty(data).c_str());
392-
this->last_confirmed_acp_state_ = alarm_control_panel::ACP_STATE_ARMING;
393-
if (this->alarm_control_panel_ != nullptr) {
394-
this->alarm_control_panel_->publish_state(alarm_control_panel::ACP_STATE_ARMING);
395-
}
396-
} else if (data[0] == 0x01 && data[1] == 0x00) {
390+
}
391+
ESP_LOGD(TAG, "[%-*s] Arming [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
392+
format_hex_pretty(data).c_str());
393+
this->last_confirmed_acp_state_ = alarm_control_panel::ACP_STATE_ARMING;
394+
if (this->alarm_control_panel_ != nullptr) {
395+
this->alarm_control_panel_->publish_state(alarm_control_panel::ACP_STATE_ARMING);
396+
}
397+
} else if (data[0] == 0x01 && data[1] == 0x00) {
398+
if (this->armed_state_ != nullptr) {
397399
this->armed_state_->publish_state("armed_away");
398-
ESP_LOGD(TAG, "[%-*s] Armed Away [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
399-
format_hex_pretty(data).c_str());
400-
this->last_confirmed_acp_state_ = alarm_control_panel::ACP_STATE_ARMED_AWAY;
401-
if (this->alarm_control_panel_ != nullptr) {
402-
this->alarm_control_panel_->publish_state(alarm_control_panel::ACP_STATE_ARMED_AWAY);
403-
}
404-
} else if (data[0] == 0x00 && data[1] == 0x00) {
400+
}
401+
ESP_LOGD(TAG, "[%-*s] Armed Away [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
402+
format_hex_pretty(data).c_str());
403+
this->last_confirmed_acp_state_ = alarm_control_panel::ACP_STATE_ARMED_AWAY;
404+
if (this->alarm_control_panel_ != nullptr) {
405+
this->alarm_control_panel_->publish_state(alarm_control_panel::ACP_STATE_ARMED_AWAY);
406+
}
407+
} else if (data[0] == 0x00 && data[1] == 0x00) {
408+
if (this->armed_state_ != nullptr) {
405409
this->armed_state_->publish_state("disarmed");
406-
ESP_LOGD(TAG, "[%-*s] Disarmed [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
407-
format_hex_pretty(data).c_str());
408-
this->last_confirmed_acp_state_ = alarm_control_panel::ACP_STATE_DISARMED;
409-
if (this->alarm_control_panel_ != nullptr) {
410-
this->alarm_control_panel_->publish_state(alarm_control_panel::ACP_STATE_DISARMED);
411-
}
412-
} else {
413-
ESP_LOGD(TAG, "[%-*s] Armed state unknown [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
414-
format_hex_pretty(data).c_str());
415410
}
411+
ESP_LOGD(TAG, "[%-*s] Disarmed [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
412+
format_hex_pretty(data).c_str());
413+
this->last_confirmed_acp_state_ = alarm_control_panel::ACP_STATE_DISARMED;
414+
if (this->alarm_control_panel_ != nullptr) {
415+
this->alarm_control_panel_->publish_state(alarm_control_panel::ACP_STATE_DISARMED);
416+
}
417+
} else {
418+
ESP_LOGD(TAG, "[%-*s] Armed state unknown [%02x.%s]", this->keypad_label_width_, CONTROLLER_LABEL, type,
419+
format_hex_pretty(data).c_str());
416420
}
417421
// ARMED_STATE is the controller's own authoritative state broadcast, independent of the
418422
// arm/disarm state machine below — this is the only signal CODE_ENTER_PENDING treats as

‎components/crow_alarm_panel/crow_alarm_panel.h‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -300,11 +300,13 @@ class CrowAlarmPanel : public Component {
300300
std::string code_;
301301
text_sensor::TextSensor *armed_state_{nullptr};
302302
alarm_control_panel::AlarmControlPanel *alarm_control_panel_{nullptr};
303-
// Last state the controller itself confirmed via an ARMED_STATE broadcast — unlike
304-
// alarm_control_panel_->get_state(), never overwritten by the optimistic ACP_STATE_ARMING /
305-
// ACP_STATE_DISARMING publish in CrowAlarmControlPanel::control(). Used to restore the entity
306-
// to ground truth if the arm/disarm watchdog aborts a sequence, so a failed disarm doesn't
307-
// strand the entity in ACP_STATE_DISARMING forever (see arm_disarm_state_machine.md).
303+
// Last state we're confident is ground truth — set from the controller's own ARMED_STATE
304+
// broadcasts, and also from heuristic zone-activity publishes (zone motion -> DISARMED, zone
305+
// alarm -> PENDING) in the ZONE_STATE handler. Unlike alarm_control_panel_->get_state(), never
306+
// overwritten by the optimistic ACP_STATE_ARMING / ACP_STATE_DISARMING publish in
307+
// CrowAlarmControlPanel::control(). Used to restore the entity to ground truth if the
308+
// arm/disarm watchdog aborts a sequence, so a failed disarm doesn't strand the entity in
309+
// ACP_STATE_DISARMING forever (see arm_disarm_state_machine.md).
308310
alarm_control_panel::AlarmControlPanelState last_confirmed_acp_state_{alarm_control_panel::ACP_STATE_DISARMED};
309311
Trigger<uint8_t, std::vector<uint8_t>> *on_message_trigger_{new Trigger<uint8_t, std::vector<uint8_t>>()};
310312

0 commit comments

Comments
 (0)