[one_wire][dallas_temp] adjust timings and reduce disabled interrupts (#8744)

Co-authored-by: Samuel Sieb <samuel@sieb.net>

esphome/components/dallas_temp/dallas_temp.cpp

@@ -56,21 +56,13 @@ void DallasTemperatureSensor::update() {
   });
 }
 
-void IRAM_ATTR DallasTemperatureSensor::read_scratch_pad_int_() {
+bool DallasTemperatureSensor::read_scratch_pad_() {
+  bool success = this->send_command_(DALLAS_COMMAND_READ_SCRATCH_PAD);
+  if (success) {
     for (uint8_t &i : this->scratch_pad_) {
       i = this->bus_->read8();
     }
-}
+  } else {
-
-bool DallasTemperatureSensor::read_scratch_pad_() {
-  bool success;
-  {
-    InterruptLock lock;
-    success = this->send_command_(DALLAS_COMMAND_READ_SCRATCH_PAD);
-    if (success)
-      this->read_scratch_pad_int_();
-  }
-  if (!success) {
     ESP_LOGW(TAG, "'%s' - reading scratch pad failed bus reset", this->get_name().c_str());
     this->status_set_warning("bus reset failed");
   }
@@ -113,8 +105,6 @@ void DallasTemperatureSensor::setup() {
     return;
   this->scratch_pad_[4] = res;
 
-  {
-    InterruptLock lock;
   if (this->send_command_(DALLAS_COMMAND_WRITE_SCRATCH_PAD)) {
     this->bus_->write8(this->scratch_pad_[2]);  // high alarm temp
     this->bus_->write8(this->scratch_pad_[3]);  // low alarm temp
@@ -124,7 +114,6 @@ void DallasTemperatureSensor::setup() {
   // write value to EEPROM
   this->send_command_(DALLAS_COMMAND_COPY_SCRATCH_PAD);
 }
-}
 
 bool DallasTemperatureSensor::check_scratch_pad_() {
   bool chksum_validity = (crc8(this->scratch_pad_, 8) == this->scratch_pad_[8]);
@@ -138,6 +127,10 @@ bool DallasTemperatureSensor::check_scratch_pad_() {
   if (!chksum_validity) {
     ESP_LOGW(TAG, "'%s' - Scratch pad checksum invalid!", this->get_name().c_str());
     this->status_set_warning("scratch pad checksum invalid");
+    ESP_LOGD(TAG, "Scratch pad: %02X.%02X.%02X.%02X.%02X.%02X.%02X.%02X.%02X (%02X)", this->scratch_pad_[0],
+             this->scratch_pad_[1], this->scratch_pad_[2], this->scratch_pad_[3], this->scratch_pad_[4],
+             this->scratch_pad_[5], this->scratch_pad_[6], this->scratch_pad_[7], this->scratch_pad_[8],
+             crc8(this->scratch_pad_, 8));
   }
   return chksum_validity;
 }

esphome/components/dallas_temp/dallas_temp.h

@@ -23,7 +23,6 @@ class DallasTemperatureSensor : public PollingComponent, public sensor::Sensor,
   /// Get the number of milliseconds we have to wait for the conversion phase.
   uint16_t millis_to_wait_for_conversion_() const;
   bool read_scratch_pad_();
-  void read_scratch_pad_int_();
   bool check_scratch_pad_();
   float get_temp_c_();
 };

esphome/components/gpio/one_wire/gpio_one_wire.cpp

@@ -10,8 +10,10 @@ static const char *const TAG = "gpio.one_wire";
 void GPIOOneWireBus::setup() {
   ESP_LOGCONFIG(TAG, "Setting up 1-wire bus...");
   this->t_pin_->setup();
-  // clear bus with 480µs high, otherwise initial reset in search might fail
   this->t_pin_->pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
+  // clear bus with 480µs high, otherwise initial reset in search might fail
+  this->pin_.digital_write(true);
+  this->pin_.pin_mode(gpio::FLAG_OUTPUT);
   delayMicroseconds(480);
   this->search();
 }
@@ -22,40 +24,49 @@ void GPIOOneWireBus::dump_config() {
   this->dump_devices_(TAG);
 }
 
-bool HOT IRAM_ATTR GPIOOneWireBus::reset() {
+int HOT IRAM_ATTR GPIOOneWireBus::reset_int() {
+  InterruptLock lock;
   // See reset here:
   // https://www.maximintegrated.com/en/design/technical-documents/app-notes/1/126.html
   // Wait for communication to clear (delay G)
-  pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
+  this->pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
   uint8_t retries = 125;
   do {
     if (--retries == 0)
-      return false;
+      return -1;
     delayMicroseconds(2);
-  } while (!pin_.digital_read());
+  } while (!this->pin_.digital_read());
 
-  bool r;
+  bool r = false;
 
   // Send 480µs LOW TX reset pulse (drive bus low, delay H)
-  pin_.pin_mode(gpio::FLAG_OUTPUT);
+  this->pin_.digital_write(false);
-  pin_.digital_write(false);
+  this->pin_.pin_mode(gpio::FLAG_OUTPUT);
   delayMicroseconds(480);
 
   // Release the bus, delay I
-  pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
+  this->pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
-  delayMicroseconds(70);
+  uint32_t start = micros();
+  delayMicroseconds(30);
 
+  while (micros() - start < 300) {
     // sample bus, 0=device(s) present, 1=no device present
-  r = !pin_.digital_read();
+    r = !this->pin_.digital_read();
+    if (r)
+      break;
+    delayMicroseconds(1);
+  }
+
   // delay J
-  delayMicroseconds(410);
+  delayMicroseconds(start + 480 - micros());
-  return r;
+  this->pin_.digital_write(true);
+  this->pin_.pin_mode(gpio::FLAG_OUTPUT);
+  return r ? 1 : 0;
 }
 
 void HOT IRAM_ATTR GPIOOneWireBus::write_bit_(bool bit) {
   // drive bus low
-  pin_.pin_mode(gpio::FLAG_OUTPUT);
+  this->pin_.digital_write(false);
-  pin_.digital_write(false);
 
   // from datasheet:
   // write 0 low time: t_low0: min=60µs, max=120µs
@@ -64,72 +75,62 @@ void HOT IRAM_ATTR GPIOOneWireBus::write_bit_(bool bit) {
   // recovery time: t_rec: min=1µs
   // ds18b20 appears to read the bus after roughly 14µs
   uint32_t delay0 = bit ? 6 : 60;
-  uint32_t delay1 = bit ? 59 : 5;
+  uint32_t delay1 = bit ? 64 : 10;
 
   // delay A/C
   delayMicroseconds(delay0);
   // release bus
-  pin_.digital_write(true);
+  this->pin_.digital_write(true);
   // delay B/D
   delayMicroseconds(delay1);
 }
 
 bool HOT IRAM_ATTR GPIOOneWireBus::read_bit_() {
   // drive bus low
-  pin_.pin_mode(gpio::FLAG_OUTPUT);
+  this->pin_.digital_write(false);
-  pin_.digital_write(false);
 
-  // note: for reading we'll need very accurate timing, as the
+  // datasheet says >= 1µs
-  // timing for the digital_read() is tight; according to the datasheet,
+  delayMicroseconds(5);
-  // we should read at the end of 16µs starting from the bus low
-  // typically, the ds18b20 pulls the line high after 11µs for a logical 1
-  // and 29µs for a logical 0
-
-  uint32_t start = micros();
-  // datasheet says >1µs
-  delayMicroseconds(2);
 
   // release bus, delay E
-  pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
+  this->pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
-
-  // measure from start value directly, to get best accurate timing no matter
-  // how long pin_mode/delayMicroseconds took
-  uint32_t now = micros();
-  if (now - start < 12)
-    delayMicroseconds(12 - (now - start));
 
+  delayMicroseconds(8);
   // sample bus to read bit from peer
-  bool r = pin_.digital_read();
+  bool r = this->pin_.digital_read();
 
-  // read slot is at least 60µs; get as close to 60µs to spend less time with interrupts locked
+  // read slot is at least 60µs
-  now = micros();
+  delayMicroseconds(50);
-  if (now - start < 60)
-    delayMicroseconds(60 - (now - start));
 
+  this->pin_.digital_write(true);
+  this->pin_.pin_mode(gpio::FLAG_OUTPUT);
   return r;
 }
 
 void IRAM_ATTR GPIOOneWireBus::write8(uint8_t val) {
+  InterruptLock lock;
   for (uint8_t i = 0; i < 8; i++) {
     this->write_bit_(bool((1u << i) & val));
   }
 }
 
 void IRAM_ATTR GPIOOneWireBus::write64(uint64_t val) {
+  InterruptLock lock;
   for (uint8_t i = 0; i < 64; i++) {
     this->write_bit_(bool((1ULL << i) & val));
   }
 }
 
 uint8_t IRAM_ATTR GPIOOneWireBus::read8() {
+  InterruptLock lock;
   uint8_t ret = 0;
-  for (uint8_t i = 0; i < 8; i++) {
+  for (uint8_t i = 0; i < 8; i++)
     ret |= (uint8_t(this->read_bit_()) << i);
-  }
   return ret;
 }
 
 uint64_t IRAM_ATTR GPIOOneWireBus::read64() {
+  InterruptLock lock;
   uint64_t ret = 0;
   for (uint8_t i = 0; i < 8; i++) {
     ret |= (uint64_t(this->read_bit_()) << i);
@@ -144,6 +145,7 @@ void GPIOOneWireBus::reset_search() {
 }
 
 uint64_t IRAM_ATTR GPIOOneWireBus::search_int() {
+  InterruptLock lock;
   if (this->last_device_flag_)
     return 0u;
 

esphome/components/gpio/one_wire/gpio_one_wire.h

@@ -18,7 +18,6 @@ class GPIOOneWireBus : public one_wire::OneWireBus, public Component {
     this->pin_ = pin->to_isr();
   }
 
-  bool reset() override;
   void write8(uint8_t val) override;
   void write64(uint64_t val) override;
   uint8_t read8() override;
@@ -31,10 +30,12 @@ class GPIOOneWireBus : public one_wire::OneWireBus, public Component {
   bool last_device_flag_{false};
   uint64_t address_;
 
+  int reset_int() override;
   void reset_search() override;
   uint64_t search_int() override;
   void write_bit_(bool bit);
   bool read_bit_();
+  bool read_bit_(uint32_t *t);
 };
 
 }  // namespace gpio

esphome/components/one_wire/one_wire_bus.cpp

@@ -17,8 +17,15 @@ const uint8_t ONE_WIRE_ROM_SEARCH = 0xF0;
 
 const std::vector<uint64_t> &OneWireBus::get_devices() { return this->devices_; }
 
+bool OneWireBus::reset_() {
+  int res = this->reset_int();
+  if (res == -1)
+    ESP_LOGE(TAG, "1-wire bus is held low");
+  return res == 1;
+}
+
 bool IRAM_ATTR OneWireBus::select(uint64_t address) {
-  if (!this->reset())
+  if (!this->reset_())
     return false;
   this->write8(ONE_WIRE_ROM_SELECT);
   this->write64(address);
@@ -31,16 +38,13 @@ void OneWireBus::search() {
   this->reset_search();
   uint64_t address;
   while (true) {
-    {
+    if (!this->reset_()) {
-      InterruptLock lock;
-      if (!this->reset()) {
       // Reset failed or no devices present
       return;
     }
 
     this->write8(ONE_WIRE_ROM_SEARCH);
     address = this->search_int();
-    }
     if (address == 0)
       break;
     auto *address8 = reinterpret_cast<uint8_t *>(&address);

esphome/components/one_wire/one_wire_bus.h

@@ -9,14 +9,6 @@ namespace one_wire {
 
 class OneWireBus {
  public:
-  /** Reset the bus, should be done before all write operations.
-   *
-   * Takes approximately 1ms.
-   *
-   * @return Whether the operation was successful.
-   */
-  virtual bool reset() = 0;
-
   /// Write a word to the bus. LSB first.
   virtual void write8(uint8_t val) = 0;
 
@@ -50,6 +42,20 @@ class OneWireBus {
   /// log the found devices
   void dump_devices_(const char *tag);
 
+  /** Reset the bus, should be done before all write operations.
+   *
+   * Takes approximately 1ms.
+   *
+   * @return Whether the operation was successful.
+   */
+  bool reset_();
+
+  /**
+   * Bus Reset
+   * @return -1: signal fail, 0: no device detected, 1: device detected
+   */
+  virtual int reset_int() = 0;
+
   /// Reset the device search.
   virtual void reset_search() = 0;