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

ZMODEM send: Disable window if receiver does not set CANFDX flag #408

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

cmcqueen
Copy link
Contributor

If the receiver does not set CANFDX, then the sender should wait for the receiver to send ZACK for each data subpacket before sending more data.

This allows ZMODEM to be used with a small embedded device that needs ZMODEM to pause while it writes to Flash memory.

If the receiver does not set CANFDX, then the sender should wait for the
receiver to send ZACK for each data subpacket before sending more data.
@cmcqueen
Copy link
Contributor Author

cmcqueen commented Jan 4, 2025

@zmatsuo
Copy link
Member

zmatsuo commented Jan 8, 2025

Thanks for pull request.

In what environment have you tested this?
I would like to test this.

@cmcqueen
Copy link
Contributor Author

cmcqueen commented Jan 9, 2025

I have a small embedded device with small serial RX buffer and slow Flash writes. Serial speed is 115200 bps. I send a firmware file to it.

I could not compile my modified source code myself. I have done these tests with Tera Term 5.3:

  1. With ZmodemWinSize=32767, transfer fails (halts with no further progress) after 32822 bytes.
    • I think the embedded device's serial RX buffer overflows.
  2. With ZmodemWinSize=0, transfer succeeds.
  3. With ZmodemLog=on, I see in the device ZRINIT, it does not set CANFDX or CANOVIO flags.

@cmcqueen
Copy link
Contributor Author

cmcqueen commented Jan 9, 2025

Would you like me to send you ZMODEM.LOG files for tests 1 and 2?

If you compile my code change and send me the ttermpro.exe, I can test that and send you the log file for that too.

@zmatsuo
Copy link
Member

zmatsuo commented Jan 10, 2025

I would not be able to test PR in my environment.

I built Tera Term from zmodem-rinit-canfdx branch (2d47c21).
https://ci.appveyor.com/project/teraterm/github-snapshot/build/artifacts

Please test this.
If it looks OK, I will merge this PR.

@cmcqueen
Copy link
Contributor Author

ありがとうございます。
I have tested that build. It behaved as expected.

  1. I tested with the CANFDX flag clear. It sent subpackets of type ZCRCQ as I hoped.
    • The receiver was able to write Flash and then send ZACK.
  2. I tested with the CANFDX flag set. It sent subpackets of type ZCRCG as expected.

@cmcqueen
Copy link
Contributor Author

I found a problem: I tried testing with TERATERM.INI setting

ZmodemEscCtl=on

Then I found that my code change didn't work.

The problem seems to be the return; statement above it.

	if (zv->CtlEsc) {
		if ((zv->RxHdr[ZF0] & ESCCTL) == 0) {
			zv->ZState = Z_SendInitHdr;
			ZSendInitHdr(zv);
			return;                // <--- This prevents my code change from running
		}
	} else

This ensures that the CANFDX test is effective even if ZSINIT needs to be sent.
Move it before the code that checks CtlEsc, so it works even if ZSINIT needs to be sent.
@zmatsuo
Copy link
Member

zmatsuo commented Jan 12, 2025

  1. I tested with the CANFDX flag clear. It sent subpackets of type ZCRCQ as I hoped.
    The receiver was able to write Flash and then send ZACK.
  2. I tested with the CANFDX flag set. It sent subpackets of type ZCRCG as expected.

😀👍

Then I found that my code change didn't work.

I built binary from zmodem-rinit-canfdx branch (006167b).
https://ci.appveyor.com/project/teraterm/github-snapshot/builds/51309904/artifacts

Please use this.

@cmcqueen
Copy link
Contributor Author

ありがとうございます。
I have tested the new binary. It behaves as expected for both ZmodemEscCtl=on and ZmodemEscCtl=off. It behaves as I expected for CANFDX flag both clear and set.

@zmatsuo
Copy link
Member

zmatsuo commented Jan 14, 2025

It behaves as I expected for CANFDX flag both clear and set.

👍

I would like to add the fix for this PR to history.

Is this okay?

Changes

  • ZMODEM: CANFDX flag handling is implemented in ZRINIT. While the receiver cannot receive data, the sender(Tera Term) can wait for the receiver.

In Japanese

  • ZMODEM: ZRINIT 内の CANFDX フラグの処理を実装した。受信側がデータを受信できない間、送信側(Tera Term)は受信側を待つことができます。

@cmcqueen
Copy link
Contributor Author

cmcqueen commented Jan 14, 2025

That sounds mostly good. I would suggest

  • Change "ZMODEM:" to "ZMODEM send:".
  • Change "in ZRINIT" to "when receiving ZRINIT".

That hopefully clarifies that the change is for ZModem Send, not Receive.

@zmatsuo zmatsuo merged commit cbf2010 into TeraTermProject:main Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants