Bugfix keypad

[KlausMu]

Bugfix 1: key 'r' was doubled ("record" and "right") Bugfix 2: due to "Keypad\Examples\MultiKey", keypad handling is wrong. Without the bugfix you get too many events. With the bugix you get at least only two events (first "PRESSED", and maybe "HOLD", if you press long).

[KlausMu]

If you want me to, I could also provide a pull request which distinguishes between short and long press and creates only a single event on this, not two, as currently.

[CoretechR]

Thank you for this bugfix! I just tried it out and I have one issue: When I hold down a button, the IR command is only sent twice. What I would like is a continuous IR signal, which is useful e.g. for quickly setting the volume. Until now, the solution was to just run the for loop regardless of the getKey return value. Do you know if there is a better way to do this?

[KlausMu]

OK, then you indeed have to ignore the return value. But take care, you have to use getKeys (with an s at the end), not getKey. Otherwise the loop does not make sense.

As you probably know, you can get the key states IDLE, PRESSED, HOLD and RELEASED. If you don't want to differentiate between PRESSED and HOLD, then of course you can do it as you did.

But I wanted to differentiate, and for this I needed a more complex solution. The solution depends on what you want to achieve if you hold the key. Let's say you have "ACTION_PRESSED" and "ACTION_HOLD"

What you could want to achieve when a key is hold is:

1. ACTION_PRESSED, ACTION_PRESSED, ACTION_PRESSED, ACTION_PRESSED, ...
2. only ACTION_HOLD, nothing else
3. ACTION_PRESSED, ACTION_HOLD
4. ACTION_PRESSED, ACTION_HOLD, ACTION_HOLD, ACTION_HOLD, ...
5. ACTION_HOLD, ACTION_HOLD, ACTION_HOLD, ...

Obviously, it is not possible to have 1. and 2. at the same time. At least not for the same key.

Depending on the result you want to achieve, you have to ignore the result of getKeys and/or to use the events cleverly, e.g. to do your ACTION_PRESSED in the RELEASED event.

I'm afraid the needs are so different, that it is hard to provide a universally valid code example. But let me know if I can provide some more code, e.g. for variant 2.

Conclusion: yes, if you want to achieve variant 1, my "bugfix" is only partly a bugfix

• getKeys (with a "s" at the end) is indeed a bugfix
• but not considering the result of getKeys

[KlausMu]

I think, the best really would be that you can define for every key the behaviour you want to have:

1. repeat mode: if key is hold, the action ACTION_PRESSED is repeatedly sent (e.g. for IR devices like amplifiers or TVs)

2. singleXORhold mode: if key is short pressed, ACTION_PRESSED is sent, if key is hold, only "ACTION_HOLD" is sent (and not a ACTION_PRESSED before). Here we have two different actions for short press and hold (e.g. Fire TV wants to have it that way)

and maybe someone also wants to have

