Update timeout mechanism for serial on Linux#156
Merged
Conversation
Earlier, serial interface was configured with termios, and ioctl_tty (TCSETS2/TCGETS2 ioctl calls) was used for configuring a custom baud rate because it is not possible with termios. There have been observations on some kernels that the TCSETS2 with custom baud rate reset the earlier VMIN/VTIME parameters set via termios. This should fix that since everything is now made with one ioctl call. Since this is linux specific serial implementation and we anyway depend on termios2 here, there is no harm in removing the termios part. This also simplifies the code.
Previously, read() was configured to block with a timeout using VMIN/VTIME and ICANON(c_lflag) attributes. These attributes belong to the shared serial device and they might be changed by another process. This can make read() calls block without a timeout. If read() starts blocking forver, it can cause a deadlock when multiple threads try to send a dualmcu api request and wait for response bytes. Since wpc_internal.c:check_if_timeout_reached_locked() is called in these threads after writing or reading bytes, the timeout mechanism might never kick in and program will not exit. TCSETS2 ioctl call might also report success if *any* of the given attributes were updated so it is not guaranteed that the serial has correct attributes after the set_interface_attribs() call. However, the other attributes being invalid is likely to cause data corruptions and other errors which should be more visible elsewhere (as opposed to being stuck in read()).
This is similar to the earlier behavior; read() failures are treated as timeouts. Therefore, using debug log level for now.
sgunes-wirepas
force-pushed
the
fix/linux_serial
branch
from
b262f80 to
7a51dd8
Compare
GwendalRaoul
reviewed
Jul 3, 2026
|
|
||
| } | ||
|
|
||
| return (remaining_sec * 1000) + (remaining_nsec / 1000000); |
Contributor
There was a problem hiding this comment.
In our case it is fine as value are small, but is the compiler happy to silently convert time_t * 1000 as an int ?
Contributor
Author
There was a problem hiding this comment.
Good point, thanks! No warnings with the current sink-service build. I think it would be good to enable the -Wconversion flag (that introduces the warning for above line) and add explicit casts so avoid accidental conversions, but better in another commit because there are existing places in the code that would need to be checked.
GwendalRaoul
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should prevent potential deadlocks caused by read() blocking with no timeout. See individual commits for details.