IRMP-org / IRMP

Infrared Multi Protocol Decoder
GNU General Public License v3.0
273 stars 44 forks source link

[FR] Make PIN configuration at runtime #7

Closed eku closed 3 years ago

eku commented 4 years ago

To make IRMP integrable into the Tasmota project, it must be possible to assign the pins at runtime.

There is another project that offers this possibility and is even already integrated in Tasmota, but does not offer the protocol support of IRMP by far. I would like to integrate IRMP into Tasmota.

ArminJo commented 4 years ago

I just commited a first version where you can define ALLOW_DYNAMIC_INPUT_PIN and call irmp_set_input_pin() to set the pin. Is this OK for you?

eku commented 4 years ago

To be honest, I wished for a more Arduino-like implementation, which makes the integration object-oriented and passes the pin assignment in the constructor.

ArminJo commented 4 years ago

Ok, I can see the point. Because there is only one object possible, it makes no sense to wrap it in an object, but now the irmpInit(PinNumber) function is provided (instead of a constructor). I added an example ReceiveAndSendDynamicPins. The whole thing is not tested yet.

eku commented 4 years ago

Can the function irmpInit(PinNumber) be called more than once if the pin configuration changes at runtime?

ArminJo commented 4 years ago

It must, otherwise it makes no sense as replacement for a constructor (and must therefore be corrected). You can use disableIRTimerInterrupt() as destructor if you want. If it is required, I can deliver this as alias irmpDeInit().

Are you interested in receiving IR by Pin Change Interrupts? This is not possible for every protocol and currently not impleneted for dynamic pin numbers but saves some CPU resources.

eku commented 4 years ago

On this occasion I noticed that in library.json the old version is still there instead of 3.0.0.

ArminJo commented 4 years ago

Thanks, but 2.2.1 is the latest release available for arduino library manager, 3.0.0 is still under construction.

eku commented 4 years ago

Please correct the function names in README.md. No camel case but underscore.

ArminJo commented 4 years ago

Ok, but which function name do you mean?

eku commented 4 years ago

Ok, but which function name do you mean?

Well, the functions that do not exist in the source code;) Look at the new chapter for dynamic pins.

eku commented 4 years ago

Obviously there is no way to build IRMP without feedback LED, because the code assumes the definition of LED_BUILTIN. Could you please make this configurable by a define.

ArminJo commented 4 years ago

I do not understand totally. Do you mean to add:

#ifndef LED_BUILTIN
#define LED_BUILTIN 0
#endif
eku commented 4 years ago
lib/IRMP-3.0.0/src/irmpArduinoExt.cpp.h: In function 'void irmp_LEDFeedback(bool)':
lib/IRMP-3.0.0/src/irmpArduinoExt.cpp.h:70:21: error: 'LED_BUILTIN' was not declared in this scope
         pinModeFast(LED_BUILTIN, OUTPUT);
                     ^
In file included from tasmota/xdrv_05_irmp.ino:31:0:
lib/IRMP-3.0.0/src/irmp.c.h: In function 'uint8_t irmp_ISR()':
lib/IRMP-3.0.0/src/irmp.c.h:3085:22: error: 'LED_BUILTIN' was not declared in this scope
         digitalWrite(LED_BUILTIN, !irmp_input);

Code requires LED_BUILTIN to be defined. If the board does not provide a LED or I simply do not want to use the feature, the code does not compile. That's what I mean. I suggest to enclose every usage of LED_BUILTIN with, i.e.

#ifdef LED_BUILDIN
  digitalWrite(LED_BUILTIN,...)
#endif
ArminJo commented 4 years ago

Ok, lets see what I can make.

eku commented 4 years ago

Ideally, the assignment for the LED can also be given as a parameter for initialization, i.e. set at runtime. That would be perfect and would fit well with the Arduino style.

ArminJo commented 4 years ago

OK, Done, Like to test it?

eku commented 4 years ago

Like to test it?

Yes, will test it....

eku commented 4 years ago

@ArminJo Hi Armin,

I call irmp_init(Pin(GPIO_IRRECV)) because there is no LED. This still leads to a compiler error.

In file included from lib/IRMP-master/src/irmpArduinoExt.cpp.h:19:0,
                 from lib/IRMP-master/src/irmp.c.h:3094,
                 from tasmota/xdrv_05_irmp.ino:30:
lib/IRMP-master/src/IRFeedbackLed.cpp.h: In function 'void irmp_irsnd_LEDFeedback(bool)':
lib/IRMP-master/src/IRFeedbackLed.cpp.h:49:21: error: 'LED_BUILTIN' was not declared in this scope
         pinModeFast(LED_BUILTIN, OUTPUT);
                     ^
lib/IRMP-master/src/IRFeedbackLed.cpp.h: In function 'void irmp_irsnd_SetFeedbackLED(bool)':
lib/IRMP-master/src/IRFeedbackLed.cpp.h:98:18: error: 'LED_BUILTIN' was not declared in this scope 
     digitalWrite(LED_BUILTIN, aSwitchLedOn);
                  ^
ArminJo commented 4 years ago

Line 49 is guarded with

#if defined(IRMP_IRSND_ALLOW_DYNAMIC_PINS)
        pinMode(irmp_irsnd_LedFeedbackPin, OUTPUT);
...
#else
 LINE 49       pinModeFast(LED_BUILTIN, OUTPUT);
eku commented 4 years ago

@ArminJo thanks, I had a typo in the define.

Now that I have corrected the typo, the source code compiles with warnings.

In file included from tasmota/xdrv_05_irmp.ino:32:0:
lib/IRMP-master/src/irsnd.c.h:311:0: warning: "DENON_AUTO_REPETITION_PAUSE_LEN" redefined
 #define DENON_AUTO_REPETITION_PAUSE_LEN         (uint16_t)(F_INTERRUPTS * DENON_AUTO_REPETITION_PAUSE_TIME + 0.5)               // use uint16_t!
 ^
In file included from tasmota/xdrv_05_irmp.ino:31:0: 
lib/IRMP-master/src/irmp.c.h:265:0: note: this is the location of the previous definition
 #define DENON_AUTO_REPETITION_PAUSE_LEN         ((uint_fast16_t)(F_INTERRUPTS * DENON_AUTO_REPETITION_PAUSE_TIME * MAX_TOLERANCE_10 + 0.5) + 1)
 ^
In file included from tasmota/xdrv_05_irmp.ino:32:0:
lib/IRMP-master/src/irsnd.c.h:520:0: warning: "ROOMBA_0_PAUSE_LEN" redefined
 #define ROOMBA_0_PAUSE_LEN                      (uint8_t)(F_INTERRUPTS * ROOMBA_0_PAUSE_TIME + 0.5)
 ^
In file included from tasmota/xdrv_05_irmp.ino:31:0:
lib/IRMP-master/src/irmp.c.h:550:0: note: this is the location of the previous definition
 #define ROOMBA_0_PAUSE_LEN                      ((uint_fast8_t)(F_INTERRUPTS * ROOMBA_0_PAUSE_TIME))
 ^
In file included from tasmota/xdrv_05_irmp.ino:32:0:
lib/IRMP-master/src/irsnd.c.h:528:0: warning: "PENTAX_0_PAUSE_LEN" redefined
 #define PENTAX_0_PAUSE_LEN                      (uint8_t)(F_INTERRUPTS * PENTAX_0_PAUSE_TIME + 0.5)
 ^
In file included from tasmota/xdrv_05_irmp.ino:31:0:
lib/IRMP-master/src/irmp.c.h:580:0: note: this is the location of the previous definition
 #define PENTAX_0_PAUSE_LEN                      ((uint_fast8_t)(F_INTERRUPTS * PENTAX_0_PAUSE_TIME))
 ^

Since the values of the defines are different, I could imagine that this leads to problems. Maybe it would be better to rename the defines to avoid conflicts when using IRMP and IRSND at the same time.

eku commented 4 years ago

Both the call to irmp_init(13) and irsnd_init(14) will cause an immediate crash and restart in an infinite loop on an ESP32. 13 and 14 are the GPIOs.

ArminJo commented 4 years ago

