esphome / issues

Issue Tracker for ESPHome
https://esphome.io/
290 stars 36 forks source link

AM2320 sensor component is unreliable with AM2315 #1354

Closed ei-ke closed 3 years ago

ei-ke commented 4 years ago

Operating environment/Installation (Hass.io/Docker/pip/etc.):

Arch Linux / PIP / HomeAssistant

ESP (ESP32/ESP8266, Board/Sonoff):

NodeMCU ESP8266

ESPHome version (latest production, beta, dev branch)

Version: 1.14.5

Affected component:

https://esphome.io/components/sensor/am2320.html

Description of problem:

I'm using the AM2315 sensor with the AM2320 sensor component. It is not working 100% of the time which you can see in the following logs. If I unplug the sensor and replug it to the I2C bus while the rest is alive it works. Sometimes it was also working after a cold boot of the entire setup which is powering the NodeMCU throught the VIN pin but also not everytime. Maybe this should rather become a feature request as I'm using the AM2315, but I thought it might just be a tiny change to the existing code.

Problem-relevant YAML-configuration entries:

# Example configuration entry
i2c:
    sda: D2
    scl: D1
    scan: True

sensor:  
  - platform: am2320
    temperature:
      name: "Lufttemperatur"
    humidity:
      name: "Luftfeuchtigkeit"
    update_interval: 10s

Logs (if applicable):

[11:36:45][W][i2c:070]: Received NACK on transmit of address 0x5C
[11:36:45][W][i2c:070]: Received NACK on transmit of address 0x5C
[11:36:45][W][am2320:074]: Writing bytes for AM2320 failed!
[11:36:45][W][am2320:089]: Updating AM2320 failed!
[11:36:48][V][sensor:013]: 'Zerfälle pro Minute': Received new state 18.000000
[11:36:48][D][sensor:092]: 'Zerfälle pro Minute': Sending state 18.00000 CPM with 0 decimals of accuracy
[11:36:48][V][sensor:013]: 'Dosisleistung': Received new state 0.102600
[11:36:48][D][sensor:092]: 'Dosisleistung': Sending state 0.10260 µSv/h with 4 decimals of accuracy
[11:36:51][D][pulse_counter:159]: 'Niederschlag aktuell': Retrieved counter: 1.00 pulses/min
[11:36:51][V][sensor:013]: 'Niederschlag aktuell': Received new state 1.000000
[11:36:51][D][sensor:092]: 'Niederschlag aktuell': Sending state 0.27731 mm/min with 2 decimals of accuracy
[11:36:55][V][sensor:013]: 'Wetterstation Betriebszeit': Received new state 38.993000
[11:36:55][D][sensor:092]: 'Wetterstation Betriebszeit': Sending state 38.99300 s with 0 decimals of accuracy
[11:36:55][V][bmp280.sensor:125]: Sending conversion request...
[11:36:55][D][bmp280.sensor:149]: Got temperature=23.2°C pressure=1009.0hPa
[11:36:55][V][sensor:013]: 'Lufttemperatur (BMP280)': Received new state 23.200001
[11:36:55][D][sensor:092]: 'Lufttemperatur (BMP280)': Sending state 23.20000 °C with 1 decimals of accuracy
[11:36:55][V][sensor:013]: 'Luftdruck': Received new state 1008.975464
[11:36:55][D][sensor:092]: 'Luftdruck': Sending state 1008.97546 hPa with 1 decimals of accuracy
[11:36:55][W][i2c:070]: Received NACK on transmit of address 0x5C
[11:36:56][D][am2320:044]: Got temperature=20.6°C humidity=64.8%
[11:36:56][V][sensor:013]: 'Lufttemperatur': Received new state 20.600000
[11:36:56][D][sensor:092]: 'Lufttemperatur': Sending state 20.60000 °C with 1 decimals of accuracy
[11:36:56][V][sensor:013]: 'Luftfeuchtigkeit': Received new state 64.800003
[11:36:56][D][sensor:092]: 'Luftfeuchtigkeit': Sending state 64.80000 % with 1 decimals of accuracy

