Harmonize WIN32 implementation of serial time-outs with Posix one #1249

mariusgreuel · 2022-12-29T14:51:16Z

The Posix time-out mechanism for serial reads is specified as the total time for any 1024 byte chunk of data.

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.

We should probably change it to something like this, or even read the data in 1024 byte chunks, as the Posix one does.

static BOOL serial_w32SetTimeOut(HANDLE hComPort, DWORD timeout) // in ms
{
	COMMTIMEOUTS ctmo;
	ZeroMemory (&ctmo, sizeof(COMMTIMEOUTS));
	ctmo.ReadIntervalTimeout = 0;
	ctmo.ReadTotalTimeoutMultiplier = 0;
	ctmo.ReadTotalTimeoutConstant = timeout;

	return SetCommTimeouts(hComPort, &ctmo);
}

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

The text was updated successfully, but these errors were encountered:

mcuee · 2023-01-03T02:36:27Z

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

So I think this can be postponed after 7.1 release.

stefanrueger · 2023-01-03T11:08:53Z

@mariusgreuel I broadly agree with your analysis and had similar thoughts when reading the documentation. On the issue of reading in blocks of 1024 characters, I wondered whether it's OK to actually read byte by byte. It's serial comms after all and the overhead involved in bytewise read is minimal given today's computing power, even on a RPi.

mcuee · 2023-03-27T02:42:37Z

@mariusgreuel

I add this as part of the AVRDUDE 7.2 milestone. But you can remove this if you feel that this is of lower priority. Thanks.

mcuee · 2023-08-15T03:07:08Z

@mariusgreuel

Just wondering if you have some time to look at this. Thanks.

mcuee · 2023-10-28T13:29:23Z

@stefanrueger

Looks like @mariusgreuel is busy. Just wondering if you want to take a stab at this one.

mcuee · 2023-10-28T13:41:43Z

Here is the first test.

