Unused synth_immediate method

[Okash Khawaja]

On Mon, Feb 20, 2017 at 1:10 PM, Okash Khawaja <okash.khawaja at gmail.com> wrote:
> Hi,
>
> On Mon, Feb 20, 2017 at 12:57 PM, Samuel Thibault
> <samuel.thibault at ens-lyon.org> wrote:
>> Hello,
>>
>> Okash Khawaja, on lun. 20 févr. 2017 12:23:45 +0000, wrote:
>>> > On 19 Feb 2017, at 23:06, Samuel Thibault <samuel.thibault at ens-lyon.org> wrote:
>>> >
>>> > Okash Khawaja, on dim. 19 févr. 2017 10:44:36 +0000, wrote:
>>> >> 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.
>>
>>> 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.
>>
>>> 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;
>> }
>
> I've already done this for speakup_acntsa.
>
>>
>> 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.
>>
>> Thinking about this, spk_ttyio_out should check the result of the write
>> operation, something like this:
>
> Good point. Will update the code.
>>
>> 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.

>>
>> 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