Button actions implemented as RTOS queue

[vortigont]

Hi @rwmingis,
so, I've rushed a couple of elephants to your china shop :)
Here is an implementation with rtos queue - check this branch. Not making a PR for now, still lot's of dirty code and comments but already works pretty fine.

pros:

• obsolete sync/async callback, it all async now but executed out of ISR, so no IRAM needed for callbacks and no restrictions for callback code
• no loop() hooks required
• unlimited menu depth levels per each button class instance
• current menu level maintained per-class instance
• dynamically created obj works fine now, so could be used for arbitrary pins at run-time

cons:

• external lib required LinkedList
• DoubleClick/Repeat flags affects all menu levels (not sure about this, just kept it simple for now)
• not sure about eventMask, mostly useless for now, might need some refactoring there

it's just a draft for now. Thinking about some other improvements, i.e. lot's of static methods within the class does not look nice.
Feel free to try this out.

Cheers!

[rwmingis]

OK @vortigont, here we go. I have created an experimental branch, that will probably become v2.0.0 once in a good state. The changes are based on your suggestions and example code you shared under your #10 for RTOS message queues.. See it here:

https://github.com/rwmingis/InterruptButton/tree/RTOS-message-Queue

I've just made a working copy, but still quite messy.

Looking at your dot-points above:

1. Yes, agree, the synchronous stuff is messy; I took it out, but then thought some users may still want synchronous stuff so things don't change behind the scenes while their main code is running. So i reintroduced it using the same bindings for Async functions and as a Static member queue rather than the RTOS queue. The user can then set mode using the 'setMode' function (Asynchronous or Synchronous), then the execution of the events is set either to the RTOS queue, or the Static queue serviced when called in the main loop. The detection of events, and the event bindings themselves are the same either way.
2. Yep, get that, but I imagine the user will still be running other code in there. Works both ways
3. I still have unlimited menu depth, but you only get to set it once. Minor loss of functionality without the need to have an external library and extra memory req'ts. Maybe this can be extended one day, but trying to keep simple where I can.
4. I have left the menulevel global across all buttons at this point still. No biggie to change to per instance, but need to think about what is best for the users. Potentially, we could do it as each button defaults to following the common menu level, unless we set it to be changed on it's own, then it can stray individually from the the rest of hte buttons.. THis may be the case where say you have 4 buttons part of the UI which all change on a per screen basis, but then on/off or up/down buttons are common across all. This may be the way to go.
5. Have not tested dynamically created objects on this verion, but agree, that should be a feature. NIce to get rid of all the IRAM_ATTR's.
6. External lib no longer required as noted under point 3.
7. I think i have fixed the doubleclick/repeat flags. I have just used my original mask. Basically, the main change was under the 'action' function byt adding to the queue (Async by default, or sync if the user chooses) rather than firing off the action immediately from the ISR, but using the same logic to get to that point.
8. As per point 7.

[vortigont]

Hi @rwmingis,
I've checked your branch, you've done quite a rework for my PR as I see it. Pls, mind some gotchas below.
The whole idea of enqueuing function pointers of a class instances is bad by design. Messages are enqueued via copy and before it could be dequeued an object it points could be destroyed. You'll get a dangling pointer and code will crush. For sure it will work fine for predefined IButton objects and still might work for dynamic also if you are not creating/destructing it every now and then. But... this is bad design right from the start. In general Q's are used to pass events, or data states. BTW, I've debugged the issue with run-time objects not working properly, your new branch is still affected, while mine is not. The issue is in esp_timer_handle_t being undefined members. For run-time created instances it might end up pointing to some dirty heap region instead of a proper struct and that leads to a crash. Pointers could become a hidden trap anytime, especially in multi-threaded envs.
And one more point - with function pointers approach you do not even need a RTOS queue struct. This could be done much simpler with an array like func_ptr_t** eventActions and a queueServicerHandle task notification.

for 2) I have my doubts for synch stuff. There are plenty of button libs around having such alike loop() hooks being called every cycle. Yours could be better designed for RTOS, so does it worth the efforts dragging around the complexity of this legacy for just in case?

InterruptButton/InterruptButton.cpp

Line 386 in 9681dc0