PS C:\work\avr\avrdude_test\avrdude_main> git diff
diff --git a/src/ser_win32.c b/src/ser_win32.c
index b7480ac2..c5b9fafa 100644
--- a/src/ser_win32.c
+++ b/src/ser_win32.c
@@ -88,8 +88,8 @@ static BOOL serial_w32SetTimeOut(HANDLE hComPort, DWORD timeout) // in ms
 {
        COMMTIMEOUTS ctmo;
        ZeroMemory (&ctmo, sizeof(COMMTIMEOUTS));
-       ctmo.ReadIntervalTimeout = timeout;
-       ctmo.ReadTotalTimeoutMultiplier = timeout;
+       ctmo.ReadIntervalTimeout = 0;
+       ctmo.ReadTotalTimeoutMultiplier = 0;
        ctmo.ReadTotalTimeoutConstant = timeout;

        return SetCommTimeouts(hComPort, &ctmo);

And it is good for my serialupdi MSYS2 mingw64 Sleep test. The behavior is the same as git main mingw64 build:
-v and -vvvv will fail.

PS>.\avrdude_issue1249 -c serialupdi -P COM41 -p avr64dd32 -b 230400
avrdude_issue1249: AVR device initialized and ready to accept instructions
avrdude_issue1249: device signature = 0x1e961a (probably avr64dd32)

avrdude_issue1249 done.  Thank you.

PS>.\avrdude_issue1249 -c serialupdi -P COM41 -p avr64dd32 -b 230400 -v

avrdude_issue1249: Version 7.2-20231028 (e9999120)
                   Copyright the AVRDUDE authors;
                   see https://github.com/avrdudes/avrdude/blob/main/AUTHORS

                   System wide configuration file is C:\work\avr\avrdude_test\avrdude_bin\avrdude.conf

                   Using Port                    : COM41
                   Using Programmer              : serialupdi
                   Overriding Baud Rate          : 230400
avrdude_issue1249: serial_baud_lookup(): using non-standard baud rate: 230400                   AVR Part                      : AVR64DD32
                   RESET disposition             : dedicated
                   RETRY pulse                   : SCK
                   Serial program mode           : yes
                   Parallel program mode         : yes
                   Memory Detail                 :
...
avrdude_issue1249: serial_baud_lookup(): using non-standard baud rate: 230400
avrdude_issue1249 serialupdi_initialize() error: UPDI link initialization failed
avrdude_issue1249 main() error: initialization failed, rc=-1
                  - double check the connections and try again
                  - use -b to set lower baud rate, e.g. -b 115200
                  - use -F to override this check
avrdude_issue1249: leaving NVM programming mode

avrdude_issue1249 done.  Thank you.

PS>.\avrdude_issue1249 -c serialupdi -P COM41 -p avr64dd32 -b 230400 -vvvv

avrdude_issue1249: Version 7.2-20231028 (e9999120)
                   Copyright the AVRDUDE authors;
                   see https://github.com/avrdudes/avrdude/blob/main/AUTHORS

                   System wide configuration file is C:\work\avr\avrdude_test\avrdude_bin\avrdude.conf

                   Using Port                    : COM41
                   Using Programmer              : serialupdi
                   Overriding Baud Rate          : 230400
avrdude_issue1249: opening serial port ...
avrdude_issue1249: serial_baud_lookup(): using non-standard baud rate: 230400
avrdude_issue1249: sending 1 bytes [0x00]
avrdude_issue1249: send: . [00]
avrdude_issue1249: recv: . [00]
                   AVR Part                      : AVR64DD32
                   RESET disposition             : dedicated
                   RETRY pulse                   : SCK
                   Serial program mode           : yes
                   Parallel program mode         : yes
                   Memory Detail                 :
...
avrdude_issue1249: STCS 0x08 to address 0x03
avrdude_issue1249: sending 3 bytes [0x55, 0xc3, 0x08]
avrdude_issue1249: send: U [55] . [c3] . [08]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: STCS 0x80 to address 0x02
avrdude_issue1249: sending 3 bytes [0x55, 0xc2, 0x80]
avrdude_issue1249: send: U [55] . [c2] . [80]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: LDCS from 0x00
avrdude_issue1249: sending 2 bytes [0x55, 0x80]
avrdude_issue1249: send: U [55] . [80]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: serialupdi_recv(): programmer is not responding
avrdude_issue1249: check failed
avrdude_issue1249: datalink not active, resetting ...
avrdude_issue1249: sending double break
avrdude_issue1249: send: . [00]
avrdude_issue1249: recv: . [00]
avrdude_issue1249: send: . [00]
avrdude_issue1249: recv: . [00]
avrdude_issue1249: serial_baud_lookup(): using non-standard baud rate: 230400
avrdude_issue1249: STCS 0x08 to address 0x03
avrdude_issue1249: sending 3 bytes [0x55, 0xc3, 0x08]
avrdude_issue1249: send: U [55] . [c3] . [08]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: STCS 0x80 to address 0x02
avrdude_issue1249: sending 3 bytes [0x55, 0xc2, 0x80]
avrdude_issue1249: send: U [55] . [c2] . [80]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: LDCS from 0x00
avrdude_issue1249: sending 2 bytes [0x55, 0x80]
avrdude_issue1249: send: U [55] . [80]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: serialupdi_recv(): programmer is not responding
avrdude_issue1249: check failed
avrdude_issue1249: restoring datalink failed
avrdude_issue1249 serialupdi_initialize() [serialupdi.c:569] error: UPDI link initialization failed
avrdude_issue1249 main() [main.c:1399] error: initialization failed, rc=-1
                  - double check the connections and try again
                  - use -b to set lower baud rate, e.g. -b 115200
                  - use -F to override this check
avrdude_issue1249: leaving NVM programming mode
avrdude_issue1249: sending reset request
avrdude_issue1249: STCS 0x59 to address 0x08
avrdude_issue1249: sending 3 bytes [0x55, 0xc8, 0x59]
avrdude_issue1249: send: U [55] . [c8] Y [59]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: sending release reset request
avrdude_issue1249: STCS 0x00 to address 0x08
avrdude_issue1249: sending 3 bytes [0x55, 0xc8, 0x00]
avrdude_issue1249: send: U [55] . [c8] . [00]
avrdude_issue1249: ser_recv(): programmer is not responding
avrdude_issue1249: STCS 0x0C to address 0x03
avrdude_issue1249: sending 3 bytes [0x55, 0xc3, 0x0c]
avrdude_issue1249: send: U [55] . [c3] . [0c]
avrdude_issue1249: ser_recv(): programmer is not responding

avrdude_issue1249 done.  Thank you.

mcuee · 2023-10-28T13:43:30Z

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

@stefanrueger

Just wondering how to quickly search hard-coded timeout in avrdude code base. Thanks.

stefanrueger · 2023-10-28T14:05:11Z

@mcuee Afraid to tell I am not the right person to deal with this. Should be done by someone who has a clear grasp of the COMMTIMEOUTS structure in windows.

mcuee · 2023-10-28T14:09:29Z

@mcuee Afraid to tell I am not the right person to deal with this. Should be done by someone who has a clear grasp of the COMMTIMEOUTS structure in windows.

No problem. I will just give the proposed changes by @mariusgreuel a try on a few programmers and bootloaders to see how it goes.

mcuee · 2023-10-29T06:58:44Z

Simple tests using -c urclock or -c arduino are also positive, using an Arduino Nano CH340 clone.

PS>.\avrdude_issue1249 -c urclock -P ch340 -p m328p -xshowall
avrdude_issue1249: AVR device initialized and ready to accept instructions
0 2022-11-16 19.36 blink_m328p_readback.hex 960 store 31260 meta 36 boot 512 o4.4 
--s-h-r-- vector 0 (RESET) ATmega328P
PS>.\avrdude_issue1249 -c urclock -P ch340 -p m328p -U .\hex3\blink_m328p_readback.hex -qq && echo OK
OK
PS>.\avrdude_issue1249 -c arduino -P ch340 -p m328p -U .\hex3\blink_m328p_readback.hex -qq && echo OK
OK

mcuee · 2023-10-29T07:01:14Z

Maybe I will just post a PR using @mariusgreuel's codes so that more people can carry out the testing and review.

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;

@mariusgreuel

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.

@mariusgreuel

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.

@mariusgreuel

…amming interface (#1563) * Harmonize WIN32 implementation of serial time-outs with Posix This is from @mariusgreuel in Issue #1249. #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. * Update NEWS * Always print error if part and programmer doesn't have a common programming interface even though the user uses -F. It won't exit though. * Don't mention -F in error message if -F is already used --------- Co-authored-by: mcuee <[email protected]> Co-authored-by: Stefan Rueger <[email protected]>

mariusgreuel self-assigned this Dec 29, 2022

mariusgreuel added the enhancement New feature or request label Dec 29, 2022

mcuee added this to the AVRDUDE 7.2 milestone Jan 9, 2023

mcuee modified the milestone: AVRDUDE 7.2 Mar 20, 2023

mcuee removed this from the AVRDUDE 7.2 milestone May 7, 2023

mcuee mentioned this issue Jun 2, 2023

CH341A futher improvement -- to support MSVC build #1339

Closed

mcuee mentioned this issue Oct 27, 2023

Report: Incorrect SerialUPDI control of AVR64EA32 #1529

Closed

mcuee mentioned this issue Nov 1, 2023

Harmonise WIN32 and POSIX serial comms timeout #1550

Merged

stefanrueger linked a pull request Nov 2, 2023 that will close this issue

Harmonise WIN32 and POSIX serial comms timeout #1550

Merged

stefanrueger closed this as completed in #1550 Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harmonize WIN32 implementation of serial time-outs with Posix one #1249

Harmonize WIN32 implementation of serial time-outs with Posix one #1249

mariusgreuel commented Dec 29, 2022

mcuee commented Jan 3, 2023

stefanrueger commented Jan 3, 2023

mcuee commented Mar 27, 2023

mcuee commented Aug 15, 2023

mcuee commented Oct 28, 2023

mcuee commented Oct 28, 2023

mcuee commented Oct 28, 2023

stefanrueger commented Oct 28, 2023

mcuee commented Oct 28, 2023

mcuee commented Oct 29, 2023 •

edited

Loading

mcuee commented Oct 29, 2023

Harmonize WIN32 implementation of serial time-outs with Posix one #1249

Harmonize WIN32 implementation of serial time-outs with Posix one #1249

Comments

mariusgreuel commented Dec 29, 2022

mcuee commented Jan 3, 2023

stefanrueger commented Jan 3, 2023

mcuee commented Mar 27, 2023

mcuee commented Aug 15, 2023

mcuee commented Oct 28, 2023

mcuee commented Oct 28, 2023

mcuee commented Oct 28, 2023

stefanrueger commented Oct 28, 2023

mcuee commented Oct 28, 2023

mcuee commented Oct 29, 2023 • edited Loading

mcuee commented Oct 29, 2023

mcuee commented Oct 29, 2023 •

edited

Loading