crankyoldgit / IRremoteESP8266

Infrared remote library for ESP8266/ESP32: send and receive infrared signals with multiple protocols. Based on: https://github.com/shirriff/Arduino-IRremote/
GNU Lesser General Public License v2.1
2.94k stars 831 forks source link

sendPronto lib working bad with Gree Air conditioner prontohex codes #1855

Closed alejandrocolombo closed 2 years ago

alejandrocolombo commented 2 years ago

I am working with the latest 2.8.2 library using the "irsend.sendPronto" function, with gree brand air conditioners

this is the prontohex code or pattern where it has a small pause in the middle (02F7), the function doesn't interpret it as such

0000 006D 0046 0000 0158 00A9 001A 0013 001A 003D 001A 0013 001A 003D 001A 003D 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 003D 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 003D 001A 003D 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 003D 001A 0013 001A 003D 001A 0013 001A 0013 001A 003D 001A 0013 001A 02F7 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 003D 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0013 001A 0014 001A 0013 001A 0013 001A 0013 001A 0013 001A 003D 001A 003D 001A 01FF

attached photos where you can see with oscilloscope the code sent by a remote ok and another sent by the 2.8.2 library, where the pause is not performed

IMG_3063 IMG_3064

NiKiZe commented 2 years ago

You are using the lib to send pronto codes instead of using the gree or other internal classes?

alejandrocolombo commented 2 years ago

I am using sendPronto lib

IRsend irsend(IRpin);
irsend.sendPronto(code_array, code_len, code_repeats);

alejandrocolombo commented 2 years ago

I was debugging the IRsend::sendPronto function and it is correct, the problem is in the IRsend::space function when the time value is high

alejandrocolombo commented 2 years ago

with this change, it works ok !

Captura de Pantalla 2022-08-14 a la(s) 01 05 55

IMG_3075

crankyoldgit commented 2 years ago

@alejandrocolombo Can you please download and test branch: https://github.com/crankyoldgit/IRremoteESP8266/tree/Issue1855

I think there is an integer math overflow. I don't think it was being picked up by our unit tests as the compilers used use 32bit integers by default (I think), and the ESP's compilers use 16bit integers by default.

I am not certain it fixes it, but it probably does. If it doesn't we can work from there.

Btw, your proposed solution is not a good one. i.e. Your just spinning the CPU at 100% until the time is reached. That uses power, generates heat, and doesn't allow the RTOS on the ESP to do anything else.

That long 02F7 code produces a 20,000usec (20msec) pause. The equivelent of a delay(20); call. The OS can do a LOT in 20ms. Locking up the CPU for that long is wasteful. ;-)

alejandrocolombo commented 2 years ago

