strange-v / MHZ19

Arduino library for MH-Z19/MH-Z19B CO2 sensor
GNU General Public License v3.0
57 stars 12 forks source link

ICACHE_RAM_ATTR attribute fix for esp8266 and esp32 boards #11

Closed Di-Strix closed 3 years ago

Di-Strix commented 3 years ago

Added ICACHE_RAM_ATTR attribute to isr functions for esp8266 and esp32 boards. Added yield() to waitForData function to avoid WDT reset on esp boards. Fixes issue #9

strange-v commented 3 years ago

Thank you for contributing, could you fix a few things?

  1. Please use a similar approach to this https://github.com/strange-v/FT6X36/blob/7507eea1128b805508775894d6ad125f2d5705fd/FT6X36.h#L7
  2. I know that for ESP32 ICACHE_RAM_ATTR is defined as IRAM_ATTR in esp8266-compat.h, but I'm not sure that this backward compatibility fix is permanent. Please, use IRAM_ATTR for ESP32.
Di-Strix commented 3 years ago

@strange-v Done

Di-Strix commented 3 years ago

A few more minor changes (commented). Is it okay to have IRAM_ATTR only in definition (.cpp) or should it be in the declaration too (.h)? Honestly, I don't remember.

Because of you're including header file in cpp file then all contents of both files are "merging" together. So you need to define or in the header file(.h) or in the cpp file. In most cases all defines are in the header file, but nothing is stopping you from moving them to the cpp file.

Move IRAM_ATTR define to the MHZ19PWM.cpp?

strange-v commented 3 years ago

No, defines should be in the header, it's logical. I'm talking about attributing the method, for example, MHZ19PWM::isr() attributed (ISR_FN_ATTR) in the cpp file, but not in the header. If it is possible to attribute only in one place I'd prefer the header file.

Di-Strix commented 3 years ago

I just tested and seems it is worked. So I'm moving ISR_ATTR to the header

strange-v commented 3 years ago

Thank you!