From 6f7a238e7018d8e3d3ef29f9d2a88903417c6cd3 Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 15:43:25 +0100 Subject: [PATCH 01/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 86 +++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 9 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index b5ce591547..6b0774a664 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -542,20 +542,88 @@ bool Mcp4461Component::set_eeprom_value(Mcp4461EepromLocation location, uint16_t return true; } -bool Mcp4461Component::is_writing_() { return static_cast((this->get_status_register() >> 4) & 0x01); } +/** + * @brief Checks if the EEPROM is currently writing. + * + * This function reads the MCP4461 status register to determine if an EEPROM write operation is in progress. + * If the EEPROM is no longer writing, the timeout flag (`last_eeprom_write_timed_out_`) + * is reset to allow normal operation in future calls of `is_eeprom_ready_for_writing_()`. + * + * Behavior: + * - If the EEPROM is **writing**, the function returns `true`. + * - If the EEPROM is **not writing**, the function returns `false` and resets `last_eeprom_write_timed_out_`. + * + * @return `true` if the EEPROM is currently writing. + * @return `false` if the EEPROM is not writing (also resets the timeout flag). + */ +bool Mcp4461Component::is_writing_() { + /* Read the EEPROM write-active status from the status register */ + bool writing = static_cast((this->get_status_register() >> 4) & 0x01); -bool Mcp4461Component::is_eeprom_busy_() { - while (this->is_writing_() && this->previous_write_exec_time_ != 0) { - if ((millis() - this->previous_write_exec_time_) > EEPROM_WRITE_TIMEOUT_MS) { - this->previous_write_exec_time_ = millis(); - ESP_LOGE(TAG, - "EEPROM write timeout exceeded (%" PRIu8 " ms), aborting read/write from/to nonvolatile wiper/eeprom", - EEPROM_WRITE_TIMEOUT_MS); + /* If EEPROM is no longer writing, reset the timeout flag */ + if (!writing) { + /* This is protected boolean flag in Mcp4461Component class */ + this->last_eeprom_write_timed_out_ = false; + } + + return writing; +} + +/** + * @brief Checks if the EEPROM is ready for a new write operation. + * + * This function ensures that the EEPROM is not actively writing. + * It can either return the current status immediately or wait until the EEPROM becomes ready. + * + * Behavior: + * - If `wait_if_not_ready` is `false`, the function returns the current readiness status immediately. + * - If `wait_if_not_ready` is `true`: + * - The function waits up to `EEPROM_WRITE_TIMEOUT_MS` for the EEPROM to become ready. + * - If the EEPROM remains busy after the timeout, the `last_eeprom_write_timed_out_` flag is set to `true`, + * preventing unnecessary waits in future calls. + * - If the EEPROM becomes ready within the timeout, the function returns `true`. + * + * @param[in] wait_if_not_ready Specifies whether to wait for EEPROM readiness if it is currently busy. + * - `true` → Waits for completion (up to `EEPROM_WRITE_TIMEOUT_MS`). + * - `false` → Returns the current readiness status without waiting. + * + * @return `true` if the EEPROM is ready for a new write. + * @return `false` if: + * - The last write attempt **timed out** (`wait_if_not_ready = true`). + * - The EEPROM is still busy (`wait_if_not_ready = false`). + */ +bool Mcp4461Component::is_eeprom_ready_for_writing_(bool wait_if_not_ready) { + /* Check initial write status */ + bool ready_for_write = !this->is_writing_(); + + /* Return early if no waiting is required or EEPROM is already ready */ + if (ready_for_write || !wait_if_not_ready || this->last_eeprom_write_timed_out_) { + return ready_for_write; + } + + /* Timestamp before starting the loop */ + const uint32_t start_millis = millis(); + + ESP_LOGV(TAG, "Waiting until EEPROM is ready for write, start_millis = %" PRIu32, start_millis); + + /* Loop until EEPROM is ready or timeout is reached */ + while (!ready_for_write && ((millis() - start_millis) < EEPROM_WRITE_TIMEOUT_MS)) { + ready_for_write = !this->is_writing_(); + + /* If ready, exit early */ + if (ready_for_write) { + ESP_LOGV(TAG, "EEPROM is ready for new write, elapsed_millis = %" PRIu32, millis() - start_millis); return true; } - ESP_LOGV(TAG, "Waiting while eeprom is busy"); + + /* Not ready yet, yield before checking again */ yield(); } + + /* If still not ready after timeout, log error and mark the timeout */ + ESP_LOGE(TAG, "EEPROM write timeout exceeded (%" PRIu8 " ms)", EEPROM_WRITE_TIMEOUT_MS); + this->last_eeprom_write_timed_out_ = true; + return false; } From b83d01f2431bd07866f2903441ddb5e44a84a8e5 Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 15:44:20 +0100 Subject: [PATCH 02/11] Update mcp4461.h --- esphome/components/mcp4461/mcp4461.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.h b/esphome/components/mcp4461/mcp4461.h index 70b0c5561a..eec15a933f 100644 --- a/esphome/components/mcp4461/mcp4461.h +++ b/esphome/components/mcp4461/mcp4461.h @@ -96,10 +96,10 @@ class Mcp4461Component : public Component, public i2c::I2CDevice { protected: friend class Mcp4461Wiper; void set_write_protection_status_(); - bool is_writing_(); - bool is_eeprom_busy_(); uint8_t get_wiper_address_(uint8_t wiper); uint16_t read_wiper_level_(uint8_t wiper); + bool is_writing_(); + bool is_eeprom_ready_for_writing_(bool wait_if_not_ready); void write_wiper_level_(uint8_t wiper, uint16_t value); bool mcp4461_write_(uint8_t addr, uint16_t data, bool nonvolatile = false); uint8_t calc_terminal_connector_byte_(Mcp4461TerminalIdx terminal_connector); @@ -107,7 +107,7 @@ class Mcp4461Component : public Component, public i2c::I2CDevice { WiperState reg_[8]; void begin_(); bool update_{false}; - uint32_t previous_write_exec_time_; + bool last_write_timed_out_{false}; bool write_protected_{false}; bool wiper_0_disabled_{false}; bool wiper_1_disabled_{false}; From 14119e8d9093d1b774c089a5ac4d58be1aee8bc9 Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 15:44:54 +0100 Subject: [PATCH 03/11] Update mcp4461.h --- esphome/components/mcp4461/mcp4461.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esphome/components/mcp4461/mcp4461.h b/esphome/components/mcp4461/mcp4461.h index eec15a933f..6135850994 100644 --- a/esphome/components/mcp4461/mcp4461.h +++ b/esphome/components/mcp4461/mcp4461.h @@ -107,7 +107,7 @@ class Mcp4461Component : public Component, public i2c::I2CDevice { WiperState reg_[8]; void begin_(); bool update_{false}; - bool last_write_timed_out_{false}; + bool last_eeprom_write_timed_out_{false}; bool write_protected_{false}; bool wiper_0_disabled_{false}; bool wiper_1_disabled_{false}; From f762d2c315de2bea829ea4346d8d302e0ba6d126 Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 15:50:36 +0100 Subject: [PATCH 04/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index 6b0774a664..3682f58c66 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -212,7 +212,7 @@ uint16_t Mcp4461Component::read_wiper_level_(uint8_t wiper) { reg |= this->get_wiper_address_(wiper); reg |= static_cast(Mcp4461Commands::READ); if (wiper > 3) { - if (this->is_eeprom_busy_()) { + if (this->is_eeprom_ready_for_writing_(true)) { return 0; } } @@ -511,7 +511,7 @@ uint16_t Mcp4461Component::get_eeprom_value(Mcp4461EepromLocation location) { reg |= static_cast(Mcp4461EepromLocation::MCP4461_EEPROM_1) + (static_cast(location) * 0x10); reg |= static_cast(Mcp4461Commands::READ); uint16_t buf; - if (this->is_eeprom_busy_()) { + if (this->is_eeprom_ready_for_writing_(true)) { return 0; } if (!this->read_byte_16(reg, &buf)) { @@ -642,7 +642,7 @@ bool Mcp4461Component::mcp4461_write_(uint8_t addr, uint16_t data, bool nonvolat ESP_LOGW(TAG, "Ignoring write to write protected chip"); return false; } - if (this->is_eeprom_busy_()) { + if (this->is_eeprom_ready_for_writing_(true)) { return false; } this->previous_write_exec_time_ = millis(); From 1deba54baa71d2cee7cdc2e08f2e56e2db641f28 Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 15:56:06 +0100 Subject: [PATCH 05/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index 3682f58c66..6689ecfb8e 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -22,7 +22,6 @@ void Mcp4461Component::setup() { void Mcp4461Component::begin_() { // save WP/WL status this->set_write_protection_status_(); - this->previous_write_exec_time_ = 0; for (uint8_t i = 0; i < 8; i++) { if (this->reg_[i].initial_value.has_value()) { uint16_t initial_state; @@ -212,7 +211,7 @@ uint16_t Mcp4461Component::read_wiper_level_(uint8_t wiper) { reg |= this->get_wiper_address_(wiper); reg |= static_cast(Mcp4461Commands::READ); if (wiper > 3) { - if (this->is_eeprom_ready_for_writing_(true)) { + if (!this->is_eeprom_ready_for_writing_(true)) { return 0; } } @@ -511,7 +510,7 @@ uint16_t Mcp4461Component::get_eeprom_value(Mcp4461EepromLocation location) { reg |= static_cast(Mcp4461EepromLocation::MCP4461_EEPROM_1) + (static_cast(location) * 0x10); reg |= static_cast(Mcp4461Commands::READ); uint16_t buf; - if (this->is_eeprom_ready_for_writing_(true)) { + if (!this->is_eeprom_ready_for_writing_(true)) { return 0; } if (!this->read_byte_16(reg, &buf)) { @@ -642,10 +641,9 @@ bool Mcp4461Component::mcp4461_write_(uint8_t addr, uint16_t data, bool nonvolat ESP_LOGW(TAG, "Ignoring write to write protected chip"); return false; } - if (this->is_eeprom_ready_for_writing_(true)) { + if (!this->is_eeprom_ready_for_writing_(true)) { return false; } - this->previous_write_exec_time_ = millis(); } return this->write_byte(reg, value_byte); } From 0787429a463241adbf72a3a757dfd5a0981f2b5b Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 16:03:40 +0100 Subject: [PATCH 06/11] Update mcp4461.h --- esphome/components/mcp4461/mcp4461.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.h b/esphome/components/mcp4461/mcp4461.h index 6135850994..48a19263fa 100644 --- a/esphome/components/mcp4461/mcp4461.h +++ b/esphome/components/mcp4461/mcp4461.h @@ -7,8 +7,6 @@ namespace esphome { namespace mcp4461 { -static const LogString *const LOG_PARENT_FAILED_STR = LOG_STR("Parent MCP4461 component has failed! Aborting"); - struct WiperState { bool terminal_a = true; bool terminal_b = true; @@ -55,6 +53,15 @@ enum class Mcp4461EepromLocation : uint8_t { enum class Mcp4461TerminalIdx : uint8_t { MCP4461_TERMINAL_0 = 0, MCP4461_TERMINAL_1 = 1 }; +enum ErrorCode { + MCP4461_STATUS_OK = 0, // CMD completed successfully + MCP4461_STATUS_REGISTER_INVALID, // Status register value was invalid + MCP4461_VALUE_INVALID, // Invalid value given for wiper / eeprom + MCP4461_STATUS_WRITE_PROTECTED, // The value was read, but the CRC over the payload (valid and data) does not match + MCP4461_STATUS_WIPER_LOCKED, // The wiper is locked using WiperLock-technology - all actions for this wiper will be aborted/discarded + } error_code_{MCP4461_STATUS_OK}; + + class Mcp4461Wiper; // Mcp4461Component From 0bef8c20b7296532513ff1f17eb4e6d2388c4c4e Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 16:06:30 +0100 Subject: [PATCH 07/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index 6689ecfb8e..099b1dffb4 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -42,6 +42,24 @@ void Mcp4461Component::begin_() { } } +// Converts a status to a human readable string +static const LogString *mcp4461_get_message_string_(int status) { + switch (status) { + case Mcp4461Component::MCP4461_STATUS_I2C_ERROR: + return LOG_STR("I2C error - communication with MCP4461 failed!"); + case Mcp4461Component::ENS210_STATUS_CRC_ERROR: + return LOG_STR("CRC error"); + case Mcp4461Component::ENS210_STATUS_INVALID: + return LOG_STR("Invalid data"); + case Mcp4461Component::ENS210_STATUS_OK: + return LOG_STR("Status OK"); + case Mcp4461Component::ENS210_WRONG_CHIP_ID: + return LOG_STR("ENS210 has wrong chip ID! Is it a ENS210?"); + default: + return LOG_STR("Unknown"); + } +} + void Mcp4461Component::set_initial_value(Mcp4461WiperIdx wiper, float initial_value) { uint8_t wiper_id = static_cast(wiper); this->reg_[wiper_id].initial_value = initial_value; From 09d33c1bd117eeae43482d26d11d33a81d095b4d Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 16:07:33 +0100 Subject: [PATCH 08/11] Update mcp4461.h --- esphome/components/mcp4461/mcp4461.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.h b/esphome/components/mcp4461/mcp4461.h index 48a19263fa..cb9ff0cfaa 100644 --- a/esphome/components/mcp4461/mcp4461.h +++ b/esphome/components/mcp4461/mcp4461.h @@ -54,12 +54,13 @@ enum class Mcp4461EepromLocation : uint8_t { enum class Mcp4461TerminalIdx : uint8_t { MCP4461_TERMINAL_0 = 0, MCP4461_TERMINAL_1 = 1 }; enum ErrorCode { - MCP4461_STATUS_OK = 0, // CMD completed successfully - MCP4461_STATUS_REGISTER_INVALID, // Status register value was invalid - MCP4461_VALUE_INVALID, // Invalid value given for wiper / eeprom - MCP4461_STATUS_WRITE_PROTECTED, // The value was read, but the CRC over the payload (valid and data) does not match - MCP4461_STATUS_WIPER_LOCKED, // The wiper is locked using WiperLock-technology - all actions for this wiper will be aborted/discarded - } error_code_{MCP4461_STATUS_OK}; + MCP4461_STATUS_OK = 0, // CMD completed successfully + MCP4461_STATUS_I2C_ERROR, // Unable to communicate with device + MCP4461_STATUS_REGISTER_INVALID, // Status register value was invalid + MCP4461_VALUE_INVALID, // Invalid value given for wiper / eeprom + MCP4461_STATUS_WRITE_PROTECTED, // The value was read, but the CRC over the payload (valid and data) does not match + MCP4461_STATUS_WIPER_LOCKED, // The wiper is locked using WiperLock-technology - all actions for this wiper will be aborted/discarded +} error_code_{MCP4461_STATUS_OK}; class Mcp4461Wiper; From c4f5a84bf2b7e0a59524a3339b2bdf7e6deb96da Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 16:11:28 +0100 Subject: [PATCH 09/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index 099b1dffb4..f586fdf475 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -47,14 +47,16 @@ static const LogString *mcp4461_get_message_string_(int status) { switch (status) { case Mcp4461Component::MCP4461_STATUS_I2C_ERROR: return LOG_STR("I2C error - communication with MCP4461 failed!"); - case Mcp4461Component::ENS210_STATUS_CRC_ERROR: - return LOG_STR("CRC error"); - case Mcp4461Component::ENS210_STATUS_INVALID: - return LOG_STR("Invalid data"); - case Mcp4461Component::ENS210_STATUS_OK: + case Mcp4461Component::MCP4461_STATUS_REGISTER_INVALID: + return LOG_STR("Invalid status register value - bits 1,7 or 8 are 0"); + case Mcp4461Component::MCP4461_VALUE_INVALID: + return LOG_STR("Invalid value for wiper given"); + case Mcp4461Component:::MCP4461_STATUS_OK: return LOG_STR("Status OK"); - case Mcp4461Component::ENS210_WRONG_CHIP_ID: - return LOG_STR("ENS210 has wrong chip ID! Is it a ENS210?"); + case Mcp4461Component::MCP4461_STATUS_WRITE_PROTECTED: + return LOG_STR("MCP4461 is write protected. Setting nonvolatile wipers/eeprom values is prohibited."); + case Mcp4461Component::MCP4461_STATUS_WIPER_LOCKED: + return LOG_STR("MCP4461 Wiper is locked using WiperLock-technology. All actions on this wiper are prohibited."); default: return LOG_STR("Unknown"); } From 8050fa9ff6b90616b30ac5e5d384b8567394226e Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 16:46:03 +0100 Subject: [PATCH 10/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 107 +++++++++++++++++-------- 1 file changed, 74 insertions(+), 33 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index f586fdf475..bac92fc307 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -10,9 +10,10 @@ static const char *const TAG = "mcp4461"; constexpr uint8_t EEPROM_WRITE_TIMEOUT_MS = 10; void Mcp4461Component::setup() { - ESP_LOGCONFIG(TAG, "Setting up mcp4461 (0x%02" PRIX8 ")...", this->address_); + ESP_LOGCONFIG(TAG, "Setting up mcp4461 using address (0x%02" PRIX8 ")...", this->address_); auto err = this->write(nullptr, 0); if (err != i2c::ERROR_OK) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->mark_failed(); return; } @@ -47,16 +48,22 @@ static const LogString *mcp4461_get_message_string_(int status) { switch (status) { case Mcp4461Component::MCP4461_STATUS_I2C_ERROR: return LOG_STR("I2C error - communication with MCP4461 failed!"); + case Mcp4461Component::MCP4461_STATUS_REGISTER_ERROR: + return LOG_STR("Status register could not be read"); case Mcp4461Component::MCP4461_STATUS_REGISTER_INVALID: return LOG_STR("Invalid status register value - bits 1,7 or 8 are 0"); case Mcp4461Component::MCP4461_VALUE_INVALID: return LOG_STR("Invalid value for wiper given"); + case Mcp4461Component::MCP4461_WRITE_PROTECTED: + return LOG_STR("MCP4461 is write protected. Setting nonvolatile wipers/eeprom values is prohibited."); + case Mcp4461Component::MCP4461_WIPER_ENABLED: + return LOG_STR("MCP4461 Wiper is already enabled, ignoring cmd to enable."); + case Mcp4461Component::MCP4461_WIPER_DISABLED: + return LOG_STR("MCP4461 Wiper is disabled. All actions on this wiper are prohibited."); + case Mcp4461Component::MCP4461_WIPER_LOCKED: + return LOG_STR("MCP4461 Wiper is locked using WiperLock-technology. All actions on this wiper are prohibited."); case Mcp4461Component:::MCP4461_STATUS_OK: return LOG_STR("Status OK"); - case Mcp4461Component::MCP4461_STATUS_WRITE_PROTECTED: - return LOG_STR("MCP4461 is write protected. Setting nonvolatile wipers/eeprom values is prohibited."); - case Mcp4461Component::MCP4461_STATUS_WIPER_LOCKED: - return LOG_STR("MCP4461 Wiper is locked using WiperLock-technology. All actions on this wiper are prohibited."); default: return LOG_STR("Unknown"); } @@ -80,6 +87,9 @@ void Mcp4461Component::set_write_protection_status_() { void Mcp4461Component::dump_config() { ESP_LOGCONFIG(TAG, "mcp4461:"); LOG_I2C_DEVICE(this); + if (this->is_failed()) { + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); + } // log wiper status for (uint8_t i = 0; i < 8; ++i) { // terminals only valid for volatile wipers 0-3 - enable/disable is terminal hw @@ -118,9 +128,6 @@ void Mcp4461Component::dump_config() { ((status_register_value >> 5) & 0x01), ((status_register_value >> 4) & 0x01), ((status_register_value >> 3) & 0x01), ((status_register_value >> 2) & 0x01), ((status_register_value >> 1) & 0x01), ((status_register_value >> 0) & 0x01)); - if (this->is_failed()) { - ESP_LOGE(TAG, "Communication with mcp4461 failed!"); - } } void Mcp4461Component::loop() { @@ -157,16 +164,16 @@ void Mcp4461Component::loop() { uint8_t Mcp4461Component::get_status_register() { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); - return 0; + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); + return; } uint8_t reg = 0; reg |= static_cast(Mcp4461Addresses::MCP4461_STATUS); reg |= static_cast(Mcp4461Commands::READ); uint16_t buf; if (!this->read_byte_16(reg, &buf)) { + this->error_code_ = MCP4461_STATUS_REGISTER_ERROR; this->mark_failed(); - ESP_LOGE(TAG, "Error fetching status register value"); return 0; } uint8_t msb = buf >> 8; @@ -175,6 +182,7 @@ uint8_t Mcp4461Component::get_status_register() { if (msb != 1 || ((lsb >> 7) & 0x01) != 1 || ((lsb >> 1) & 0x01) != 1) { // D8, D7 and R1 bits are hardlocked to 1 -> a status msb bit 0 (bit 9 of status register) of 0 or lsb bit 1/7 = 0 // indicate device/communication issues, therefore mark component failed + this->error_code_ = MCP4461_STATUS_REGISTER_INVALID; this->mark_failed(); return 0; } @@ -214,9 +222,13 @@ uint8_t Mcp4461Component::get_wiper_address_(uint8_t wiper) { uint16_t Mcp4461Component::get_wiper_level(Mcp4461WiperIdx wiper) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return 0; } + if (!(this->reg_[wiper_idx].enabled)) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_DISABLED))); + return; + } uint8_t wiper_idx = static_cast(wiper); if (!(this->reg_[wiper_idx].enabled)) { ESP_LOGW(TAG, "reading from disabled volatile wiper %" PRIu8 ", returning 0", wiper_idx); @@ -236,6 +248,7 @@ uint16_t Mcp4461Component::read_wiper_level_(uint8_t wiper) { } } if (!(this->read_byte_16(reg, &buf))) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); ESP_LOGW(TAG, "Error fetching %swiper %" PRIu8 " value", (wiper > 3) ? "nonvolatile " : "", wiper); return 0; @@ -253,7 +266,7 @@ void Mcp4461Component::update_wiper_level(Mcp4461WiperIdx wiper) { void Mcp4461Component::set_wiper_level(Mcp4461WiperIdx wiper, uint16_t value) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return; } uint8_t wiper_idx = static_cast(wiper); @@ -262,13 +275,17 @@ void Mcp4461Component::set_wiper_level(Mcp4461WiperIdx wiper, uint16_t value) { return; } if (value > 0x100) { - ESP_LOGW(TAG, "ignoring invalid wiper level %" PRIu16 "!"); + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_VALUE_INVALID))); return; } if (!(this->reg_[wiper_idx].enabled)) { - ESP_LOGW(TAG, "writing to disabled volatile wiper %" PRIu8 " is prohibited", wiper_idx); + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_DISABLED))); return; } + if (this->reg_[wiper_idx].wiper_lock_active) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_LOCKED))); + return false; + } ESP_LOGV(TAG, "Setting MCP4461 wiper %" PRIu8 " to %" PRIu16 "!", wiper_idx, value); this->reg_[wiper_idx].state = value; this->update_ = true; @@ -280,17 +297,26 @@ void Mcp4461Component::write_wiper_level_(uint8_t wiper, uint16_t value) { nonvolatile = true; } if (!(this->mcp4461_write_(this->get_wiper_address_(wiper), value, nonvolatile))) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; + this->status_set_warning(); ESP_LOGW(TAG, "Error writing %swiper %" PRIu8 " level %" PRIu16 "", (wiper > 3) ? "nonvolatile " : "", wiper, value); - this->status_set_warning(); } } void Mcp4461Component::enable_wiper(Mcp4461WiperIdx wiper) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return; } + if ((this->reg_[wiper_idx].enabled)) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_ENABLED))); + return; + } + if (this->reg_[wiper_idx].wiper_lock_active) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_LOCKED))); + return false; + } uint8_t wiper_idx = static_cast(wiper); ESP_LOGV(TAG, "Enabling wiper %" PRIu8, wiper_idx); this->reg_[wiper_idx].terminal_hw = true; @@ -299,9 +325,17 @@ void Mcp4461Component::enable_wiper(Mcp4461WiperIdx wiper) { void Mcp4461Component::disable_wiper(Mcp4461WiperIdx wiper) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return; } + if (!(this->reg_[wiper_idx].enabled)) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_DISABLED))); + return; + } + if (this->reg_[wiper_idx].wiper_lock_active) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_LOCKED))); + return false; + } uint8_t wiper_idx = static_cast(wiper); ESP_LOGV(TAG, "Disabling wiper %" PRIu8, wiper_idx); this->reg_[wiper_idx].terminal_hw = false; @@ -310,16 +344,16 @@ void Mcp4461Component::disable_wiper(Mcp4461WiperIdx wiper) { bool Mcp4461Component::increase_wiper(Mcp4461WiperIdx wiper) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return false; } uint8_t wiper_idx = static_cast(wiper); if (!(this->reg_[wiper_idx].enabled)) { - ESP_LOGW(TAG, "increasing disabled volatile wiper %" PRIu8 " is prohibited", wiper_idx); - return false; + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_DISABLED))); + return; } if (this->reg_[wiper_idx].wiper_lock_active) { - ESP_LOGW(TAG, "Ignoring request to increase wiper %" PRIu8 " as it is locked by WiperLock", wiper_idx); + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_LOCKED))); return false; } ESP_LOGV(TAG, "Increasing wiper %" PRIu8 "", wiper_idx); @@ -330,6 +364,7 @@ bool Mcp4461Component::increase_wiper(Mcp4461WiperIdx wiper) { reg |= static_cast(Mcp4461Commands::INCREMENT); auto err = this->write(&this->address_, reg, sizeof(reg)); if (err != i2c::ERROR_OK) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); return false; } @@ -339,16 +374,16 @@ bool Mcp4461Component::increase_wiper(Mcp4461WiperIdx wiper) { bool Mcp4461Component::decrease_wiper(Mcp4461WiperIdx wiper) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return false; } uint8_t wiper_idx = static_cast(wiper); if (!(this->reg_[wiper_idx].enabled)) { - ESP_LOGW(TAG, "decreasing disabled volatile wiper %" PRIu8 " is prohibited", wiper_idx); - return false; + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_DISABLED))); + return; } if (this->reg_[wiper_idx].wiper_lock_active) { - ESP_LOGW(TAG, "Ignoring request to decrease wiper %" PRIu8 " as it is locked by WiperLock", wiper_idx); + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_LOCKED))); return false; } ESP_LOGV(TAG, "Decreasing wiper %" PRIu8 "", wiper_idx); @@ -359,6 +394,7 @@ bool Mcp4461Component::decrease_wiper(Mcp4461WiperIdx wiper) { reg |= static_cast(Mcp4461Commands::DECREMENT); auto err = this->write(&this->address_, reg, sizeof(reg)); if (err != i2c::ERROR_OK) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); return false; } @@ -392,7 +428,7 @@ uint8_t Mcp4461Component::calc_terminal_connector_byte_(Mcp4461TerminalIdx termi uint8_t Mcp4461Component::get_terminal_register(Mcp4461TerminalIdx terminal_connector) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return 0; } uint8_t reg = 0; @@ -406,6 +442,7 @@ uint8_t Mcp4461Component::get_terminal_register(Mcp4461TerminalIdx terminal_conn if (this->read_byte_16(reg, &buf)) { return static_cast(buf & 0x00ff); } else { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); ESP_LOGW(TAG, "Error fetching terminal register value"); return 0; @@ -442,7 +479,7 @@ void Mcp4461Component::update_terminal_register(Mcp4461TerminalIdx terminal_conn bool Mcp4461Component::set_terminal_register(Mcp4461TerminalIdx terminal_connector, uint8_t data) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return false; } uint8_t addr; @@ -455,6 +492,7 @@ bool Mcp4461Component::set_terminal_register(Mcp4461TerminalIdx terminal_connect return false; } if (!(this->mcp4461_write_(addr, data))) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); return false; } @@ -463,7 +501,7 @@ bool Mcp4461Component::set_terminal_register(Mcp4461TerminalIdx terminal_connect void Mcp4461Component::enable_terminal(Mcp4461WiperIdx wiper, char terminal) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return; } uint8_t wiper_idx = static_cast(wiper); @@ -493,7 +531,7 @@ void Mcp4461Component::enable_terminal(Mcp4461WiperIdx wiper, char terminal) { void Mcp4461Component::disable_terminal(Mcp4461WiperIdx wiper, char terminal) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return; } uint8_t wiper_idx = static_cast(wiper); @@ -523,7 +561,7 @@ void Mcp4461Component::disable_terminal(Mcp4461WiperIdx wiper, char terminal) { uint16_t Mcp4461Component::get_eeprom_value(Mcp4461EepromLocation location) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return 0; } uint8_t reg = 0; @@ -534,6 +572,7 @@ uint16_t Mcp4461Component::get_eeprom_value(Mcp4461EepromLocation location) { return 0; } if (!this->read_byte_16(reg, &buf)) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); ESP_LOGW(TAG, "Error fetching EEPRom location value"); return 0; @@ -543,7 +582,7 @@ uint16_t Mcp4461Component::get_eeprom_value(Mcp4461EepromLocation location) { bool Mcp4461Component::set_eeprom_value(Mcp4461EepromLocation location, uint16_t value) { if (this->is_failed()) { - ESP_LOGE(TAG, "%s", LOG_STR_ARG(LOG_PARENT_FAILED_STR)); + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); return false; } uint8_t addr = 0; @@ -555,7 +594,9 @@ bool Mcp4461Component::set_eeprom_value(Mcp4461EepromLocation location, uint16_t } addr |= static_cast(Mcp4461EepromLocation::MCP4461_EEPROM_1) + (static_cast(location) * 0x10); if (!(this->mcp4461_write_(addr, value, true))) { + this->error_code_ = MCP4461_STATUS_I2C_ERROR; this->status_set_warning(); + ESP_LOGW(TAG, "Error writing EEPRom value"); return false; } return true; @@ -658,7 +699,7 @@ bool Mcp4461Component::mcp4461_write_(uint8_t addr, uint16_t data, bool nonvolat reg |= static_cast(Mcp4461Commands::WRITE); if (nonvolatile) { if (this->write_protected_) { - ESP_LOGW(TAG, "Ignoring write to write protected chip"); + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WRITE_PROTECTED))); return false; } if (!this->is_eeprom_ready_for_writing_(true)) { From 544624c94a21a2d2daf18d6ff4fdf9ddf01e5ccb Mon Sep 17 00:00:00 2001 From: Oliver Kleinecke Date: Sat, 8 Feb 2025 16:48:22 +0100 Subject: [PATCH 11/11] Update mcp4461.cpp --- esphome/components/mcp4461/mcp4461.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/esphome/components/mcp4461/mcp4461.cpp b/esphome/components/mcp4461/mcp4461.cpp index bac92fc307..2b84997b99 100644 --- a/esphome/components/mcp4461/mcp4461.cpp +++ b/esphome/components/mcp4461/mcp4461.cpp @@ -257,6 +257,14 @@ uint16_t Mcp4461Component::read_wiper_level_(uint8_t wiper) { } void Mcp4461Component::update_wiper_level(Mcp4461WiperIdx wiper) { + if (this->is_failed()) { + ESP_LOGE(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(this->error_code_))); + return 0; + } + if (!(this->reg_[wiper_idx].enabled)) { + ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_WIPER_DISABLED))); + return; + } uint8_t wiper_idx = static_cast(wiper); uint16_t data; data = this->get_wiper_level(wiper); @@ -270,10 +278,6 @@ void Mcp4461Component::set_wiper_level(Mcp4461WiperIdx wiper, uint16_t value) { return; } uint8_t wiper_idx = static_cast(wiper); - if (this->reg_[wiper_idx].wiper_lock_active) { - ESP_LOGW(TAG, "Ignoring request to set the state for wiper %" PRIu8 " as it is locked by WiperLock", wiper_idx); - return; - } if (value > 0x100) { ESP_LOGW(TAG, "%s", LOG_STR_ARG(mcp4461_get_message_string_(MCP4461_VALUE_INVALID))); return;