3. single mode: only one single action ACTION_PRESSED is sent, even if key is hold (don't know which device wants to have it that way)

4. singleANDhold mode: if key is hold, first ACTION_PRESSED is sent, then ACTION_HOLD (here I also don't know which device wants to have it that way)

What do you think? At least for me I will implement 1. and 2. Currently I only use 2. (because of my Fire TV), but as you said, for setting volume 1. would also be helpful.

[CoretechR]

To me, ACTION_PRESSED, then ACTION_HOLD make the most sense, as that's what I expect from a keyboard. This way you can have the precision of a single press and also quickly scroll or change the volume if the key is held. Other button behaviors are probably way less common, but surely they can be added with a few lines of code. Thank you already for you help and suggestions!

[KlausMu]

To me, ACTION_PRESSED, then ACTION_HOLD make the most sense, as that's what I expect from a keyboard.

Really? But this is not what you expect from your IR remote when increasing volume. I think we have a misunderstanding.

Let me try to explain.

The Fire TV remote has a button for "fast forward". If you short press it, you jump forward 10 sec. If you long press it, you get into some kind of scan mode where the movie continues as usual, but you have a small preview window where you can go through the movie very fast, until you end this mode.

When you long press that button, you directly get into this mode, without first jumping 10 sec forward. This is what I mean with ACTION_PRESSED and ACTION_HOLD. You have two completely different kind of actions that happen here. short press -> ACTION_PRESSED: jump forward 10 sec long press -> ACTION_HOLD: get into scan mode In both cases (short press and long press) the corresponding ACTION is triggered only once.

For an IR remote, I think in most cases you have only one action: ACTION_PRESSED -> sends a certain IR command, e.g. for increasing the volume So I think what you want to have when a key is hold is: ACTION_PRESSED, ACTION_PRESSED, ACTION_PRESSED, ... You don't want to send a completely different IR code when you hold the key.

On a more basic level, a keyboard (and the Fire TV remote is more or less a keyboard), sends the following events (this behaves completely different to what the Android Keypad library is sending as events): Short press: KeyDown, KeyUp Long press: KeyDown, KeyDown, KeyDown, KeyDown, ...KeyUp

It is up to the software how this is interpreted. You can interpret it like

• repeat the same action ACTION_PRESSED as long as the key is hold (and as you get repeated "KeyDown" events)
• differentiate between a ACTION_HOLD and ACTION_PRESSED, depending on how long the key is hold

So spoken in these terms, the keyboard events are normally transformed into a single execution of ACTION_PRESSED (short press) or a series of ACTION_PRESSED (long press). So the same command is either sent only once or repeated. Here you dont have a completely different ACTION_HOLD. But the Fire TV does so ...

So I think the "1. repeat mode" is how a normal keyboard or an IR remote behaves. "2. singleXORhold mode" is how the Fire TV behaves.

How does this all fit together? The Keypad library sends the following events: IDLE, PRESSED, HOLD and RELEASED, and all of them only once (HOLD maybe never).

We have to think how to transform this into keyboard events. If a button is hold, should we sent

1. KeyDown, KeyUp, KeyDown, KeyUp, KeyDown, KeyUp, KeyDown, KeyUp,... (which is ACTION_PRESSED, ACTION_PRESSED, ACTION_PRESSED, ...)
2. or KeyDown, KeyDown, KeyDown, KeyDown, KeyUp (which is ACTION_HOLD)

The second is needed at least for the Fire TV. That's the reason why I think it is better to have a separate ACTION_HOLD

Addendum (sorry for the long post): You could ask why I do not do this Keypad event PRESSED -> send keyboard event KeyDown Keypad event RELEASED -> send keyboard event KeyUp

Well, to have a more high level view on the devices, I introduced a commandHandler, where each device can register commands. Whereever you are in the application (Keypad, Touch-Event), you can simply send that command. You don't have to bother about is this an IR command or a keystroke or sending an MQTT message or whatever. This has a lot of benefits, because the different frontends (keys, display) do not need to know anything about the devices. And this pattern works best with these more high level ACTIONS. So in the code I'm only calling executeCommand(key_commands_long[keyChar]); in case of a long key press or executeCommand(key_commands[keyChar]); in case of a short key press.

Hm, was this understandable?

And, BTW, I was not able to activate bluetooth because of a "region `dram0_0_seg’ overflowed". The resulting "firmware.elf" cannot even be linked. Most of the DRAM is used by "bufA", "bufB" and "LV_MEM_SIZE". And bluetooth is a beast by itself ...

That's why currently I cannot use a bluetooth connection. Instead I'm sending MQTT messages to another ESP32 which is connected to the Fire TV and behaves like a keyboard. But the bluetooth problem is something we could discuss in another thread.

Edit: In the meantime I found the solution for the DRAM issue. I changed bufA and bufA from static memory to dynamic allocated memory. That saved a lot in DRAM. Now there is enough space for bluetooth.

[Dark1886]

@KlausMu you weren't able to use bluetooth on this device? What library are you using to do the bluetooth connection? I was able to add a BLE Gamepad/Keyboard to my library without too much of a problem. Although I did have to slim down a different library I also added.

[KlausMu]

Correct, I use "t-vk/ESP32 BLE Keyboard". I have some more static content compared to your original code, and that together with bluetooth does not fit into DRAM. I got region dram0_0_seg overflowed by 7008 bytes In total there are 124.580 bytes available in DRAM. When I change bufA and bufB from static to dynamic allocated memory, I save 2 x 15.360 = 30.720 bytes. That's a lot. I guess your original code is already hard at the limit, together with bluetooth you might get problems.

Do you have any concerns using dynamic allocated memory for bufA and bufB? For me it works.

BTW, bluetooth does not seem to work so easily together with WiFi. You cannot directly use both at the same time. My first tries failed. It is possible, but I have to dig deeper into it. https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/coexist.html https://espressif-docs.readthedocs-hosted.com/projects/esp-faq/en/latest/software-framework/coexistence.html https://esp32.com/viewtopic.php?t=6707&start=30 https://esp32.com/viewtopic.php?t=16747

[Dark1886]

Since you're already this deep into the memory management i'm sure you probably thought of this already, but are you using the 'hugeapp' partition table? I believe I needed to do that to get additional features working.

As for dynamic allocating memory, I don't really know enough about that subject to have a part in that conversation.

[KlausMu]

Im using the "huge_app.csv" partition table as you do in "platformio.ini". But in this case this does not help.

Whenever you trigger a build in PlatformIO, you see a log message like HARDWARE: ESP32 240MHz, 320KB RAM, 4MB Flash With the partition table you can define how the 4MB flash should look like, but not the 320KB RAM. Look at the last line in file c:\Users\\\.platformio\packages\framework-arduinoespressif32\tools\partitions\huge_app.csv

# Name,   Type, SubType, Offset,  Size, Flags
nvs,      data, nvs,     0x9000,  0x5000,
otadata,  data, ota,     0xe000,  0x2000,
app0,     app,  ota_0,   0x10000, 0x300000,
spiffs,   data, spiffs,  0x310000,0xE0000,
coredump, data, coredump,0x3F0000,0x10000,

0x3F0000 + 0x10000 = 4MB

The DRAM is part of the RAM and cannot be changed. It is defined in this file: c:\Users\\\.platformio\packages\framework-arduinoespressif32\tools\sdk\esp32\ld\memory.ld There you can find:

  /* Shared data RAM, excluding memory reserved for ROM bss/data/stack.

     Enabling Bluetooth & Trace Memory features in menuconfig will decrease
     the amount of RAM available.

     Note: Length of this section *should* be 0x50000, and this extra DRAM is available
     in heap at runtime. However due to static ROM memory usage at this 176KB mark, the
     additional static memory temporarily cannot be used.
  */
  dram0_0_seg (RW) : org = 0x3FFB0000 + 0xdb5c,
                                     len = 0x2c200 - 0xdb5c

overall RAM: 0x50000 = 327.680 bytes = 320KB DRAM: 0x2c200 - 0xdb5c = 124.580 bytes. This is fixed, you have to live with that.

So solution is: Instead of

static lv_color_t bufA[ screenWidth * screenHeight / 10 ];
static lv_color_t bufB[ screenWidth * screenHeight / 10 ];

which would go into DRAM, use

lv_color_t * bufA = (lv_color_t *) malloc(sizeof(lv_color_t) * screenWidth * screenHeight / 10);
lv_color_t * bufB = (lv_color_t *) malloc(sizeof(lv_color_t) * screenWidth * screenHeight / 10);

which goes into the flash. That's all. With that, you save 30.720 bytes of your precious DRAM.

The partition table helps if you are dynamically allocating a lot of memory and you don't have enough memory for this. The "huge_app" simply waives the second partition for the app, so you have doubled the size for your app. But you cannot use OTA (over the air updates) anymore.

And no, I'm not really deep into memory management. Some of it I now only since yesterday. If you are interested: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/memory-types.html https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/partition-tables.html https://community.platformio.org/t/esp32-ld-region-dram0-0-seg-overflowed-by-156768-bytes/18753/2

[CoretechR]

To me, ACTION_PRESSED, then ACTION_HOLD make the most sense, as that's what I expect from a keyboard. ... Edit: In the meantime I found the solution for the DRAM issue. I changed bufA and bufA from static memory to dynamic allocated memory. That saved a lot in DRAM. Now there is enough space for bluetooth.

Thank you, I think I understand. So far I have mostly used the remote for IR applications, so differentiating between long and short button presses did not seem necessary. But for a Fire TV that makes total sense. And yes, in terms of keyboard events for an IR remote, ACTION_PRESSED, ACTION_PRESSED... would be best.

[phant0mias]

There are several ways to detect different kinds of button pressing. As I can remember (the times I write might be a little different), devices from OSRAM (pushbutton ) have following signals: short press : press and release, signal is send immeadiatly Longpress: press and release, detection of long press after 400/500ms double click: two fast short press in a time about (not sure 200/300ms) The disadvantage is that you might have to wait some milliseconds until the first signal. I don´t think that you need longpress with the remote control, you can signal press and release to handle it.

@KlausMu, @CoretechR : For a continous volume up/down following should work. When pressing the button you send a signal and start a timer. The timer is called every x ms. In the timer you repeat the signal. When releasing the button you stop the timer. Doing it this way you can add some comfort feature, like changing the volume faster after some time.

If you don´t want to use a timer you can use a mechanism like in: https://docs.arduino.cc/built-in-examples/digital/BlinkWithoutDelay/

[CoretechR]

So solution is: Instead of

static lv_color_t bufA[ screenWidth * screenHeight / 10 ];
static lv_color_t bufB[ screenWidth * screenHeight / 10 ];

which would go into DRAM, use

lv_color_t * bufA = (lv_color_t *) malloc(sizeof(lv_color_t) * screenWidth * screenHeight / 10);
lv_color_t * bufB = (lv_color_t *) malloc(sizeof(lv_color_t) * screenWidth * screenHeight / 10);

which goes into the flash. That's all. With that, you save 30.720 bytes of your precious DRAM.

@KlausMu This seems like the solution to our RAM issues. I was afraid that we would have to add PSRAM to continue with expanding the LVGL UI. Just to understand: Malloc() moves the buffers from DRAM to RAM, right? Or how is the flash involved here?

[KlausMu]

Yes, I think currently there is no need for an ESP32 with more PSRAM.

I digged a little bit further into the topic. I think, the main idea I wrote is correct, and also the solution. But to be more correct, we are not moving the static variables from the DRAM into the flash, but from the statically allocated DRAM into another part of the DRAM called heap.

BTW, one more comment to the partition: You can have different kind of problems with memory, since there are different types of memory in an ESP32 as described above. The problem with the flash (application size) was already solved by using the "huga_app.csv" partition. The problem with the DRAM stayed and cannot be solved with the partition. Here you can only find the right balance between static variables and dynamic allocated variables.

With moving bufA and bufB to the heap, we saved 30.720 bytes in the statically allocated DRAM. That solved the DRAM issue.

In theory it is also possible to move another 32.768 bytes from the lvgl static variable called "work_mem_int" to the heap. For doing this, in file "lv_conf.h" the line #undef LV_MEM_POOL_ALLOC has to be replaced with #define LV_MEM_POOL_ALLOC ps_malloc Please see https://github.com/lvgl/lvgl/issues/3244

I tried it, but was not successfull with my first try, so I let it be.

Anyway there would not be enough space for moving all three of them to the heap.

If you move bufA and bufB to the heap, but not work_mem_int, my heap looks like this at runtime: "heapSize: 200076, heapFree: 34840, heapMin: 22412, heapMax: 26612" So it is not possible to also move "work_mem_int" to the heap.

So my recommendation for the best belance is:

• move bufA and bufB to heap as described
• let "work_mem_int" be where it is as a static variable

[CoretechR]

@KlausMu Perfect, du you want to create a pull request for the buffer allocation? I think it will be a great improvement, especially when it comes to future UI updates.

[KlausMu]

@MatthewColvin Thanks for approving. I think we shouldn't merge this PR because it does not cover all needs. Repeatedly sending the same IR code while holding a key would not be possible anymore. See discussion above.

In the meantime I proposed a much more comprehensive solution, not only covering the different needs for the keypad handling. Please see here