Flush serial output buffer before closing on Linux and ignore permission errors changing closing_wait (IDFGH-10909) #7
Conversation
github-actions
bot
changed the title
| try: | ||
| fcntl.ioctl(self.serial.fd, termios.TIOCSSERIAL, buf) | ||
| except PermissionError: | ||
| pass |
There was a problem hiding this comment.
How about using the tcflush here in except? This way we would still try to wait for the data to be sent if you had permission to set closing_wait, but would also mean that if you don't have the permission console won't get stuck. What do you think?
The way it is implemented currently, it makes no sense to set the closing_wait as based on documentation tcflush will discard all data waiting for writing. The closing_wait was set to 1 second to at least try to finish writing while at the same time making sure the console won't get stuck for a noticeable time. We need to make sure it does work as well for some slower systems that might benefit from the longer timeout.
There was a problem hiding this comment.
The value used for closing_wait is irrelevant because closing the serial port has no impact on whether or not cdc_acm will continue to transmit previously written characters. It's just a thing the tty layer does that waits for all data to be transmitted before letting close() finish. The data will still be written to the serial port. I will change it to disable the timeout instead of waiting unnecessarily for 1 second.
This is supposed to be an interactive application. Sending characters and then expecting them to have all been received without waiting for a response shouldn't need to be supported? Everything is being sent 1 character at a time and cdc_acm only has 16 transmit buffers so this is limited to a queue of only 16 characters.
I'm going to rework it a bit because I've made another kernel patch that makes it easier to fix the closing_wait time using udev rules. We only need to discard data if the timeout hasn't been changed.
There was a problem hiding this comment.
I've included a 300ms delay before discarding data to match your other 300ms delay in write().
I've now taken this out because it'll be annoying when it can't change the closing_wait time and doesn't need to because there's nothing buffered or the device is reading it.
|
The only problem I have with this now is that the |
nomis
force-pushed
the
use-tcflush
branch
2 times, most recently
from
8436ea6 to
09d92dd
Compare
The |
There was a problem hiding this comment.
Overall LGTM, left two suggestions. Please make sure that pre-commit passes. I would suggest following a new section of README - to enable a pre-commit run with every commit to the repo.
| # `65535` means "don't wait at all" | ||
| if serial_struct[12] == 65535: | ||
| return | ||
|
|
||
| serial_struct[12] = 65535 |
There was a problem hiding this comment.
Can you please make those values hexa - 0xFFFF? Seems a less of 'random' number at first glance.
There was a problem hiding this comment.
I've added an ASYNC_CLOSING_WAIT_NONE variable for this.
Change the closing_wait time to "no wait". Handle permission errors by flushing the output buffer. If the driver implements it, this will stop close() from blocking. Linux requires CAP_SYS_ADMIN to change the closing_wait time. It may already have been set to "no wait" so nothing needs to be done for this case.
espressif-bot
added
Status: In Progress
Status: Reviewing
and removed
Status: Opened
Status: In Progress
labels
|
@nomis Thank you for your valuable contribution. |
If the driver implements it, flushing the output buffer will stop close() from blocking.
Linux requires CAP_SYS_ADMIN to change the closing_wait time, so ignore permission errors.