Discussion:
Unused synth_immediate method
Okash Khawaja
2017-02-19 10:44:36 UTC
Permalink
Hi,

Some drivers like apollo and bns only assign synth_immediate method in
their spk_synth structs but don't seem to use it. For those drivers,
can it be set to NULL?

Okash
Samuel Thibault
2017-02-19 23:06:19 UTC
Permalink
Post by Okash Khawaja
Some drivers like apollo and bns only assign synth_immediate method in
their spk_synth structs but don't seem to use it. For those drivers,
can it be set to NULL?
I guess this method could be useful for some screen reading features,
just not used yet.

Samuel
Okash Khawaja
2017-02-20 12:23:45 UTC
Permalink
Hi,
Post by Samuel Thibault
Post by Okash Khawaja
Some drivers like apollo and bns only assign synth_immediate method in
their spk_synth structs but don't seem to use it. For those drivers,
can it be set to NULL?
I guess this method could be useful for some screen reading features,
just not used yet.
I see. Now because they are assigned but not used, they may cause confusion. For example, spk_synth_immediate uses outb. When migrating a synth to ttyio, if synth_immediate is left assigned, it may look like that synth isn't fully migrated yet.

We can leave as it is although I would prefer it to be set to NULL until it is actually used.
Post by Samuel Thibault
Samuel
Samuel Thibault
2017-02-20 12:57:11 UTC
Permalink
Hello,
Post by Okash Khawaja
Post by Samuel Thibault
Post by Okash Khawaja
Some drivers like apollo and bns only assign synth_immediate method in
their spk_synth structs but don't seem to use it. For those drivers,
can it be set to NULL?
I guess this method could be useful for some screen reading features,
just not used yet.
I see. Now because they are assigned but not used, they may cause confusion. For example, spk_synth_immediate uses outb.
Oh, I didn't realize this.
Post by Okash Khawaja
When migrating a synth to ttyio, if synth_immediate is left assigned, it may look like that synth isn't fully migrated yet.
Indeed, and that can only lead to problems later.
Post by Okash Khawaja
We can leave as it is although I would prefer it to be set to NULL
until it is actually used.
Indeed.

Ideally however we'd have a ttyio variant to assign here. It shouldn't
be hard, something like this (untested):

const char *spk_ttyio_synth_immediate(struct spk_synth *synth, const char *buff)
{
u_char ch;

while ((ch = *buff)) {
if (ch == '\n')
ch = synth->procspeech;
if (tty_write_room(speakup_tty) < 1 || !spk_ttyio_out(ch))
return buff;
buff++;
}
return NULL;
}

And the existing spk_synth_immediate should be renamed to
spk_serial_synth_immediate to make it clear it uses I/O ports.

Thinking about this, spk_ttyio_out should check the result of the write
operation, something like this:

int spk_ttyio_out(const char ch)
{
if (synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
if (ret == 0)
/* No room */
return 0;
if (ret < 0) {
pr_warn("%s: I/O error, deactivating speakup\n", synth->long_name);
/* No synth any more, so nobody will restart TTYs, and we thus
* need to do it ourselves. Now that there is no synth we can
* let application flood anyway
*/
synth->alive = 0;
speakup_start_ttys();
return 0;
}
return 1;
}
return 0;
}

BTW, I can see that spk_serial_synth_probe uses outb too, it should be
moved to serialio.c too. In the end, we should have a situation where
only serialio.c functions use in[bwl]/out[bwl], and these functions be
prefixed with spk_serial_, so that it's easy to know which code parts
still use them.

Samuel
Okash Khawaja
2017-02-20 13:10:54 UTC
Permalink
Hi,

