edgetx
edgetx copied to clipboard
EdgeTX
Adds audio/haptic feedback for all hard keys except trim switches
This addresses https://github.com/EdgeTX/edgetx/issues/2194
Changed logic from application decides if feedback has to be given (was inconsistent and inefficient) to key driver detects button down and triggers immediate feedback if radio settings allowed for acoustic and/or haptic feedback (radio settings, radio setup, sound set to All)
With this change all instances of the application calling for feedback were removed in favor of the centralized approach.
This first commit also introduces a dummy function for providing feedback on key presses in boot.cpp because keys.cpp is also used as the bootloader key driver but with no logic to determine if feedback is allowed. Using a define that indicates keys.cpp is compiled for use in the bootloader would make a much nicer and backwards compatible solution.
@pfeerick is there a define I could use to identfiy a bootloader compile?
Yes... If memory serves me correct it is simply
BOOTLOADER...
That would be great. I'll try later.
Ok, close, but not quite... it's
BOOT
@pfeerick Hi Peter, BOOT worked. The change in keys.cpp is now excluded when compiled for the bootloader.
Another thing. All instances of onKeyPress() in libopoenui and the function definition of onKeyPress() (now a dummy function) in audio.cpp could be removed. I did this locally and it worked but can't push the changes. I reckon this is due to libopenui being marked third party.
Not quite - it's a submodule, so it needs to be done against the libopenui repo. If that is the extent of the change I can follow that up if this is the way this goes... while I agree with Malte it would have been nice to use a LVGL event, I have this feeling from the digging around that I have done that the way we've shore-horned LVGL support in there probably isn't one... and this makes more sense as the way to catch what we're after... keypress down.
This also gets rid of all those silly other onKeyPress() triggers which seem to be more there to confuse as to when it will and won't beep, and getting rid of the onKeyPress() in libopenui gets rid of some more 'hardware' stuff that really shouldn't be there. I like! :) Next will be to see how long I last with 'All' enabled! 😆
Ok, understood. So I'd have to do a separate PR for the libopenui change to get rid of onKeyPress entirely?
Yes, exactly. And then before merging this, I would merge that PR first. Then this PR can be updated to suit. I normally make that sort of change just before merging, so the PR doesn't get stuck in a broken state.
@pfeerick I removed onKeyPress() now entirely and created a PR for the respective libopenui change. Now this should blow up big time
Actually, you might get away with it this time... since the commit actually exists on the libopenui repo rather than just in your working copy, it looks like it's going to take...
@pfeerick I removed onKeyPress() now entirely and created a PR for the respective libopenui change. Now this should blow up big time
To not stress you too much with "All" the removal of onKeyPress() in libopenui numberedit.cpp and table.cpp also removed the feedback for rotary events. I found the constant beeping while turning the rotary wheel annoying. If this is too much of a change this could be reverted by calling audioKeyPress() instead of onKeyPress() as application feedback (like the trim beeps).
Hm... okay... what did you do (or do I just have the magic touch?)... I've built this PR locally... (with the submodule update for libopenui !!!) and it currently crashes on hard keypress (touch is working fine) when set to 'all'... can you repo this or is there a whopper of a PBKAC over here? 😆
Yes, I think we can dispense with the rotary encoder beeps... I think most people will agree they are more annoying than useful. If someone complains we can add a "Even the Kitchen Sink" option ;)
Not quite sure what you mean... I tried this PR, which includes the libopenui change. Hard keys are triggering EM, and touch is working fine. But only when Sound is set to all... and oh... only for Sound: All ... Haptic: All is fine... 🙃
Hi Peter,
sorry, I missed that you merged https://github.com/EdgeTX/edgetx/pull/2193 (the touch one) 10 days ago. I am glad that at least the touch one works as expected.
Setting Sound and Haptic on my TX16s with #2193 works as expected: Sound: NoKey, Haptic: NoKey -> no beep, no buzz Sound: All, Haptic: NoKey -> beep, no buzz Sound: All, Haptic: All -> beep, buzz Sound: NoKey, Haptic: All -> no beep, buzz
As far as this PR is concerned: shame on me. It crashes the radio with Sound: All on hard keys. It works with haptic feedback though. Sound: NoKey and Haptic: All works for keys too.
Well, I demonstrated - in the name of science - that calling audioKeyPress() from keys.cpp nukes the radio (sim works). I couldn't figure out why exactly but audioKeyPress() calls audioQueue.playTone() in case Sound: All. audioQueue.playTone() locks the audioMutex for a while. key::input() is called every 10ms, so probably not the best idea to load it with audioKeyPress().
Back to the drawing board. Sorry for stealing your time.
Oh raspberry... yeah tying up
key::input()and co is probably it... 😭
I rolled back keys.cpp back implemented some logic in LvglWrapper.cpp/keyboardDriverRead() to determine a first key press on the 7 hard keys (trims are handled by the application). Tried it on my TX16s. No crash, works as expected and allowed to cleanup a minor TODO (removing getWindowEvent() - this is the reason for a new commit on libOpenUI).
I'd appreciate you giving this and the libOpenUI commit a try.
lol... I see you were thinking the same as me... I meant to add " hm... maybe try looking for a LVGL again / pushing an event" before... looks promising ;) and cleanup of (now?) basically pointless functions is always good :)
Yes, I think we can dispense with the rotary encoder beeps... I think most people will agree they are more annoying than useful. If someone complains we can add a "Even the Kitchen Sink" option ;)
Why not :)
