Modify

Opened 9 years ago

Closed 8 years ago

#2444 closed defect (fixed)

Slow bidirectional printing in p910nd, fix available

Reported by: anonymous Owned by: developers
Priority: normal Milestone: Kamikaze 8.09
Component: packages Version:
Keywords: p910nd slow Cc: n2rjt@…

Description

With a Canon Pixma printer, the printing is horrendously slow in bidirectional mode.
When printing from Linux, the fix is to disable bidirectional.
When printing from XP, the printer driver requires bidirectional, though.
The problem is caused by the huge amount of data flowing from the printer back to the driver.
The back-data gets flow-controlled on its way out of openwrt, and since it carries the stream acknowledgements for the driver-to-printer stream, the printing is slow.
The fix is to pace the printer-to-driver stream, and make stream output asynchronous via select, like the stream input is.
Contact n2rjt@… for a patch. With this patch, printing is fast again in bidirectional mode, from XP or Linux.

Attachments (4)

p910nd.c (12.8 KB) - added by n2rjt@… 9 years ago.
modified source file
p910nd.patch (5.5 KB) - added by n2rjt@… 9 years ago.
patch file
p910nd_version_0.91.patch (17.0 KB) - added by Tom St. 9 years ago.
p910nd version 0.91 without unmaintained patches
p910nd-0.91.patch (8.4 KB) - added by n2rjt@… 9 years ago.
bidirectional performance, and use of open() family, patches raw 0.91

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by florian

  • Cc n2rjt@… added

Can you please paste the patch ?

Changed 9 years ago by n2rjt@…

modified source file

comment:2 Changed 9 years ago by n2rjt@…

I realized I don't have the original source file at hand,
so I uploaded the modified source. Sorry, I know that's not the convention.
I'm re-fetching the tree now, so I will be able to create the patch soon.

comment:3 Changed 9 years ago by n2rjt@…

--- p910nd.c.orig 2007-09-29 17:03:03.000000000 -0700
+++ p910nd.c 2007-09-29 17:06:53.000000000 -0700
@@ -144,73 +144,185 @@

(void)close(lockfd);

}


-ssize_t safe_write(int fd, char *buf, size_t count)
+typedef struct {
+ int detectEof;
+ int infd;
+ int outfd;
+ int startidx;
+ int endidx;
+ int bytes;
+ int totalin;
+ int totalout;
+ int eof;
+ int err;
+ char buffer[8192];
+} Buffer_t;
+
+void initBuffer( Buffer_t *b, int infd, int outfd, int detectEof )
+{
+ b->detectEof = detectEof;
+ b->infd = infd;
+ b->outfd = outfd;
+ b->startidx = 0;
+ b->endidx = 0;
+ b->bytes = 0;
+ b->totalin = 0;
+ b->totalout = 0;
+ b->eof = 0;
+ b->err = 0;
+}
+
+void prepBuffer( Buffer_t *b, fd_set *readfds, fd_set *writefds )