Standalone example ReceiveAndSend works for me. Do you have Wifi or BT running? Do you have the complete code to test?

eku commented 4 years ago

Please find attached my driver for Tasmota. Yes Tasmota uses WiFi.

ArminJo commented 4 years ago
  1. include (which defines additional values) must before #include .

Can you specify what combination works (eg irmp_init on esp8266) and what crashes?

Maybe the IRMP timer usage is incompatible with Tasmota, but I have no knowledge about Tasmota Timer usage and restrictions.

eku commented 4 years ago

Hi @ArminJo

Thanks for the helpful instructions. The crash happens in irmp_DoLEDFeedback during the interrupt.

During debugging I noticed the following:

It would be helpful if you could also specify the timer to use during initialization, since the integration in Tasmota would have to use timer 3.

The discussion about LED for feedback was already mentioned above. You have different signatures of irmp_init and irsnd_init which can be called without aFeedbackLedPin and then Pin 0 is associated. In the code there is a comment "avoid activating feedback LED by using 0 as led pin". I don't know if it is a good idea to do a digitalWrite(0, ...) then. Probably this will trigger the crash.

ArminJo commented 4 years ago

Is there a symbol set by Tasmota, so I can check for #if defined(TASMOTA) ... use Timer 3? I changed it now globally to timer 3. Regarding the LED.: sorry You are totally right, I missed to check for pin != 0. IRFeedbackLed.h is now fixed.

eku commented 4 years ago

Is there a symbol set by Tasmota, so I can check for #if defined(TASMOTA) ... use Timer 3?

Well, not that I know off. For a library it would be better not to depend on external defines.

I am still searching for the cause of the crash, because the last change alone is obviously not enough. Therefore I have taken IRMP completely out and try to get IRSND running on its own. But this case is apparently not planned, because the compilation fails with

lib/IRMP-master/src/irsndArduinoExt.cpp.h: In function 'void irsnd_init(uint_fast8_t)':
lib/IRMP-master/src/irsndArduinoExt.cpp.h:85:40: error: 'irmp_init' was not declared in this scope
     irmp_init(aIrsndOutputPin, 0, false);

According to the example SimpleSender it is sufficient to include irsnd.c.h. And that's exactly what I did. Is again a problem with the feedback LED. I wonder if the line

#if defined(ARDUINO)
    irmp_DoLEDFeedback(irmp_input);
#endif

are necessary at all. Doesn't @ukw100 have IRMP_USE_CALLBACK provided for this?

ArminJo commented 4 years ago

Hi Erik, thanks for testing, you found another copy and paste bug 🥇 I fixed it in irsndArduinoExt.cpp.h.

BTW. All libraries depend on special symbols set by the environment if the environment has some restrichtions, as you can see for Arduino, Teensy, Digispark etc.

eku commented 4 years ago

Hi Armin, I have now found out that calling irmp_DoLEDFeedback from irmp_ISR causes a crash. No idea in which line it happens. If no LED is defined, neither the initialization nor the activation of the LED should ever be called.

Can you please test and reproduce this?

ArminJo commented 4 years ago

Hi Erik, I have no idea how to test it. Do you have a platformio workspace I can copy? I personally work with sloeber (Eclipse plugin for Arduino). Please help.

eku commented 4 years ago

The path to knowledge is rocky. On the serial console Core 1 panic'ed (Cache disabled but cached memory region accessed) is output. According to this issue this means that the ISR calls code outside the IRAM. And that's where my knowledge of Arduino and ESP32 ends.

Do I now have to move irmp_irsnd_SetFeedbackLED into the IRAM and what about the functions called therein for port output? Since the latter are actually not called in my configuration (no LED configured), the call of irmp_irsnd_SetFeedbackLED probably already causes a crash. Anyway, it does not crash when I comment out the call in irmp_ISR.

Do you have an ESP32 and can test the library completely independent from Tasmota with the extensions of the dynamic pins and the feedback LED?

Another one:

Beware, not only the ISR (Interrupt) has to be in IRAM! Every function called from the ISR also needs to be declared as IRAM_ATTR yourFunction() {}.

ArminJo commented 4 years ago