Additional information and things you've tried: I've created a custom component based on this library which works flawlessly. Further I tried using setup_priority: -100 for the AM2320 sensor component which was mentioned here and once had a success around -50 but would rather call it a glitch as I couldn't reproduce it consistently. Also changing the update_interval for the component or using other I2C bus frequencys changed nothing relating to this issue. I also tried to compare your implemantation with the one of asukiaaa but that's beyond my "programming knowledge".

The YAML with the custom AM2315 component is this:

i2c:
    sda: D2
    scl: D1
    scan: True

sensor:
  - platform: custom
    lambda: |-
      auto AM2315sensor = new AM2315();
      App.register_component(AM2315sensor);
      return {AM2315sensor->temperature, AM2315sensor->humidity};

    sensors:
      - name: "Lufttemperatur"
        unit_of_measurement: °C
        accuracy_decimals: 1
      - name: "Luftfeuchtigkeit"
        unit_of_measurement: "%"
        accuracy_decimals: 1

AM2315.h

#include "esphome.h"
#include "AM2320_asukiaaa.h"

class AM2315 : public PollingComponent, public Sensor {
 public:
  AM2320_asukiaaa mySensor;

  Sensor *temperature = new Sensor();
  Sensor *humidity = new Sensor();

  AM2315() : PollingComponent (10000) {}

  void setup() override {
    // Initialize the device here. Usually Wire.begin() will be called in here,
    // though that call is unnecessary if you have an 'i2c:' entry in your config

    Wire.begin();
    mySensor.setWire(&Wire);
  }
  void update() override {
    if (mySensor.update() == 0) {
      float temp = mySensor.temperatureC;
      temperature->publish_state(temp);
      float humid = mySensor.humidity;
      humidity->publish_state(humid);
    }
  }
};
OttoWinter commented 4 years ago