On Mon, Feb 20, 2017 at 12:57 PM, Samuel Thibault
Post by Samuel Thibault
Hello,
Post by Okash Khawaja
Post by Samuel Thibault
Post by Okash Khawaja
Some drivers like apollo and bns only assign synth_immediate method in
their spk_synth structs but don't seem to use it. For those drivers,
can it be set to NULL?
I guess this method could be useful for some screen reading features,
just not used yet.
I see. Now because they are assigned but not used, they may cause confusion. For example, spk_synth_immediate uses outb.
Oh, I didn't realize this.
Post by Okash Khawaja
When migrating a synth to ttyio, if synth_immediate is left assigned, it may look like that synth isn't fully migrated yet.
Indeed, and that can only lead to problems later.
Post by Okash Khawaja
We can leave as it is although I would prefer it to be set to NULL
until it is actually used.
Indeed.
Ideally however we'd have a ttyio variant to assign here. It shouldn't
const char *spk_ttyio_synth_immediate(struct spk_synth *synth, const char *buff)
{
u_char ch;
while ((ch = *buff)) {
if (ch == '\n')
ch = synth->procspeech;
if (tty_write_room(speakup_tty) < 1 || !spk_ttyio_out(ch))
return buff;
buff++;
}
return NULL;
}
I've already done this for speakup_acntsa.
Post by Samuel Thibault
And the existing spk_synth_immediate should be renamed to
spk_serial_synth_immediate to make it clear it uses I/O ports.
Sure, and I'll also move it into serialio.c from your comments below.
Post by Samuel Thibault
Thinking about this, spk_ttyio_out should check the result of the write
Good point. Will update the code.
Post by Samuel Thibault
int spk_ttyio_out(const char ch)
{
if (synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
if (ret == 0)
/* No room */
return 0;
if (ret < 0) {
pr_warn("%s: I/O error, deactivating speakup\n", synth->long_name);
/* No synth any more, so nobody will restart TTYs, and we thus
* need to do it ourselves. Now that there is no synth we can
* let application flood anyway
*/
synth->alive = 0;
speakup_start_ttys();
return 0;
}
return 1;
}
return 0;
}
BTW, I can see that spk_serial_synth_probe uses outb too, it should be
moved to serialio.c too. In the end, we should have a situation where
only serialio.c functions use in[bwl]/out[bwl], and these functions be
prefixed with spk_serial_, so that it's easy to know which code parts
still use them.
Cool, will send in the patch set.

Okash
Okash Khawaja
2017-02-20 21:49:31 UTC
Permalink
Post by Okash Khawaja
Hi,
On Mon, Feb 20, 2017 at 12:57 PM, Samuel Thibault
Post by Samuel Thibault
Hello,
Post by Okash Khawaja
Post by Samuel Thibault
Post by Okash Khawaja
Some drivers like apollo and bns only assign synth_immediate method in
their spk_synth structs but don't seem to use it. For those drivers,
can it be set to NULL?
I guess this method could be useful for some screen reading features,
just not used yet.
I see. Now because they are assigned but not used, they may cause confusion. For example, spk_synth_immediate uses outb.
Oh, I didn't realize this.
Post by Okash Khawaja
When migrating a synth to ttyio, if synth_immediate is left assigned, it may look like that synth isn't fully migrated yet.
Indeed, and that can only lead to problems later.
Post by Okash Khawaja
We can leave as it is although I would prefer it to be set to NULL
until it is actually used.
Indeed.
Ideally however we'd have a ttyio variant to assign here. It shouldn't
const char *spk_ttyio_synth_immediate(struct spk_synth *synth, const char *buff)
{
u_char ch;
while ((ch = *buff)) {
if (ch == '\n')
ch = synth->procspeech;
if (tty_write_room(speakup_tty) < 1 || !spk_ttyio_out(ch))
return buff;
buff++;
}
return NULL;
}
I've already done this for speakup_acntsa.
Post by Samuel Thibault
And the existing spk_synth_immediate should be renamed to
spk_serial_synth_immediate to make it clear it uses I/O ports.
Sure, and I'll also move it into serialio.c from your comments below.
Post by Samuel Thibault
Thinking about this, spk_ttyio_out should check the result of the write
Good point. Will update the code.
Post by Samuel Thibault
int spk_ttyio_out(const char ch)
{
if (synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
if (ret == 0)
/* No room */
return 0;
if (ret < 0) {
pr_warn("%s: I/O error, deactivating speakup\n", synth->long_name);
/* No synth any more, so nobody will restart TTYs, and we thus
* need to do it ourselves. Now that there is no synth we can
* let application flood anyway
*/
synth->alive = 0;
speakup_start_ttys();
return 0;
}
return 1;
}
return 0;
}
On second thought, would you say it's better to separate out the check
for synth and restarting TTYs from spk_ttyio_out()? How about keeping
spk_ttyio_out() responsible for calling speakup_tty->ops->write()
only, and then choose one of the following two options:
a) create a separate function, say spk_ttio_check_and_out() (or
something better), which do these checks and restarts TTYs if
necessary
b) putting these checks and the restart of TTYs inside
spk_ttyio_immediate() for now and then when migrating other synths,
refactor these checks (or not) accordingly.

