fix(devkit2): Add button deboucing and long press haptic feedback#4245
fix(devkit2): Add button deboucing and long press haptic feedback#4245kevtae wants to merge 1 commit intoBasedHardware:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a debouncing mechanism for the button to prevent spurious presses and adds haptic feedback for long presses. The changes look good and directly address the issue described. I've provided a couple of suggestions to further improve the robustness of the new debouncing logic: ensuring correct access to the was_pressed variable shared between an ISR and a thread, and preventing a potential integer overflow in the time calculation. Addressing these points will make the implementation more robust.
|
|
||
| u_int8_t btn_state = was_pressed ? BUTTON_PRESSED : BUTTON_RELEASED; | ||
| // Debounce logic - filter out electrical noise/contact bounce | ||
| bool raw_state = was_pressed; |
There was a problem hiding this comment.
The global variable was_pressed is written in an interrupt service routine (button_pressed_callback) and read here in a workqueue thread context. To prevent incorrect compiler optimizations and ensure the latest value is always read, it should be declared as volatile.
For example, in its declaration:
static volatile bool was_pressed = false;For even greater safety in a concurrent environment like Zephyr, consider using atomic variables (atomic_t) for was_pressed to guarantee atomic access and proper memory ordering between the ISR and the thread.
| uint32_t stable_time = (current_time - last_state_change_time) * BUTTON_CHECK_INTERVAL; | ||
| if (stable_time >= DEBOUNCE_TIME_MS) { |
There was a problem hiding this comment.
The calculation of stable_time involves a multiplication that can overflow if the button state remains unchanged for a long period (e.g., over ~49 days), as current_time - last_state_change_time could become very large. This overflow would cause stable_time to wrap around to a small value, potentially leading to incorrect debounce behavior.
To make this more robust, it's safer to perform the comparison in 'ticks' rather than milliseconds. This avoids the large multiplication and removes the intermediate stable_time variable.
if ((current_time - last_state_change_time) >= ((DEBOUNCE_TIME_MS + BUTTON_CHECK_INTERVAL - 1) / BUTTON_CHECK_INTERVAL)) {
|
|
||
| // Only update debounced state after stable for DEBOUNCE_TIME_MS | ||
| uint32_t stable_time = (current_time - last_state_change_time) * BUTTON_CHECK_INTERVAL; | ||
| if (stable_time >= DEBOUNCE_TIME_MS) { |
There was a problem hiding this comment.
I think this condition never occurs. Basically, the button is checked with a 40 ms period, and your debounce time is shorter than one cycle.
There was a problem hiding this comment.
ah yes debounce logic is already in place. I dug deeper and I'm seeing state 3 BLE notification get dropped on long press 30% of the time, while state 5 is delivered consistently.
My hypothesis is that when the audio streaming is filling the BLE buffers, the button notification is getting deprioritize or dropped. And by the time state 5 is handled, the streaming backlog has cleared.
For now, I added a temporary fallback in my application to handle the missing notification, and we can close this PR since it doesn't appear to be a debounce issue.
Long term tho, we can consider:
- add a retries for button/state notification similar to ones we have for audio packets
- introduce a queue so button events are prioritized over audio packets
- haven't dive too deep into this but i do see that there is a callback function version of bt_gatt_notify()
There was a problem hiding this comment.
Yeah, I also noticed that problem. Sometimes, it can't notify when audio is streaming. I opened an issue for it before with the battery percentage. It's nice to see your comming PR for it 🥳
| LOG_PRINTK("long press detected\n"); | ||
| btn_last_event = event; | ||
| notify_long_tap(); | ||
| play_haptic_milli(100); // Haptic feedback for long press |
There was a problem hiding this comment.
This behavior is not our intention. We only play the haptic on power-up and power-down. If you prefer this behavior, you can keep it locally or implement a solution that allows customization from the phone app based on user preferences.
|
Hey @kevtae 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
3 participants
Fixed a bug where a long press on the button could accidentally power off the device instead of only sending a BLE notification.
Problem: Physical buttons can “bounce,” meaning the signal rapidly flickers on/off before settling. The firmware was interpreting that noise as a quick tap, which triggered the power-off behavior.
Solution: added a 20ms debounce filter to wait for the signal to stabilize before counting a press and also added haptic feedback (a vibration) when a long press is recognized so you get clear confirmation it worked