[FR] Make PIN configuration at runtime
eku opened this issue · 45 comments
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?
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.
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.
Can the function irmpInit(PinNumber) be called more than once if the pin configuration changes at runtime?
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.
On this occasion I noticed that in library.json the old version is still there instead of 3.0.0.
Thanks, but 2.2.1 is the latest release available for arduino library manager, 3.0.0 is still under construction.
Please correct the function names in README.md. No camel case but underscore.
Ok, but which function name do you mean?
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.
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.
I do not understand totally.
Do you mean to add:
#ifndef LED_BUILTIN
#define LED_BUILTIN 0
#endif
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
Ok, lets see what I can make.
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.
OK, Done,
Like to test it?
Like to test it?
Yes, will test it....
@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);
^
Line 49 is guarded with
#if defined(IRMP_IRSND_ALLOW_DYNAMIC_PINS)
pinMode(irmp_irsnd_LedFeedbackPin, OUTPUT);
...
#else
LINE 49 pinModeFast(LED_BUILTIN, OUTPUT);
@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.
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.
Standalone example ReceiveAndSend works for me. Do you have Wifi or BT running?
Do you have the complete code to test?
- #include <irmpSelectAllProtocols.h> (which defines additional values) must before #include <irmp.c.h>.
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.
- Try to comment out the timer init code in IRTimer, to see if this is the reason.
- Next start the timer, but disable the interrupt.
- Next enable interrupt and strip the ISR down to empty or only handling the feedback LED, to see if the length of the ISR is the reason.
- Can you generate a lss file with xtensa-esp32-elf-objdump -h -S IRMP_ESP32.elf 1>IRMP_ESP32.lss and check the dumped crash adresses against the addresses in the lss, to find the reason for the crash?
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.
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.
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?
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.
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?
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.
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() {}.
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.
Hi, tanks. Did you miss irmp_DoLEDFeedback?
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)
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?
Does the feedback LED react, when you send IR signals?
How is it going?
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.
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.
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?
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.
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?
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.
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?
I suppose this only applies to IRMP and not to simultaneous use of IRSND, right?
Right!