Improve dallas timing (#3181)

* Improve dallas timing

* Format

esphome/components/dallas/dallas_component.cpp

@@ -32,6 +32,11 @@ void DallasComponent::setup() {
   ESP_LOGCONFIG(TAG, "Setting up DallasComponent...");
 
   pin_->setup();
+
+  // clear bus with 480µs high, otherwise initial reset in search_vec() fails
+  pin_->pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
+  delayMicroseconds(480);
+
   one_wire_ = new ESPOneWire(pin_);  // NOLINT(cppcoreguidelines-owning-memory)
 
   std::vector<uint64_t> raw_sensors;
@@ -99,20 +104,22 @@ void DallasComponent::update() {
   this->status_clear_warning();
 
   bool result;
-  if (!this->one_wire_->reset()) {
+  {
-    result = false;
+    InterruptLock lock;
-  } else {
+    result = this->one_wire_->reset();
-    result = true;
-    this->one_wire_->skip();
-    this->one_wire_->write8(DALLAS_COMMAND_START_CONVERSION);
   }
-
   if (!result) {
     ESP_LOGE(TAG, "Requesting conversion failed");
     this->status_set_warning();
     return;
   }
 
+  {
+    InterruptLock lock;
+    this->one_wire_->skip();
+    this->one_wire_->write8(DALLAS_COMMAND_START_CONVERSION);
+  }
+
   for (auto *sensor : this->sensors_) {
     this->set_timeout(sensor->get_address_name(), sensor->millis_to_wait_for_conversion(), [this, sensor] {
       bool res = sensor->read_scratch_pad();
@@ -152,16 +159,26 @@ const std::string &DallasTemperatureSensor::get_address_name() {
 }
 bool IRAM_ATTR DallasTemperatureSensor::read_scratch_pad() {
   auto *wire = this->parent_->one_wire_;
-  if (!wire->reset()) {
+
-    return false;
+  {
+    InterruptLock lock;
+
+    if (!wire->reset()) {
+      return false;
+    }
   }
 
-  wire->select(this->address_);
+  {
-  wire->write8(DALLAS_COMMAND_READ_SCRATCH_PAD);
+    InterruptLock lock;
 
-  for (unsigned char &i : this->scratch_pad_) {
+    wire->select(this->address_);
-    i = wire->read8();
+    wire->write8(DALLAS_COMMAND_READ_SCRATCH_PAD);
+
+    for (unsigned char &i : this->scratch_pad_) {
+      i = wire->read8();
+    }
   }
+
   return true;
 }
 bool DallasTemperatureSensor::setup_sensor() {
@@ -200,17 +217,20 @@ bool DallasTemperatureSensor::setup_sensor() {
   }
 
   auto *wire = this->parent_->one_wire_;
-  if (wire->reset()) {
+  {
-    wire->select(this->address_);
+    InterruptLock lock;
-    wire->write8(DALLAS_COMMAND_WRITE_SCRATCH_PAD);
+    if (wire->reset()) {
-    wire->write8(this->scratch_pad_[2]);  // high alarm temp
+      wire->select(this->address_);
-    wire->write8(this->scratch_pad_[3]);  // low alarm temp
+      wire->write8(DALLAS_COMMAND_WRITE_SCRATCH_PAD);
-    wire->write8(this->scratch_pad_[4]);  // resolution
+      wire->write8(this->scratch_pad_[2]);  // high alarm temp
-    wire->reset();
+      wire->write8(this->scratch_pad_[3]);  // low alarm temp
+      wire->write8(this->scratch_pad_[4]);  // resolution
+      wire->reset();
 
-    // write value to EEPROM
+      // write value to EEPROM
-    wire->select(this->address_);
+      wire->select(this->address_);
-    wire->write8(0x48);
+      wire->write8(0x48);
+    }
   }
 
   delay(20);  // allow it to finish operation

esphome/components/dallas/esp_one_wire.cpp

@@ -15,8 +15,6 @@ ESPOneWire::ESPOneWire(InternalGPIOPin *pin) { pin_ = pin->to_isr(); }
 bool HOT IRAM_ATTR ESPOneWire::reset() {
   // See reset here:
   // https://www.maximintegrated.com/en/design/technical-documents/app-notes/1/126.html
-  InterruptLock lock;
-
   // Wait for communication to clear (delay G)
   pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
   uint8_t retries = 125;
@@ -43,16 +41,18 @@ bool HOT IRAM_ATTR ESPOneWire::reset() {
 }
 
 void HOT IRAM_ATTR ESPOneWire::write_bit(bool bit) {
-  // See write 1/0 bit here:
-  // https://www.maximintegrated.com/en/design/technical-documents/app-notes/1/126.html
-  InterruptLock lock;
-
   // drive bus low
   pin_.pin_mode(gpio::FLAG_OUTPUT);
   pin_.digital_write(false);
 
-  uint32_t delay0 = bit ? 10 : 65;
+  // from datasheet:
-  uint32_t delay1 = bit ? 55 : 5;
+  // write 0 low time: t_low0: min=60µs, max=120µs
+  // write 1 low time: t_low1: min=1µs, max=15µs
+  // time slot: t_slot: min=60µs, max=120µs
+  // 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 ? 54 : 5;
 
   // delay A/C
   delayMicroseconds(delay0);
@@ -63,72 +63,100 @@ void HOT IRAM_ATTR ESPOneWire::write_bit(bool bit) {
 }
 
 bool HOT IRAM_ATTR ESPOneWire::read_bit() {
-  // See read bit here:
+  // drive bus low
-  // https://www.maximintegrated.com/en/design/technical-documents/app-notes/1/126.html
-  InterruptLock lock;
-
-  // drive bus low, delay A
   pin_.pin_mode(gpio::FLAG_OUTPUT);
   pin_.digital_write(false);
+
+  // note: for reading we'll need very accurate timing, as the
+  // timing for the digital_read() is tight; according to the datasheet,
+  // 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(3);
 
   // release bus, delay E
   pin_.pin_mode(gpio::FLAG_INPUT | gpio::FLAG_PULLUP);
-  delayMicroseconds(10);
+
+  // Unfortunately some frameworks have different characteristics than others
+  // esp32 arduino appears to pull the bus low only after the digital_write(false),
+  // whereas on esp-idf it already happens during the pin_mode(OUTPUT)
+  // manually correct for this with these constants.
+
+#ifdef USE_ESP32_FRAMEWORK_ARDUINO
+  uint32_t timing_constant = 14;
+#elif defined(USE_ESP32_FRAMEWORK_ESP_IDF)
+  uint32_t timing_constant = 12;
+#else
+  uint32_t timing_constant = 14;
+#endif
+
+  // measure from start value directly, to get best accurate timing no matter
+  // how long pin_mode/delayMicroseconds took
+  while (micros() - start < timing_constant)
+    ;
 
   // sample bus to read bit from peer
   bool r = pin_.digital_read();
 
-  // delay F
+  // read slot is at least 60µs; get as close to 60µs to spend less time with interrupts locked
-  delayMicroseconds(53);
+  uint32_t now = micros();
+  if (now - start < 60)
+    delayMicroseconds(60 - (now - start));
+
   return r;
 }
 
-void ESPOneWire::write8(uint8_t val) {
+void IRAM_ATTR ESPOneWire::write8(uint8_t val) {
   for (uint8_t i = 0; i < 8; i++) {
     this->write_bit(bool((1u << i) & val));
   }
 }
 
-void ESPOneWire::write64(uint64_t val) {
+void IRAM_ATTR ESPOneWire::write64(uint64_t val) {
   for (uint8_t i = 0; i < 64; i++) {
     this->write_bit(bool((1ULL << i) & val));
   }
 }
 
-uint8_t ESPOneWire::read8() {
+uint8_t IRAM_ATTR ESPOneWire::read8() {
   uint8_t ret = 0;
   for (uint8_t i = 0; i < 8; i++) {
     ret |= (uint8_t(this->read_bit()) << i);
   }
   return ret;
 }
-uint64_t ESPOneWire::read64() {
+uint64_t IRAM_ATTR ESPOneWire::read64() {
   uint64_t ret = 0;
   for (uint8_t i = 0; i < 8; i++) {
     ret |= (uint64_t(this->read_bit()) << i);
   }
   return ret;
 }
-void ESPOneWire::select(uint64_t address) {
+void IRAM_ATTR ESPOneWire::select(uint64_t address) {
   this->write8(ONE_WIRE_ROM_SELECT);
   this->write64(address);
 }
-void ESPOneWire::reset_search() {
+void IRAM_ATTR ESPOneWire::reset_search() {
   this->last_discrepancy_ = 0;
   this->last_device_flag_ = false;
   this->last_family_discrepancy_ = 0;
   this->rom_number_ = 0;
 }
-uint64_t ESPOneWire::search() {
+uint64_t IRAM_ATTR ESPOneWire::search() {
   if (this->last_device_flag_) {
     return 0u;
   }
 
-  if (!this->reset()) {
+  {
-    // Reset failed or no devices present
+    InterruptLock lock;
-    this->reset_search();
+    if (!this->reset()) {
-    return 0u;
+      // Reset failed or no devices present
+      this->reset_search();
+      return 0u;
+    }
   }
 
   uint8_t id_bit_number = 1;
@@ -137,58 +165,61 @@ uint64_t ESPOneWire::search() {
   bool search_result = false;
   uint8_t rom_byte_mask = 1;
 
-  // Initiate search
+  {
-  this->write8(ONE_WIRE_ROM_SEARCH);
+    InterruptLock lock;
-  do {
+    // Initiate search
-    // read bit
+    this->write8(ONE_WIRE_ROM_SEARCH);
-    bool id_bit = this->read_bit();
+    do {
-    // read its complement
+      // read bit
-    bool cmp_id_bit = this->read_bit();
+      bool id_bit = this->read_bit();
+      // read its complement
+      bool cmp_id_bit = this->read_bit();
 
-    if (id_bit && cmp_id_bit) {
+      if (id_bit && cmp_id_bit) {
-      // No devices participating in search
+        // No devices participating in search
-      break;
+        break;
-    }
-
-    bool branch;
-
-    if (id_bit != cmp_id_bit) {
-      // only chose one branch, the other one doesn't have any devices.
-      branch = id_bit;
-    } else {
-      // there are devices with both 0s and 1s at this bit
-      if (id_bit_number < this->last_discrepancy_) {
-        branch = (this->rom_number8_()[rom_byte_number] & rom_byte_mask) > 0;
-      } else {
-        branch = id_bit_number == this->last_discrepancy_;
       }
 
-      if (!branch) {
+      bool branch;
-        last_zero = id_bit_number;
+
-        if (last_zero < 9) {
+      if (id_bit != cmp_id_bit) {
-          this->last_discrepancy_ = last_zero;
+        // only chose one branch, the other one doesn't have any devices.
+        branch = id_bit;
+      } else {
+        // there are devices with both 0s and 1s at this bit
+        if (id_bit_number < this->last_discrepancy_) {
+          branch = (this->rom_number8_()[rom_byte_number] & rom_byte_mask) > 0;
+        } else {
+          branch = id_bit_number == this->last_discrepancy_;
+        }
+
+        if (!branch) {
+          last_zero = id_bit_number;
+          if (last_zero < 9) {
+            this->last_discrepancy_ = last_zero;
+          }
         }
       }
-    }
 
-    if (branch) {
+      if (branch) {
-      // set bit
+        // set bit
-      this->rom_number8_()[rom_byte_number] |= rom_byte_mask;
+        this->rom_number8_()[rom_byte_number] |= rom_byte_mask;
-    } else {
+      } else {
-      // clear bit
+        // clear bit
-      this->rom_number8_()[rom_byte_number] &= ~rom_byte_mask;
+        this->rom_number8_()[rom_byte_number] &= ~rom_byte_mask;
-    }
+      }
 
-    // choose/announce branch
+      // choose/announce branch
-    this->write_bit(branch);
+      this->write_bit(branch);
-    id_bit_number++;
+      id_bit_number++;
-    rom_byte_mask <<= 1;
+      rom_byte_mask <<= 1;
-    if (rom_byte_mask == 0u) {
+      if (rom_byte_mask == 0u) {
-      // go to next byte
+        // go to next byte
-      rom_byte_number++;
+        rom_byte_number++;
-      rom_byte_mask = 1;
+        rom_byte_mask = 1;
-    }
+      }
-  } while (rom_byte_number < 8);  // loop through all bytes
+    } while (rom_byte_number < 8);  // loop through all bytes
+  }
 
   if (id_bit_number >= 65) {
     this->last_discrepancy_ = last_zero;
@@ -217,7 +248,7 @@ std::vector<uint64_t> ESPOneWire::search_vec() {
 
   return res;
 }
-void ESPOneWire::skip() {
+void IRAM_ATTR ESPOneWire::skip() {
   this->write8(0xCC);  // skip ROM
 }
 

esphome/core/helpers.cpp

@@ -328,6 +328,8 @@ void hsv_to_rgb(int hue, float saturation, float value, float &red, float &green
 IRAM_ATTR InterruptLock::InterruptLock() { xt_state_ = xt_rsil(15); }
 IRAM_ATTR InterruptLock::~InterruptLock() { xt_wsr_ps(xt_state_); }
 #elif defined(USE_ESP32)
+// only affects the executing core
+// so should not be used as a mutex lock, only to get accurate timing
 IRAM_ATTR InterruptLock::InterruptLock() { portDISABLE_INTERRUPTS(); }
 IRAM_ATTR InterruptLock::~InterruptLock() { portENABLE_INTERRUPTS(); }
 #endif