@crankyoldgit - I tried this code that I attached in the photo, that you uploaded, and the same problem does not work. :(

If you don't like my solution, I think we could use a timer with multiples of the time we need for the pause, in the function IRsend::space, maybe the solution should go that way.

Captura de Pantalla 2022-08-14 a la(s) 10 46 47

IMG_3077

crankyoldgit commented 2 years ago

That's interesting. BTW, can you please paste your code in, rather than with screen shots. It allows Cut & Pasteing ;-)

I am sure it is calling space(19961); in the middle there. See here: https://github.com/crankyoldgit/IRremoteESP8266/blob/8e4d2b472887d759f4dfa6b32e7bd99a791b01e4/test/ir_Pronto_test.cpp#L478

The question is why sendPronto() isn't working yet other protocols are.

Can get you to capture the output (oscilliscope) using TurnOnKelvinatorAC.ino & TurnOnGreeAC.ino using the SAME ESP chip, and the same GPIO for transmitting, and of course the same circuit. That should generate a similar length space() call. Please use that exact code, as is, except for what ever GPIO you need to switch it to.

I know those do produce a space(20000); in them, as I use them to control my own AC.

Your suggestion of:

I think we could use a timer with multiples of the time we need for the pause, in the function IRsend::space, maybe the solution should go that way.

is exactly what IRsend::space() does.

Something is odd/very broken for it to not be working.

NiKiZe commented 2 years ago

Pronto uses uint16 internally?

alejandrocolombo commented 2 years ago

this is the IRsend::space function working fine for me, inside IRsend.cpp. When i use irsend.sendPronto lib

void IRsend::space(uint32_t time) { ledOff(); if (time == 0) return;

unsigned long start = micros(); while (micros() - start < time) { }

///_delayMicroseconds(time); }

alejandrocolombo commented 2 years ago

this is the result with TurnOnKelvinatorAC.ino, I see a big space in the middle, I don't know why with the prontohex lib it doesn't work

IMG_3085

alejandrocolombo commented 2 years ago

What do you think about this?? "delayMicroseconds() is only accurate to 16383us.", the problem is totally exposed, your kelvinator codes that may work for you should not have such large spaces, you should use the code that I propose or improve your delay function to support higher delays

https://www.arduino.cc/en/Reference/delayMicroseconds Description Pauses the program for the amount of time (in microseconds) specified by the parameter. There are a thousand microseconds in a millisecond and a million microseconds in a second.

Currently, the largest value that will produce an accurate delay is 16383; larger values can produce an extremely short delay. This could change in future Arduino releases. For delays longer than a few thousand microseconds, you should use delay() instead.

/// An ESP8266 RTOS watch-dog timer friendly version of delayMicroseconds(). /// @param[in] usec Nr. of uSeconds to delay for. void IRsend::_delayMicroseconds(uint32_t usec) { // delayMicroseconds() is only accurate to 16383us. // Ref: https://www.arduino.cc/en/Reference/delayMicroseconds if (usec <= kMaxAccurateUsecDelay) {

ifndef UNIT_TEST

delayMicroseconds(usec);

endif

} else {

ifndef UNIT_TEST

// Invoke a delay(), where possible, to avoid triggering the WDT.
delay(usec / 1000UL);  // Delay for as many whole milliseconds as we can.
// Delay the remaining sub-millisecond.
delayMicroseconds(static_cast<uint16_t>(usec % 1000UL));

endif

}

crankyoldgit commented 2 years ago

The code you're referring to: https://github.com/crankyoldgit/IRremoteESP8266/blob/376fde20d21680306a6e4966413308a72fc6d6b3/src/IRsend.cpp#L112-L129 Should do the right thing. (and does so for other protocols) Kelvinator has spaces of ~20,000usecs & ~40,000usecs, and Gree has spaces of ~20,000usecs too. That is correct & verified; experimentally & programatically.

If you examine the code above, it doesn't ever make a call to delayMicroseconds() for longer than 16,383us. In the "normal" case, it actually works out how many Milliseconds it can delay for via delay() if we are over that value, and then does the remainer using delayMicroseconds().

So, lets walk through the _delayMicroseconds() code by hand for a space(19961); which is what is being invoked by the library in the sendPronto() code. (Note: This is the same approximate value as used in sendKelvinator & sendGree()) You've establised the code works (via the recent osciliscope reading).

so ... _delayMicroseconds(19961) called. The first thing it does is: if (usec <= kMaxAccurateUsecDelay) { i.e. Check if we are delaying for less that 16383us. usec has the value of 19961, so we skip that. i.e. 19961 > 16383 so we proceed to: delay(usec / 1000UL); // Delay for as many whole milliseconds as we can. usec / 1000 is 19961 / 1000 == 19 So it does a delay(19); call. ie. delay for 19 milli seconds. or. 19,000usec. (give or take a few usec it's not super accurate. It should be at least 19,000us though. Next line: delayMicroseconds(static_cast<uint16_t>(usec % 1000UL)); usec % 1000 is 19961 % 1000 == 961 So it will make a delayMicroseconds(961); call. Then it exits. So it will delay for 19ms (19000+ usecs) and then for 961 more usecs. Giving a total of a delay of >19961 usecs.

Now, experimentally, we know the Kelvinator example code is producing the ~20,000 usec gap as your osciliscope demonstrates. So, the library works. Now, for why your call of sendPronto() isn't working. The unit test I added in that branch indicates it's calling space(19961); As far as I can tell, the only way it can't be working (via your scope) is if the delay() function isn't working in your code/build/project. My guess is something must be causing that to not behave correctly. Can you simplify your code using sendPronto does to something very basic that you can share so I can try to reproduce it? Are you doing something that would affect the delay() call? Are you sure you're doing this in the Arduino Framework (not just in ESP8266/ESP32 native framework)? Have you modified kMaxAccurateUsecDelay at all? Have you changed/set/unset UNIT_TEST at all? Have you changed/set/unset ALLOW_DELAY_CALLS at all?

crankyoldgit commented 2 years ago

Pronto uses uint16 internally?

@NiKiZe Pronto codes are stored as uint16's, however, the operations on them should be all 32bit. The new test I put in that branch worked fine on the old code. i.e. It is/was doing the math correctly previously. It was a hail-mary to force everything into 32bit like I have. That's a belt & suspenders approach.

I'm fairly convinced (especially based on their working solution) that the delay() function isn't working as expected in his project's environment. Clearly the ESP can/does work with the delay() call when using our simple example code. So, the chip/board isn't faulty. Unit tests, code examination, and the example code point to the library working as expected/correctly. Something outside of the library (or it's been altered/corrupted/etc) is causing the issue the most likely candidate. I think .. Who knows. More data needed. :-/

NiKiZe commented 2 years ago

Pronto uses uint16 internally?

@NiKiZe Pronto codes are stored as uint16's, however, the operations on them should be all 32bit. The new test I put in that branch worked fine on the old code. i.e. It is/was doing the math correctly previously. It was a hail-mary to force everything into 32bit like I have. That's a belt & suspenders approach.

I'm fairly convinced (especially based on their working solution) that the delay() function isn't working as expected in his project's environment. Clearly the ESP can/does work with the delay() call when using our simple example code. So, the chip/board isn't faulty. Unit tests, code examination, and the example code point to the library working as expected/correctly. Something outside of the library (or it's been altered/corrupted/etc) is causing the issue the most likely candidate. I think .. Who knows. More data needed. :-/

Totally agree, and I don't think any more time should be spent at this at all based on that it has been demonstrated that is not in the library (we don't support external code), but also based on pure demands.

We love to create a working library for as many as possible. But not by being abused.

alejandrocolombo commented 2 years ago

I am using arduino IDE 1.8.19

Have you modified kMaxAccurateUsecDelay at all? (No) Have you changed/set/unset UNIT_TEST at all? (No) Have you changed/set/unset ALLOW_DELAY_CALLS at all? (No)

I tried cutting the code and trying only the part where it sends the "off - gree" command through the sendpronto library and it worked ok! with library 2.8.2, but when I use it in my complete program the same part of the code fails, surely there is a problem with the rest of the libraries that include my program, I'm still investigating the problem.

alejandrocolombo commented 2 years ago

Pronto uses uint16 internally?

@NiKiZe Pronto codes are stored as uint16's, however, the operations on them should be all 32bit. The new test I put in that branch worked fine on the old code. i.e. It is/was doing the math correctly previously. It was a hail-mary to force everything into 32bit like I have. That's a belt & suspenders approach. I'm fairly convinced (especially based on their working solution) that the delay() function isn't working as expected in his project's environment. Clearly the ESP can/does work with the delay() call when using our simple example code. So, the chip/board isn't faulty. Unit tests, code examination, and the example code point to the library working as expected/correctly. Something outside of the library (or it's been altered/corrupted/etc) is causing the issue the most likely candidate. I think .. Who knows. More data needed. :-/

Totally agree, and I don't think any more time should be spent at this at all based on that it has been demonstrated that is not in the library (we don't support external code), but also based on pure demands.

We love to create a working library for as many as possible. But not by being abused.

wow @NiKiZe, I apologize if my malfunction report annoyed you, but maybe others are going through the same thing when using the library in conjunction with others, since I changed the code of the "space" function and everything worked ok with "gree" brand commands for me. I hope not to bother you again, good luck in future with your projects.

crankyoldgit commented 2 years ago

Whoa! Every body Chill!

@NiKiZe I agree, it is probably not something wrong in the library per se. But something is interracting with it badly. It is totally worth getting to the bottom of why/what/etc so that others who have a similar issue can learn from this, and we may be able to prevent it pro-actively.

@alejandrocolombo There is not a lot we can easily do to help diagnose what is causing your issue, but I'm eager to find out the root cause. It seems from your & my debugging is pointing to something outside the library is influencing it. There are some known issues with some other libraries etc where they don't like or behave well with the use of delay() calls. e.g. AsyncWebserver See: https://github.com/crankyoldgit/IRremoteESP8266/blob/e7058c6aeace706bb8f4f2de2b3ae06a06cd4f03/src/IRremoteESP8266.h#L953-L959

Do you have a list of libraries you're using handy?

@NiKiZe Is right, in that we don't have the resources to debug your code/project for you. That's really out of scope. However, I am keen to help you isolate what is causing the issue so we can all learn from the underlying cause.

Ideally, this (issue) shouldn't happen. Preventing (if we can) it from happening in future is the goal here, the next best thing is to document a "Don't do this, otherwise sadness" case so we know for future issues.

alejandrocolombo commented 2 years ago

@crankyoldgit thank you your answer is from a gentleman, thanks for understanding that no one tries to prove anything here, we just want to help and contribute to those who use the library, with all humility.

I share the list of libraries that I am using in my project. ESP8266WiFi.h ESPAsyncWiFiManager.h WebAuthentication.h Ticker.h DoubleResetDetector.h FS.h
ESPAsyncTCP.h ESPAsyncWebServer.h AsyncElegantOTA.h Wire.h AHT10.h WiFiUdp.h IRremoteESP8266.h IRsend.h ArduinoJson.h DNSServer.h NewPing.h

I'm going to try to test what you mention about the AsyncWebserver library...

crankyoldgit commented 2 years ago

Your problem is ESPAsyncWebServer etc: See: https://github.com/me-no-dev/ESPAsyncWebServer#important-things-to-remember

Our library uses delay() calls by default. You can turn it off by setting ALLOW_DELAY_CALLS to false. See: https://github.com/crankyoldgit/IRremoteESP8266/blob/e7058c6aeace706bb8f4f2de2b3ae06a06cd4f03/src/IRremoteESP8266.h#L953-L959 Or don't send an IR message in a callback.

It used to be the case that the ESP would crash when a delay() call was used like that. Seems they stopped the crash, but now the delay call just doesn't behave normally.

See also: https://github.com/crankyoldgit/IRremoteESP8266/wiki/Frequently-Asked-Questions#what-libraries-are-known-to-be-incompatible-with-this-library

I'll extend that to mention ESPAsyncWebServer

crankyoldgit commented 2 years ago

I'll extend that to mention ESPAsyncWebServer

Done.

alejandrocolombo commented 2 years ago

Hello @crankyoldgit I wanted to tell you that I created a queue function for the received codes and sent them out of the callback, and everything worked ok, thanks for your help.

crankyoldgit commented 2 years ago

No worries. Glad we got to the bottom of it. And glad (in a way) that it wasn't a bug in my code. ;-)