CC @T3m3z (https://github.com/esphome/esphome/pull/554)

ei-ke commented 4 years ago

In case one of you needs an AM2315 I can ship and donate one or you let me know what it costs in your region and I'l send you the ammount and you can order one yourself.

T3m3z commented 4 years ago

I tried to check if I could find a significant difference from these two implementations but they seem to do essentially the same. Both wake up the sensor (needed to get the sensor ready for the actual read and this causes those NACK-errors). After that they send commands to read data (0x3, 0x0, 0x4) and then read 8 bytes (2 preamble, 2 temperature, 2 humidity, 2 CRC). Only difference seems to be that the library you are using has added 1.5ms delay between wakeup and the actual read.

This is just a wild guess but could it be that the AM2315 takes a bit longer to be operational after the wakeup call? It might be that on some boots the component gets marked as "failed" (row 57 in esphome/components/am2320/am2320.cpp) as it cannot read the data during the boot. Im not sure but ESPHome might not try to update the sensor after this has happened (source).

You seem to be quite capable with coding so you could try this with your custom component by commenting out row 42 (delay(2); // >1.5ms) in AM2320_asukiaaa.cpp . This test might reveal if the culprit is the absence of this delay.

ei-ke commented 4 years ago

Thanks for helping me T3m3z. Commenting out row 42 breaks my custom component silently (as I have no test included to see if the sensor is reading something or not). I've just tried modifying the am2320.cpp by adding a delay but I have no clue where exactly or it won't fix it. Not sure :/

T3m3z commented 4 years ago

So it seems that my wild guess was correct :)

Method AM2320Component::read_bytes_ has already support for that conversion delay - see lines 72 and 78-79 in esphome/components/am2320/am2320.cpp. You need to edit row 88 if (!this->read_bytes_(3, data, 8, 2)) { and change it to if (!this->read_bytes_(3, data, 8, 2, 2)) {. This will add 2ms delay between wakeup call and the actual read.

ei-ke commented 4 years ago

I already noticed this and changed the initial value of conversion in the header file to 2 (as I don't know what I'm doing :) ) Your recommended change throws the following error:

Compiling .pioenvs/wetterstation/src/esphome/components/am2320/am2320.cpp.o
src/esphome/components/am2320/am2320.cpp: In member function 'bool esphome::am2320::AM2320Component::read_data_(uint8_t*)':
src/esphome/components/am2320/am2320.cpp:88:42: error: no matching function for call to 'esphome::am2320::AM2320Component::read_bytes_(int, uint8_t*&, int, int, int)'
   if (!this->read_bytes_(3, data, 8, 2, 2)) {
                                          ^
src/esphome/components/am2320/am2320.cpp:88:42: note: candidate is:
src/esphome/components/am2320/am2320.cpp:72:6: note: bool esphome::am2320::AM2320Component::read_bytes_(uint8_t, uint8_t*, uint8_t, uint32_t)
 bool AM2320Component::read_bytes_(uint8_t a_register, uint8_t *data, uint8_t len, uint32_t conversion) {
      ^
src/esphome/components/am2320/am2320.cpp:72:6: note:   candidate expects 4 arguments, 5 provided
*** [.pioenvs/wetterstation/src/esphome/components/am2320/am2320.cpp.o] Error 1

Isn't the original call already handing over 2ms delay? 3, data, 8, 2 uint8_t a_register, uint8_t *data, uint8_t len, uint32_t conversion

T3m3z commented 4 years ago

Oh sorry. Yes it seems that you are right. Therefore let me withdraw that wild guess as it was completely wrong. You could try to modify the setup method and comment out the "if" in which the component is marked as failed if it does not respond correctly. Again, this is just a wild guess but it might be that the component gets marked as failed during bootup.

Or was it that after booting, the sensor does not work and you could get it to work by unplugging and replugging it?(You might have mentioned this but I am replying by email so I cannot see the original issue report.)

su 19. heinäk. 2020 klo 11.13 Eike notifications@github.com kirjoitti:

I already noticed this and changed the initial value of conversion in the header file to 2 (as I don't know what I'm doing :) ) You recommended change throws the following error

Compiling .pioenvs/wetterstation/src/esphome/components/am2320/am2320.cpp.o src/esphome/components/am2320/am2320.cpp: In member function 'bool esphome::am2320::AM2320Component::readdata(uint8_t)': src/esphome/components/am2320/am2320.cpp:88:42: error: no matching function for call to 'esphome::am2320::AM2320Component::readbytes(int, uint8_t&, int, int, int)' if (!this->readbytes(3, data, 8, 2, 2)) { ^ src/esphome/components/am2320/am2320.cpp:88:42: note: candidate is: src/esphome/components/am2320/am2320.cpp:72:6: note: bool esphome::am2320::AM2320Component::readbytes(uint8_t, uint8_t, uint8_t, uint32_t) bool AM2320Component::readbytes(uint8_t a_register, uint8_t data, uint8_t len, uint32_t conversion) { ^ src/esphome/components/am2320/am2320.cpp:72:6: note: candidate expects 4 arguments, 5 provided *** [.pioenvs/wetterstation/src/esphome/components/am2320/am2320.cpp.o] Error 1

Isn't the original call already handing over 2ms delay? 3, data, 8, 2 uint8_t a_register, uint8_t *data, uint8_t len, uint32_t conversion

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/esphome/issues/issues/1354#issuecomment-660606938, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRGTAGZBGRFLUYJOAITDD3R4KTKRANCNFSM4OZLTXVA .

ei-ke commented 4 years ago

I could get it to work after a hot unplug and replug. That's correct.

I played around (fumbled around in the dark) today and tried various things. Waking it up with begin_transmission, write_raw, end_transmission using the values from the working library (though the existing ones look like those in the data sheet). Changed the wakeup to just one byte / no byte - at least the datasheets looks to me like it needs just a I2C start and stop. Adding delays here and there, raising the existing delay from 2 to 5,10,20,1000.

I also commented out the mark as failed section which results in the two new errors: 1) write fail 2) read fail (which is in the right order as the write fail is the wakeup write).

I'll use the second AM2315 (thought maybe the first one has a defect before using the mentioned library) tomorrow and do all these changes again and leave them as a comment in my repo and notify you so you can have a look what doesn't help. I'll also hook up my el cheapo logic analyser just in case it shows something interesting.

ei-ke commented 4 years ago

Sorry for the delay. I've added my logic analyser recordings to this branch. The noticable difference to me is that with the custom library on cold boots it beginning with 4x Setup Write to ['184'] + NAK on the bus and with the internal implementation there are only 2x Setup Write to ['184'] + NAK. Further I purchased an AM2320 an can confirm that this component doesn't have these issues.

ei-ke commented 4 years ago

As promised this is what I've tried so far.

T3m3z commented 4 years ago

Interesting results (and nice logic analyzer logs!). I tried to check and see if I could see something significant in those but the only major difference seems to be that the custom library makes 3 of those Setup Write to ['184'] + NAK followed by one Setup Write to ['184'] + ACK (you mentioned that there were 4x Setup Write to ['184'] + NAK but I didn't see that). Anyway, this means that with the asukiaa library, the first wakeup and read attempt fails as the sensor does not respond. But the next attempt after 5 seconds seems to be successful. This makes me wonder if the sensor is somehow "slower" to wakeup during the cold boot. Possibly is needs some time before it can be read? Asuakiaa library gets over this by just repeating the reading process.

Was it that you tested removing the part where component is marked as failed during setup? Does the esphome continue trying to read the sensor every now and then or does it stop polling the sensor completely? In you logic analyzer dumps it seems that after cold boot it does try to read the sensor only once and the sensor does not respond (the same behaviour as with the asuakiaa library but that library just tries to read again and succeeds the second time it tries to read the data.

ei-ke commented 4 years ago

That was my fault. The working library has shown 3x NAK and 1x ACK as the log also shows.

Yes, I just removed the part of the code that marks the sensor as failed in ESPHome in this commit. Than it's trying to poll the sensor but fails with communication error in the ESPHome log and this is the logic analyser log. Further when I boot the original ESPHome code with the AM2320 sensor attached and just swap it with the AM2315 the measurements continue without any issue.

ei-ke commented 4 years ago

I've just tried it with delays in the setup section without success. Changes can be found here. Also going up to 5000ms or using the original code like the following doesn't work:

void AM2320Component::setup() {
  ESP_LOGCONFIG(TAG, "Setting up AM2320...");
  uint8_t data[8];
  data[0] = 0;
  data[1] = 4;
  this->read_data_(data);
  delay(5000);
  if (!this->read_data_(data)) {
    this->mark_failed();
    return;
  }
}
1994rstefan commented 4 years ago

I came accross this issue because I have problems using the AM2320 sensors. Maybe a lot of AM2320 work without a significant delay between wake and real read, but mine seem to have a problem with that. I also checked the datasheet which states that there must be a delay of at least 800µs. (https://datasheetspdf.com/pdf-file/952504/Aosong/AM2320/1 - Page 16)

I have also another device on the same I²C-Bus, so I checked if the second device is messing something up. But it seems, that the second request comes after 200µs, which is far below the minimum of 800µs. Maybe this issue also hardly depends on the used hardware because a slower microcontroller reaches the 800µs and mine does not? But regardles of that, a delay should get implemented to make sure the timing requirements are met.

ei-ke commented 4 years ago

Moin Stefan,

if you have an idea where this delay should be added or rather how please let me know and I'll try it and also add the logic analyser logs. As you can see here I have more or less no clue what to do.

1994rstefan commented 4 years ago

@ei-ke use the original am2320 code and try to add the delay on line 86

I need some sleep now - but I would check it in detail tomorrow if this does not fix it.

ei-ke commented 4 years ago

That magic moment - or WTF... You name it.

I have reset my branch to be even with dev to create a fresh logic analyser log while running the dev version. While doing so I changed

i2c:
    sda: D2
    scl: D1
    scan: False

to

i2c:
    sda: D1
    scl: D2
    scan: False

because... OCD... It matched the jumper cables better (yellow -> SDA like in my entire weather station) and the AM2315 is connected to the breadboard with crimped on ferrules on the wires which are a pain to push into the board... While writing this I have more doubts why I changed it. Anyway: Works out of the box without adding a delay. Interchanging D1/D2 in hardware as well as the config breaks the setup which is still the same as few weeks ago: NodeMCU, AM2315 on 3.3V, nothing else. Running version 1.14.5 installed via PIP the entire setup is broken. No matter to which of D1/D2 SDA/SCL is linked.

Now the interesting question is why and why the custom library just doesn't care and if the hint in the docs is up to date:

sda (Optional, Pin): The pin for the data line of the I²C bus. Defaults to the default of your board (usually GPIO21 for ESP32 and GPIO4 for ESP8266). scl (Optional, Pin): The pin for the clock line of the I²C bus. Defaults to the default of your board (usually GPIO22 for ESP32 and GPIO5 for ESP8266).

GPIO4 = D2 GPIO5 = D1 Maybe @OttoWinter has an opinion to this behaviour.

@1994rstefan I've just tried the dev version with my original sda: D2 and scl: D1 and the suggested delay: won't fix it.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

loujdev commented 3 years ago

I added the am2315 library to my project and used the function to read the temp and humidity at the same time and that works fine

loujdev commented 3 years ago

Yaml example of esphome:

esphome:
  includes:
    - AM2315.h
  libraries:
    - "Adafruit AM2315"
  name: koeling
  platform: ESP32
  board: nodemcu-32s #esp-wrover-kit

wifi:
  ssid: "wifiexample"
  password: "pass example"

# Enable logging
logger:

# Enable Home Assistant API
api:
  password: "passsss"

ota:
  password: "passssss"

i2c:
  sda: 21
  scl: 22
  scan: false

sensor:

  - platform: custom
    lambda: |-
      auto AM2315sensor = new AM2315();
      App.register_component(AM2315sensor);
      return {AM2315sensor->temperature_sensor, AM2315sensor->humidity_sensor};

    sensors:
      - name: "Temperature"
        unit_of_measurement: °C
        accuracy_decimals: 1
      - name: "Humidity"
        unit_of_measurement: "%"
        accuracy_decimals: 1

The file i place in /config/esphome of my home assistant

#include "esphome.h"

#include "Adafruit_AM2315.h"

class AM2315 : public PollingComponent, public Sensor {
 public:
  Adafruit_AM2315 mySensor;

  Sensor *temperature_sensor = new Sensor();
  Sensor *humidity_sensor = new Sensor();

  AM2315() : PollingComponent (10000) {}
  float get_setup_priority() const override { return esphome::setup_priority::BUS; }

  void setup() override {
    // Initialize the device here. Usually Wire.begin() will be called in here,
    // though that call is unnecessary if you have an 'i2c:' entry in your config

    mySensor.begin();

  }
  void update() override {
    // This is the actual sensor reading logic.
    float temperature;
    float humidity;

    mySensor.readTemperatureAndHumidity(&temperature, &humidity);

    temperature_sensor->publish_state(temperature);
    humidity_sensor->publish_state(humidity);
  }
};

and used the library here: https://github.com/adafruit/Adafruit_AM2315/blob/master/Adafruit_AM2315.h

snailbusa commented 3 years ago

hey!! -->> : https://github.com/snailbusa/AM2315-For-ESPHome <<-- @ei-ke

snailbusa commented 3 years ago

hey!! -->> : https://github.com/snailbusa/AM2315-For-ESPHome <<-- @ei-ke 2564-09-25 16_52_52-ESPHome - Home Assistant