Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harmonise WIN32 and POSIX serial comms timeout #1550

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

mcuee
Copy link
Collaborator

@mcuee mcuee commented Nov 1, 2023

This is from @mariusgreuel in Issue #1249.

The WIN32 time-out is specified as ReadTotalTimeoutConstant + buflen * ReadTotalTimeoutMultiplier, which makes it potentially much longer than the POSIX one (i.e. at least double). ReadIntervalTimeout is also set to timeout, which essentially renders the interval timeout ineffective, so IMHO, we should explicitly disable that feature.

This is from @mariusgreuel in Issue avrdudes#1249.
avrdudes#1249

> The WIN32 time-out is specified as ReadTotalTimeoutConstant + buflen * ReadTotalTimeoutMultiplier, which makes it potentially much longer than the Posix one (i.e. at least double). ReadIntervalTimeout is also set to timeout, which essentially renders the interval timeout ineffective, so IMHO, we should explicitly disable that feature.
@mcuee
Copy link
Collaborator Author

mcuee commented Nov 1, 2023

I've carried some tests and it seems to work. However, as mentioned by @mariusgreuel, this needs more tests.

#1249 (comment)

However, this probably needs some extensive testing for those programmers where the time-out is carefully chosen to match the requirements.

See https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddser/ns-ntddser-_serial_timeouts

@mcuee mcuee added the enhancement New feature or request label Nov 1, 2023
@mcuee mcuee changed the title Harmonize WIN32 implementation of serial time-outs with Posix Harmonize WIN32 implementation of serial time-outs with POSIX implementation (WIP, DNM) Nov 1, 2023
@mcuee
Copy link
Collaborator Author

mcuee commented Nov 1, 2023

COMMTIMEOUTS structure (winbase.h)
https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-commtimeouts

typedef struct _COMMTIMEOUTS {
  DWORD ReadIntervalTimeout;
  DWORD ReadTotalTimeoutMultiplier;
  DWORD ReadTotalTimeoutConstant;
  DWORD WriteTotalTimeoutMultiplier;
  DWORD WriteTotalTimeoutConstant;
} COMMTIMEOUTS, *LPCOMMTIMEOUTS;

@stefanrueger
Copy link
Collaborator

I agree with this PR. After reading up on the COMMITTIMEOUTS structure had come to the same conclusion as @mariusgreuel. Quite a few users and applications will be affected by the change. Let's cross our fingers that this is not one of those situations where a particular application relied on the larger timeout (without AVRDUDE knowing about it). But yes, it's better to make this change now than 10 seconds before the release of 7.3 as, if it breaks something, we might hear back before 7.3

@stefanrueger stefanrueger changed the title Harmonize WIN32 implementation of serial time-outs with POSIX implementation (WIP, DNM) Harmonise WIN32 and POSIX serial comms timeout Nov 1, 2023
@mcuee
Copy link
Collaborator Author

mcuee commented Nov 1, 2023

@stefanrueger

Glad to hear that you agree with this PR. I agree with you that it is better to merge this soon so that issues will be reported within the next two months.

@stefanrueger stefanrueger linked an issue Nov 2, 2023 that may be closed by this pull request
@stefanrueger stefanrueger merged commit c37a909 into avrdudes:main Nov 3, 2023
10 checks passed
@mcuee mcuee deleted the issue1249 branch November 3, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harmonize WIN32 implementation of serial time-outs with Posix one
2 participants