Make the code less Arduino dependent

[vortigont]

@rwmingis
I've run through the code and have this key points to consider for discussion

• replace millis() call with esp_timer_get_time() microseconds, it's pretty straight forward, millis() is just a wrapper for the above
• replace digitalRead() with IDF's MUX register acces, should be no problem also
• same for the pinMode()
• digitalPinToInterrupt() and functional callback might be more complicated, depends on further lib's architecture - an open question to consider

Let me know what do you think,
Cheers!

[rwmingis]

@vortigont, I completely messed up your pull requests trying to do it the github way. Apologies for that. However, I have integrated your comments and code direclty into the main branch (again accidently insead of a new branch) and got it all working.

Few comments below:

1. I left the bit about clearing flags in when re-enabling the on-change interrupt. This clears any inputs that occur when we are polling the button using the timer, otherwise, when re-enabling the interrupt, it goes straight back into the ISR even if no button was recently pressed.
2. Rather than passing the pin or something to some of these new functions, I am continuing to pass the pointer to the button instance itself, mainly for consistency. Unless you have a specific reason which it should just be the pin only? Ie a big overhead difference.
3. the getGPIO getter, I have just used the pin, as it's a private member, is there a reason I shouldn't?
4. I size the event action array in the 'begin' method rather than the constructor to allow the user to set the number of menus first. If the button is a global variable, I don't beleive they get a chance otherwise. As this is a global value accross all instances, didn't want to pass the number of menus to an individual constructor. Happy to take suggestions for something cleaner.
5. Side change - keyUp now happens twice with double click, I think this is more appropriate.
6. I don't believe i have changed any interface with the outside world so should work directly with previous implementation.

Again, sorry for the stuff around with commits and merges, hope this doesn't make tracking too hard on you.

Cheers,

Rob

[vortigont]

Hello @rwmingis
ah.. never mind... working with git could be messy :) I usually do most of the things via CLI interface localy, than push the changes to GitHub. Especially when creating/merging branches.
So, to your questions

1. yeah, I saw that peace of code. Not sure if it still relevant for now, 'cause now disabling/enabling interrupt via IDF's method works a bit different. Let me dig the IDF's code and check if still makes any use to force clear register's bits.

2. Actually I was thinking IDF's way here. Those methods that calls IDF's C functions solely accepts only IDF's known objects, i.e. gpio_num_t. Thinking forward to thread safety, it might be even better to keep those methods not as static class members but private classless functions (or namespace members). Passing pointers to dynamically created class instances always has a tiny chance to hit the dangling pointer :))) Although it's not that critical in current implementation.

Will comment the rest a bit later.
Cheers!

[vortigont]

Hi @rwmingis, was a bit busy last days.
so, getting back to your keypoints
3) it does not work for private member if the butoon's objects has been passed via void*. Might be fixed via declaring button class as a friend, I suppose, I've just put a simple getter there. Feel free to refactor it.
4) yeah, seems reasonable, but I've immediately hit the crash there with an older code trying to assign a callback method before running begin(). I think the best way to accomlish both would be to create a dedicated method for assigning callbacks, but not just simply exposing array as a public method. There is also a hidden bug there - if you assign some callback for one class instance, then change the array size, a previous object results in an incorrectly sized array. So, hiding array behind the scene would be a more error-prone approach. This is how I see it.
5) great, agree on that!

that's all for now. No more news so far :)
Cheers!

[rwmingis]

Hi @vortigont,

1. Yea, so i think you are right. I just disabled the flag reset code to see if the routine was re-entered spuriously when releasing a button, and it does not seem to be. good catch!!

2. I have changed the type of the the pinNumber member to suit the IDF function argument type. But because of point 1 above, no longer need the helper function, so I just directly call the IDF interrupt enable and disable functions which is a nice simplification. Have put your note about thread safety in my to-do list. Not 100% sure i understand the benefits of moving to classless private functions, as when deleting instances the static class methods remain? I have seen that there conventions that suggest this should be done, but these nuances are maybe on the advanced side of my skill level; happy to be schooled.

3. But accessing private class instance members from static class members does seem to work in this implementation, or am i missing something? I assume because the void pointers are reinterpret-cast back to the InterruptButton class instance it picks them back up. The existing library has lots of private instance members accessed by the static methods.

4. Haha, yea, sorry about that, I encapsulated that 2D event-action cross reference array with accessor functions, but completely forgot to move it to the private area of the class! "I was just testing you" :) Your suggestion is exactly what I did, there are new binding (and unbinding) functions but i failed to document them in the revision. You just pass an enumerated event type to the binding function, along with the menuLevel (or overload to assume current menulevel), and a pointer to the external function, and it seems to protect the array well. As it stands, you can't change the size of the array once the first button is defined, so all buttons will have equal size arrays. I know there are ways to reallocate space, just not sure if it it's necessary complication.

5. I am still waivering on what to do with keyUp's associated with doubleClicks. Ie do a keyUp immediately (current logic) and then determine if a keyPress or doubleClick, or do the keyUp once decided. The serial printout has keyUp/keyDown sequencing either way because there is a second level of 'async''ness with the doubleclick timeout timer. NOt a big deal, I think what I have is OK.

All for now, really appreciate all the feed back and pointers excuse the pun.

Cheers!

[vortigont]

Hi @rwmingis
when you pass the pointer to the class instance to the static method of the same class it doesn't mean it can access private non-static members of the pointed object, it can access *this members only. Yeah, too much of statics here I think, a bit confusing :)
BTW, great work with binding methods! It slipped out of my sight since you've left the array public :) Now it looks better!
For the 5) I think it's totally OK to have keyUp triggered every time despite of doubleClicks. It might be handy to have both types of events like up/down and press/doublepress.
I may come up with queue events implementation draft next week, depends on spare time. Will keep in touch!

[rwmingis]

Hi @vortigont,

Yea, I understand what you are saying but racking my brain a bit, because I thought static class member functions can access the same class's non-static private instance members, and it does work in this code, ie, m_state is a private instance variable and the static readButton function happy accesses this variable during normal operation. I get that maybe private classless functions or namespace members wouldn't see it though. Am keen on whatever it takes to improve stabilty, and reduce overheads/memory however. Will consider after thoughts on queue events.

Regarding your queue events suggestion. I did have a gander at them last week, but at bit of a learning curve. Happy to take any suggested code bits in the mean time. Think I am ready to do a version 1.0.0 release for what we have come up with so far. Maybe I will do a branch for your queue events suggestion which may become an eventual 2.0.0.

Cheers!

[vortigont]

Great idea with 1.x and 2.x branch!
I think it would be great to have current implementation available anyway. Pretty good working for me :)
And we can test and try out some new approaches in another branch. Might not work as good at first.