Hi, Of couse all examples run on my ESP32 and were tested. But if the code size is small, then irmp_irsnd_SetFeedbackLED is automatically put in IRAM.

Please change line 82 in IRFeedbackLed.cpp.h from void irmp_irsnd_SetFeedbackLED(bool aSwitchLedOn) to void IRAM_ATTR irmp_irsnd_SetFeedbackLED(bool aSwitchLedOn) and check it again. I also made the change in the sources.

eku commented 4 years ago

Hi, tanks. Did you miss irmp_DoLEDFeedback?

ArminJo commented 4 years ago

Hi, tanks. Did you miss irmp_DoLEDFeedback? Yep you are right!!! and I missed irsnd_on() and irsnd_off(). THANKS!

BTW. The digitalWriteFast() expands to digitalWrite() which has IRAM atribute set. extern void IRAM_ATTR __digitalWrite(uint8_t pin, uint8_t val)

eku commented 4 years ago

I have some time again to work on this topic. Meanwhile IR receiver and transmitter are connected on the breadboard, the pins are configured in Tasmota and calling the ISR does not cause a crash anymore. However, nothing is received either. Since no log outputs can be made in the ISR, I don't know how to analyze further?

ArminJo commented 4 years ago

Does the feedback LED react, when you send IR signals?

ArminJo commented 4 years ago

How is it going?

eku commented 4 years ago

I don't have time to work on this right now. If it's not an inconvenience, I'd ask you to keep the issue open. This way I can address any needs that arise when the implementation does not work reliably with the dynamic pins.

ArminJo commented 4 years ago

Hi, I fixed a bug in feedback LED handling for dynamic pins for send and receive. I swear that the ReceiveAndSendDynamicPins example is running perfectly on my ESP8266 board. I testet it e.g. by swapping pin 14 and 12 and it worked flawlessly!!! This is the mapping from pins_arduino.h I used:

#define LED_BUILTIN 2
static const uint8_t D0   = 16;
static const uint8_t D1   = 5;
static const uint8_t D2   = 4;
static const uint8_t D3   = 0;
static const uint8_t D4   = 2;
static const uint8_t D5   = 14;
static const uint8_t D6   = 12;
static const uint8_t D7   = 13;
static const uint8_t D8   = 15;
static const uint8_t RX   = 3;
static const uint8_t TX   = 1;

and do not forget to use a samsung remote, or change the example code for testing.

eku commented 4 years ago

So, on the ESP32, IRMP now works. On the ESP8266 the IRAM overflows. So IRMP needs more IRAM than IRremote8266 with the same Tasmota configuration. Do you see a way to reduce the IRAM consumption of IRMP?

ArminJo commented 3 years ago

So, on the ESP32, IRMP now works.

Wonderful 👍

The RAM consumpion depends on the number of enabled protocols. I now it from test compile configuration that the AllProtocol examples e.g. does not fit. sketches-exclude: AllProtocols # error ".text1' will not fit in region iram1_0_seg" This is the drawback of doing it in an ISR instead of storing data and postprocessing them at the users main loop or request.

eku commented 3 years ago

So not all protocols can be used, which relativizes the advantage of the supported protocols compared to IRremoteESP8266. A pity. I don't expect Frank to change anything about the basic functionality.

A little bit of IRAM could be saved, if you could get rid of all code for the feedback LED by using a preprocessor-definine. Would that be too much to ask?

ArminJo commented 3 years ago

Using a preprocessor define will save less bytes than skipping e.g. the protocol IRMP_SUPPORT_NEC42_PROTOCOL. For a Blink program, the text1 size for IRAM is 26560 bytes. So only 5,5k are left for us. But If you comment out the last 6 enabled protocols (including IRMP_SUPPORT_MITSU_HEAVY_PROTOCOL) then it will fit into text1 section using 32496 bytes.

eku commented 3 years ago

then it will fit into text1 section using 32496 bytes

I suppose this only applies to IRMP and not to simultaneous use of IRSND, right?

ArminJo commented 3 years ago

I suppose this only applies to IRMP and not to simultaneous use of IRSND, right?

Right!