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

Buffering issues #1

Closed
zcsahok opened this issue Jan 4, 2018 · 2 comments
Closed

Buffering issues #1

zcsahok opened this issue Jan 4, 2018 · 2 comments

Comments

@zcsahok
Copy link
Contributor

zcsahok commented Jan 4, 2018

I tried to use winkeydaemon with a WK clone and had severe issues due to limited UDP read length and overflowing WK's serial buffer.
I'm not sure what the state of this code is, but the implementation of the internal buffering is clearly wrong. Example: it sends 30 chars right away and the looks for an echo and as soon as echo is received the rest of the buffer is sent: (around line 431, whole $string is sent then len($echo)<=9)

 if (length($echo) > 9) {
...
 } else {
   my $out = $string;
   $count = $port->write($out);
}

As the string from UDP req can be quite long this results in buffer overflow in WK.
Also the UDP read limit (32 chars, line 250) is way too low, a longer CQ call won't fit.
WK status evaluation is also flawed as it is done in an if/elsif/elsif/.. construct, so the first bit masks
the subsequent ones.

I've checked in an improved and actually working version to my fork and I'm happy to send a PR.

Instead of looking for echoed chars my code leverages the XOFF status bit sent by WK. Data are sent in a rate-limited way (instead of pouring 30 chars a time) to ensure that XOFF can be acted upon in time. UDP reading has been also refactored to use a more fine granular select() instead of alarm. UDP and serial timeouts add up to 15 ms, that gives a byte rate of ~60 cps. As the round trip time on serial IF at 1200 baud is ~20 ms this gives a reasonable accuracy for XOFF handling without lowering too much the data rate.

Zoli, ha5cqz

@N0NB
Copy link
Owner

N0NB commented Jan 29, 2018

Hi Zoli.

Thanks for the reminder.

Yes, please send a PR for your fix.

@N0NB
Copy link
Owner

N0NB commented Mar 28, 2018

Closing as patch has been submitted and applied.

@N0NB N0NB closed this as completed Mar 28, 2018
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 a pull request may close this issue.

2 participants