if(pxHigherPriorityTaskWoken == nullptr){

This code is wrong. You can't evaluate pxHigherPriorityTaskWoken before passing to xQueueSend... and it shouldn't be casted to nullptr either, it's a typedef'ed int.
You might wanna take a second look to the action() method. If you call it from an ISR, it needs to be placed in IRAM, or reduced to some simpler inlines.

[rwmingis]

Yes. Started to go down the route of directly copying your code, but wanted to maintain the same code structure for the most part and focus on modifying the 'action' function to cater for RTOS calls OR the Static Member Queue. Particuarlly wanted to simplify it to removed dependency for the linked list. As you said a fair bit of additional complexity for just the benefit of dynamically changing the maximum number of menus.

Main goal was to get it running based on the old code then improve together.

Edit: (@vortigont ) After a bit of thinking, I think i finally get your concerns around dangling pointers (through my thick head) and that it relates to an object the timer is supposed to act upon via pointer being destroyed before the timer returns. Better to just pass the external function that the instance pointed to rather than the instance itself, i get it. So yea, that does complicate things a bit with my logic like monitoring if a longpress occurred, or if we are waiting for a doubleclick. I could keep a list tracking each instance, and check it before doing anything to an instance, and that may work, but yea, get that it may be bad programming. I suppose I could store that data relating to doubleclicks and longpress occurances in a static class array (parallel to the instances) but that is also messy. Hmmmmmmm.

Actually, that note about line 386 is intentional. In the declaration for the action function, i have a default value of nullptr for 'pxHigherPriorityTaskWoken' If it is null when evaluated, then it means i called it from a timer and i use xQueueSendToBack, but if it's not null it means it was called from the ISR and i use xQueueSendToBackFromISR. Could be improved, but that's the intention there. What should the default value be, i can see now that I have that type wrong.

Good eye on the missing IRAM_ATTR on the action function. I added the attribute last night after my last commit (didn't think worthy of a another commit) and I also removed IRAM_ATTR from the timer call backs as those I believe are not ISR related. I have my doubts that all the ISR code is still making it to RAM with all the internal library functions called from the ISR. Not sure how to check but was going to try doing some spiffs calls whilst pushing buttons to see what happened. I'm sure you know a better way. Also, i understand some variables like strings and constants need a DRAM attribute, haven't even started on that yet.

Cheers!

[vortigont]

Hi @rwmingis
ok, it might be messy to think through ) It's not even with timers. Let me make an example.

1. You have 3 class instances ib1, ib2, ib3.
2. once created and bound with some callbacks each instance has dynamically allocated eventActions array with some pointers to functions.
3. suppose you pressed all 3 buttons at once (just for the example), action() call places 3 copies of pointers to each instance's eventActions[][], let's call it pointer to cb1, cb2, cb3
4. on a next scheduler tick consumer_task dequeues cb1 and executes it. Suppose it's some long and heavy function and it takes some time to execute it. And the same goes for ib2 and you still have cb3 pointer being kept int the queue.
   During processing cb1 or cb2 (or due to some other scheduler call) you decide that you do not need ib3 any more and destroy it. It's destructor releases eventActions member with a function pointers. More to this, some other object that was used as a callback might also have been destroyed already.
5. now scheduler dequeues cb3 that is still pointing to some region that expected to be the function pointer (or the object member) but it is no longer valid - boom!
   So, postponing execution via pointers to dynamically created/deleted objects is a dead end. The alternative is to register the events - "key_down occurred on gpio 5", whoever dequeues it later should decide if it could be processed properly or not.

I suppose I could store that data relating to doubleclicks and longpress occurances in a static class array (parallel to the instances) but that is also messy. Hmmmmmmm.

You might want to reconsider using static class members in general. Static class members are not class members at all as it may seem. Consider it as globals, those are not associated with any class instance and can't access non-static members, static methods are not polymorphic and it make inheritance and abstraction a real pain. In general static member functions breaks OOP concepts. It might be useful to bind with C system API or libs (like ISR handlers), but implementing your public classes methods as statics usually complicates things more in perspective.

Actually, that note about line 386 is intentional. In the declaration for the action function, i have a default value of nullptr for 'pxHigherPriorityTaskWoken' If it is null when evaluated, then it means i called it from a timer and i use xQueueSendToBack

Sorry, I did not get the idea anyway. Suppose you call it from here

InterruptButton/InterruptButton.cpp

Lines 135 to 136 in 375dd62

	case Pressing: // VALID KEYDOWN, assumed pressed if it had valid polls more than half the time
	btn->action(Event_KeyDown, &xCntxSwitch); // Fire the asynchronous keyDown event

pxHigherPriorityTaskWoken will resolve to false and xQueueSendToBack() will be called, which is obviously wrong.
Anyway context switch might or might NOT happen depending on scheduler, it is not mandatory and you can't rely on it to decide if you are in the ISR or not.

[rwmingis]

Hi @vortigont, thanks for the explainer above, I had just grasped your point, but now it is very clear. Thanks.

I had just started to tidy things to repair a few things, address this issue of potential dangling pointers and it became apparent why you structured your version as you did. Makes sense

Interestingly enough, when I delete an a button object instance, in the deconstructor i kill all it's associated timers, so in theory it shouldn't leave timers calling dangling pointers. Same with the RTOS queue where there is no reference to the calling button. Once i fixed the uninitalised timers, I could not get my code to crash when deleting a button, can you share your test code that you used to demonstrate this issue please?

And yes, i found your point about the undefined timers the hard way, forgot to initalise all my timers to nullptr. In my killtimer routine, once I kill a timer i set it's handle to nullptr. If i try to kill it again, there is a crash hence the nullptr check first. Unfortunately in my instance of the ESPIDF via arduino in platformIO, the function used to test if a timer is active is not available, as are a few other random functions and constants. Also I'm surprised the ESP idf doesn't set the timer handle equal to nullptr and do the check itself when deleting it.

I think i get your point about not needing a formal RTOS queue struct, just a regular array of pointers and the existing RTOS queueServicer routine. Just need to have that servicer task poll the array (which we already have running), and if it's not empty, start chugging away at it. It doesn't seem like a bad idea from the getgo. This will simplify my action routine as well that intialises it.

I have a little work ahead of me, but will get to it ASAP. Just about to go on a short holiday, if i don't do it today, will do when i get back. :)

Cheers!

[vortigont]

looks like with a current code you do not need xQueueSendToBackFromISR() and pxHigherPriorityTaskWoken at all, because you never queue on ISR trigger, but only from a timer. Might worth refactoring readButton() into a short ISR function and another for timered calls.

[rwmingis]

This point may not be necessary if changing to the basic array of pointers for the queue. I was still using the xQueueSendToBackFromISR() version because the helper function "action()" is called from the ISR, so I assume it still needs it.

[rwmingis]

Swapped out the RTOS queue as you suggested as an option, in lieu of a simplefied array of pointers to the external user functions.

Seems to work well.

Also managed to reproduce your issue with deleting dynamic buttons. In my case, it doesn't crash (may be different with the new queue in the form of an array of pointers), but now it goes into an infinite loop of calling the 'action()' function. Very strange. So that is still to solve, like you said probably dangling pointers.

👍

[vortigont]

Hello @rwmingis!
Sorry, was kind of busy lately. Nice work in progress! :) So, there could be different options to handle button actions. RTOS has it's features, although it forces you to start thinking non-linearly 👍
I've tried some other changes and approaches also, maybe better in some ways, maybe not. "RTOS way" for the queue is "multiple senders - single consumer". So consuming callback out of Button object scope could be quite useful for example in derivated classes. I.e. I have a case where I need to control two buttons as a new object - simultaneous press, consecutive press, etc... (do not say a word about encoder :))) ) So, need another class based on two pins and be able to receive events from different pins in one place.
Also, another case - have some multilevel design for 2-4 buttons, but creating multiple similar callbacks for every level and pin is quite cumbersome. I came up with an idea to be able to receive a detailed events, like "gpio, event, some_other_info". And it might be easier to process it in a "switch/case" user callback.
This is still work in progress (quite short of free time lately), but I've already implemented class logic for

• press repeat event can report a count of repeated events occurred
• double click event replaced with multi-click, multi-click returns number of clicks occurred
  todo is implement a callback that can consume a struct as a parameter.
  I.e. assign a callback for a specific gpio, a group mask of gpios, or some specific event types only. Might seem quite complicate, so not sure how to be with a simple per-class callbacks. Either to keep some compatibility layer, or decouple it completely. Waiting for an inspiration here and free time to work on this.
  You can find my work-in-progress in the same branch.
  Not updated example yet, but changes are mostly internal. Pls, pick anything you can find useful. Any ideas or feedback is always welcome )