{

  • size_t offset = 0;
  • while (offset < count) {
  • ssize_t n = write(fd, buf + offset, count - offset);
  • if (n < 0 && errno != EINTR)
  • return n;
  • if (n > 0)
  • offset += n;

+ if (!b->err && b->bytes != 0) {
+ FD_SET(b->outfd, writefds);

}

+ if (!b->eof && b->bytes < sizeof(b->buffer)) {
+ FD_SET(b->infd, readfds);
+ }
+}

  • return offset;

+ssize_t readBuffer( Buffer_t *b )
+{
+ int avail;
+ ssize_t result = 1;

+ if (b->bytes == 0
b->err) {

+ b->startidx = b->endidx = 0;
+ avail = sizeof(b->buffer);
+ } else if (b->bytes == sizeof(b->buffer)) {
+ avail = 0;
+ } else if (b->endidx > b->startidx) {
+ avail = sizeof(b->buffer) - b->endidx;
+ } else {
+ avail = b->startidx - b->endidx;
+ }
+ if (avail) {
+ result = read(b->infd, b->buffer+b->endidx, avail);
+ if (result > 0) {
+ b->endidx += result;
+ b->totalin += result;
+ b->bytes += result;
+ if (b->endidx == sizeof(b->buffer))
+ b->endidx = 0;

+ } else if (result < 0
b->detectEof) {

+ b->eof = 1;
+ }
+ }
+ return result;
+}
+
+ssize_t writeBuffer( Buffer_t *b )
+{
+ int avail;
+ ssize_t result = 1;

+ if (b->bytes == 0
b->err) {

+ avail = 0;
+ } else if (b->endidx > b->startidx) {
+ avail = b->endidx - b->startidx;
+ } else {
+ avail = sizeof(b->buffer) - b->startidx;
+ }
+ if (avail) {
+ result = write(b->outfd, b->buffer + b->startidx, avail);
+ if (result < 0) {
+ syslog(LOG_ERR, "write: %m\n");
+ b->err = 1;
+ } else {
+ b->startidx += result;
+ b->totalout += result;
+ b->bytes -= result;
+ if (b->startidx == sizeof(b->buffer))
+ b->startidx = 0;
+ }
+ }
+ return result;

}


/* Copy network socket to FILE f until EOS */
int copy_stream(int fd, int lp)
{

  • int nread, rcvd = 0, sent = 0;
  • char buffer[8192];

+ int result;
+ Buffer_t inbuffer;
+ initBuffer( &inbuffer, fd, lp, 1 );

if (bidir) {

  • for (;;) {
  • fd_set readfds;
  • int result;

+ struct timeval now;
+ struct timeval then;
+ struct timeval timeout;
+ int timer = 0;
+ Buffer_t outbuffer;
+ initBuffer ( &outbuffer, lp, fd, 0 );
+ fd_set readfds;
+ fd_set writefds;
+ FD_ZERO(&readfds);
+ FD_ZERO(&writefds);
+ FD_SET(lp, &readfds);
+ FD_SET(fd, &readfds);

+ while ( (FD_ISSET(fd, &readfds))

+ (FD_ISSET(lp, &writefds))) {

int maxfd = lp > fd ? lp : fd;

  • FD_ZERO(&readfds);
  • FD_SET(lp, &readfds);
  • FD_SET(fd, &readfds);
  • result = select(maxfd + 1, &readfds, 0, 0, 0);

+ if (timer) {
+ gettimeofday(&now,0);
+ If timer expired, clear timer
+
else don't try reading from printer

+ if ((now.tv_sec > then.tv_sec)

+ (now.tv_sec == then.tv_sec &&
+ now.tv_usec > then.tv_usec)) {
+ timer = 0;
+ } else {
+ timeout.tv_sec = then.tv_sec;
+ timeout.tv_usec = then.tv_usec;
+ FD_CLR(lp, &readfds);
+ }
+ }
+ if (timer) {
+ result = select(maxfd + 1, &readfds, &writefds, 0, &timeout);
+ } else {
+ result = select(maxfd + 1, &readfds, &writefds, 0, 0);
+ }

if (result < 0)

return (result);

  • if (result == 0)
  • continue;

if (FD_ISSET(fd, &readfds)) {

  • nread = read(fd, buffer, sizeof(buffer));
  • if (nread <= 0)
  • break;
  • if (safe_write(lp, buffer, nread) < 0) {
  • syslog(LOG_ERR, "write: %m\n");
  • break;
  • }
  • rcvd += nread;

+ result = readBuffer(&inbuffer);

}
if (FD_ISSET(lp, &readfds)) {

  • nread = read(lp, buffer, sizeof(buffer));
  • if (nread > 0) {
  • safe_write(fd, buffer, nread);
  • sent += nread;

+ result = readBuffer(&outbuffer);
+ Pace the printer data more slowly
+ if (result >= 0) {
+ gettimeofday(&then,0);
+
wait 100 msec
+ then.tv_usec += 100000;
+ if (then.tv_usec > 1000000) {
+ then.tv_usec -= 1000000;
+ then.tv_sec ++;
+ }
+ timer = 1;

}

}

+ if (FD_ISSET(lp, &writefds)) {
+ result = writeBuffer(&inbuffer);
+ }
+ if (FD_ISSET(fd, &writefds)) {
+ result = writeBuffer(&outbuffer);
+ /* If socket write error, stop reading from it */
+ if (result < 0)
+ inbuffer.eof = 1;
+ }
+ FD_ZERO(&readfds);
+ FD_ZERO(&writefds);
+ prepBuffer( &inbuffer, &readfds, &writefds );
+ prepBuffer( &outbuffer, &readfds, &writefds );

}
syslog(LOG_NOTICE, "Finished job: %d bytes received, %d bytes sent\n",

  • rcvd, sent);

+ inbuffer.totalout,
+ outbuffer.totalout);

return (0);

} else {

  • while ((nread = read(fd, buffer, sizeof(buffer))) > 0) {
  • if (safe_write(lp, buffer, nread) < 0) {
  • syslog(LOG_ERR, "write: %m\n");
  • break;
  • }
  • rcvd += nread;

+ while ((result = readBuffer( &inbuffer)) > 0) {
+ (void)writeBuffer( &inbuffer);

}

  • syslog(LOG_NOTICE, "Finished job: %d bytes received\n", rcvd);
  • return (nread);

+ syslog(LOG_NOTICE, "Finished job: %d bytes received\n", inbuffer.totalout);
+ return (result);

}

}


Changed 9 years ago by n2rjt@…

patch file

comment:4 Changed 9 years ago by florian

  • Resolution set to fixed
  • Status changed from new to closed

Applied in [9090], thanks !

comment:5 Changed 9 years ago by Torx

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hello,
The patch can not be applied to the current version 0.91. IMHO functional changes like this should be posted to the original author of the package (upstream). It is a waste of time if we make local changes here in this project and if we want to catch up with the current version, we rewrite the patch again. Please submit the patch to the p910nd developers and ask for integration, in the meantime a submit an patch to compile p910nd version 0.91.

Changed 9 years ago by Tom St.

p910nd version 0.91 without unmaintained patches

comment:6 follow-up: Changed 9 years ago by n2rjt@…

will submit patch against 0.9.1 to here and p910nd this evening.

comment:7 in reply to: ↑ 6 Changed 9 years ago by Tom St.

Hello,

will submit patch against 0.9.1 to here and p910nd this evening.

Ok, perfect! I tried to contact the p910nd author without success. I could try another e-mail source address, perhaps my gmx account was blacklisted or filtered out. As soon as you posted the patch here, i will try to contact the original author of p910nd again and ask him to integrate it. However, thank you very much for watching this ticket and (re-)writing the patch.

Cheers,
Tom

Changed 9 years ago by n2rjt@…

bidirectional performance, and use of open() family, patches raw 0.91

comment:8 Changed 9 years ago by n2rjt@…

The attached file patches p910nd-0.91 from Etherboot to include:
a) use of open() instead of fopen()
b) instead of changing the default printer location, allows PRINTERFILE to be specified on compiler command line with -DPRINTERFILE="/dev/printers/%c". If you want the same default as current openwrt version of p910nd, the Makefile needs to change, to add this option. I did not do that.
c) Use of LOG_NOTICE for print job logging
d) Use of select() for writes as well as reads, with time delay on reads from printer, to increase performance of bidirectional printing to chatty printers.

comment:9 Changed 8 years ago by anonymous

-> Fixed in #3729?

comment:10 Changed 8 years ago by n2rjt@…

It was fixed upstream several months ago, and is available as p910nd 0.9.2.
I haven't looked carefully at 3729, but it looks like it:
a) updates openwrt's p910nd to the latest, which I presume to be 0.9.2 or later.
b) removes the local patches I submitted in the history above, which are unnecessary since 0.9.2 includes those changes.

So, yes, it appears to me that 3729 includes the fix.
I have not tested 3729 on a router, since I don't currently have a router with USB.

This particular item can probably be closed.
--
Dave Brown

comment:11 Changed 8 years ago by blogic

  • Milestone set to Kamikaze 808

comment:12 Changed 8 years ago by florian

  • Resolution set to fixed
  • Status changed from reopened to closed

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.