Keeping spk_ttio_out() as it is also means that it mirrors
spk_serial_out behaviour more closely while having few side effects,
i.e. restarting of TTYs.
Post by Okash Khawaja
Post by Samuel Thibault
BTW, I can see that spk_serial_synth_probe uses outb too, it should be
moved to serialio.c too. In the end, we should have a situation where
only serialio.c functions use in[bwl]/out[bwl], and these functions be
prefixed with spk_serial_, so that it's easy to know which code parts
still use them.
Cool, will send in the patch set.
Okash
Samuel Thibault
2017-02-20 21:52:12 UTC
Permalink
Post by Okash Khawaja
Keeping spk_ttio_out() as it is also means that it mirrors
spk_serial_out behaviour more closely while having few side effects,
i.e. restarting of TTYs.
Actually, spk_serial_out does have the side effect of restarting TTYs
etc. on error, that's done by the spk_wait_for_xmitr() call. That's
actually one of the reasons why I was thinking that spk_ttyio_out should
be doing it.

Samuel
Okash Khawaja
2017-02-20 22:01:12 UTC
Permalink
Post by Samuel Thibault
Post by Okash Khawaja
Keeping spk_ttio_out() as it is also means that it mirrors
spk_serial_out behaviour more closely while having few side effects,
i.e. restarting of TTYs.
Actually, spk_serial_out does have the side effect of restarting TTYs
etc. on error, that's done by the spk_wait_for_xmitr() call. That's
actually one of the reasons why I was thinking that spk_ttyio_out should
be doing it.
Ah okay. Right in that case it's actually better now because that side effect will at least be explicit.

Thanks
Post by Samuel Thibault
Samuel
Okash Khawaja
2017-02-21 16:53:15 UTC
Permalink
Post by Okash Khawaja
Post by Samuel Thibault
Post by Okash Khawaja
Keeping spk_ttio_out() as it is also means that it mirrors
spk_serial_out behaviour more closely while having few side effects,
i.e. restarting of TTYs.
Actually, spk_serial_out does have the side effect of restarting TTYs
etc. on error, that's done by the spk_wait_for_xmitr() call. That's
actually one of the reasons why I was thinking that spk_ttyio_out should
be doing it.
Ah okay. Right in that case it's actually better now because that side effect will at least be explicit.
Thanks
Post by Samuel Thibault
Samuel
Should spk_ttyio_out() also take in synth instance as its first
argument? That will ofcourse mean changing signature of serial_out
method inside spk_synth struct.

Currently it uses a global synth instance which gets asigned in
do_synth_init() which is fine but leaves one wondering where it came
from.
Samuel Thibault
2017-02-21 16:56:01 UTC
Permalink
Post by Okash Khawaja
Should spk_ttyio_out() also take in synth instance as its first
argument?
That'd be better, yes.

Samuel

Loading...