Skip to content

Preserve UART bridge frame when transmit buffer acquisition times out - #1642

Closed
leadZERO wants to merge 1 commit into
UltimateHackingKeyboard:masterfrom
leadZERO:codex/uart-transmit-timeout
Closed

leadZERO wants to merge 1 commit into
UltimateHackingKeyboard:masterfrom
leadZERO:codex/uart-transmit-timeout

Conversation

@leadZERO

Copy link
Copy Markdown
Contributor

If UartBridge_SendMessage() cannot acquire txBufferBusy within its 10-second timeout, it logs the error but continues without owning the transmit buffer. It can then overwrite the outstanding frame, change its watermark, and attempt another transmission.

Return the semaphore error before changing the frame or link state. This adds one early return to the production code, plus a regression test for timeout ownership and the existing successful-send and error paths.

Changelog:

  • Preserve the outstanding UART bridge frame when waiting for its transmit buffer times out.

Validation:

  • Host regression under AddressSanitizer and UndefinedBehaviorSanitizer; the timeout case fails against unchanged upstream source and passes with this fix. Run with python3 tests/uart_bridge/test_send_timeout.py.
  • Built both UHK80 release presets and verified the signed images.
  • Physical UHK80 baseline/fix comparison using the real 10-second semaphore timeout: baseline changed 10 owned buffer bytes and sent an acknowledged frame; fixed returned the timeout error, preserved the buffer, and submitted nothing. A/B results.
  • On-device Transport/Link suites and hands-on typing, key/modifier release, module discovery and configured controls, bridge reconnection, sustained typing, and Windows S3 sleep/wake passed. Saved configuration remained unchanged. Manual results and observations.

The timeout test deliberately held the transmit slot over canary storage before DMA. Neither a natural stall nor a timeout with active DMA was reproduced, so this does not establish the cause of a reported typing failure. Unbound module buttons were not tested. The linked notes retain an earlier intermittent module-discovery discrepancy and one USB transfer-error log entry; their causes remain unestablished, and the final hands-on checks passed.

@cla-sign-bot

cla-sign-bot Bot commented Oct 11, 2026

Copy link
Copy Markdown

Thank you for your contribution!
Please sign the CLA.

@leadZERO

Copy link
Copy Markdown
Contributor Author

This is my first PR proposing a firmware change here; I've also submitted the CLA in #1641. I have other changes I'd like to contribute, but before submitting those I'd appreciate feedback on whether this PR's scope, format, and validation are appropriate for the project.

The code and tests in this PR are AI-generated. I do what I can to independently validate that the changes work as intended and do not introduce unintended side effects, including hands-on testing with my own UHK80. I welcome feedback on the implementation, test coverage, and contribution process.

@kareltucek

kareltucek commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Hi!

I'd appreciate feedback on whether this PR's scope, format, and validation are appropriate for the project.

In the first place, it seems to me that it answers the entirelly wrong questions. Or that it even doesn't ask many.

The right questions might be:

  1. What exactly did lead you to this investigation?
  2. Is there any evidence that the above scenario is an actual problem?
  3. Is it fixing the right thing?
  4. Is it fixing anything or making the situation better?
  5. Is the added code complexity adequate to the problem?

My take on these is:

  1. What exactly did lead you to this investigation?

Well, maybe let's focus on the actual problem?

  1. Is there any evidence that the above scenario is an actual problem?

This (a 10 second uart stall) should never happen.

  1. Is it fixing the right thing?

If it does, it indicates a serious problem with the uart control logic (the resending logic and its control thread), and possibly another problem ith uart driver implementation.

That is what needs to be investigated and fixed.

  1. Is it fixing anything?

So, let's assume this scenario actually happens. Uart goes into a 10 second stall. Someone else wants to send another message.

Clearly, that transfer is either already successfuly finished (and we just don't know that), or dead (and we don't know that).

You propose to continue waiting on the stall, without attempting to recover in any way.

Actually, instead of throwing out the old message and attempting a new transfer, you just throw out the new message, and that's it. What is the motivation for that?

  1. Is the added code complexity adequate to the problem?

Adding a complex mocking setup that (probably) (more or less) depends on an exact code form just to test a scenario that should never ever happen?

Definitely no.


Given the above, additional questions suggests themself:

  1. What problem did this actually solve for you and what is the evidence that it solved the problem?
  2. What is it that I am missing?

@leadZERO

Copy link
Copy Markdown
Contributor Author

Good questions, I forgot to put this into context...

I've really want the ability to connect a 2nd computer to the left USB connection so I didn't have to use a wireless dongle. With AI's help, I have a version of this that more or less works (leadZERO#2) but want to work on testing things a lot more. It was adding the left-half connection manually that won't make sense once it's using the Agent for adding the left host connection.

Before submitting a big PR like that, I wanted to better understand how things were submitted. During my testing for the aforementioned left-host-connection change, I discovered a few things that the agent and I changed along the way. This was the smallest change that seemed like it might be a fix (but again, this was largely agent driven). I had the agent pull this out to submit as my first PR just to start understanding better how I might go about the larger submissions. (There were 2 other items that seemed to fix issues I was having but those were again more substantive:: leadZERO#8 and leadZERO#7).

Anyway, I hope that missing context helps.

@kareltucek

kareltucek commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Well, this is a nice illustration of the problem with ai agents. They tend to follow the wrong scent and then fail to put their findings into the real world context.

To answer your question in general:

  • Small, well isolated issue / feature / refactor is good.

  • Ai code is welcome, but has to be reasonably validated and the validation has to be sufficiently decoupled from the ai reasoning, and consider real-world impacts of the changeset.

  • It has to have a short human-written introduction that contains a human-understandable summary of the issue. In the context of the above, it would be explanation of what is wrong, why it is wrong, and what the evidence is. It has to contain evidence and sound argumentation in a well formatted manner. (Verbose ai analysis with details can follow later.) This is just an AI-generated wall of text that uses words that try to sound technical, but in reality don't carry much real meaning.

What I see here is (without offense) unfortunately just a noise submitted without any (visible) validation, for me to pick out things from.

That could still be useful if there was some story behind it (reproducible or even non-reproducible but present issue, or a specific concern).


Regarding c2usb, if you believe you have found an issue, please report it here: https://github.com/IntergatedCircuits/c2usb .

(Prior experience (learned the hard way) says that Benedek reacts promptly if (and only if) a real issue is reported.)


Looking forward to further PRs, but it can happen that I will prefer if you continue maintaining that usb extension on your own fork.

@kareltucek kareltucek closed this Oct 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants