Bug 56318

Yep, its a synth hal problem. hal_diag_write_char buffers up charactors until
either a \n or 128 charactors are buffered. It then make one system call to
write out the buffer and a second one to flush it. 

There does not appear to be a way for libc to cause this buffer to be flushed.

This may be a more generic bug in the libc interface to the HAL serial drivers.
We may need to add a HAL level serial flush macro to make this work.

An alternative would be to flush the output in the synthetic target after every
character.

I'll leave it to bart to decide what is best here. I'm not really sure why it
was sent to me.

The steps to do this would be pretty easy. libc already calls cyg_io_get_config
with the CYG_IO_GET_CONFIG_SERIAL_OUTPUT_DRAIN key (or fsync() which for "devfs"
calls cyg_io_get_config the same way). So nothing is required there in libc.

The only difficulty is passing this request into the haldiag pseudo driver in
io/serial/current/src/common/haldiag.c. Once that is solved we could just
provide a macro like HAL_DIAG_FLUSH_OUTPUT in hal_diag.h.

But I'm not sure of the best way to do that because serial drivers are only
considered to have set_config, not get_config functions. I think maybe it would
be best just to flush the output for every character in the synth target. I
agree with Nick that Bart can decide. I just wanted to clarify the structure a
bit.

Oh, and Nick, you were the lucky winner because bugzilla insists on an initial
owner, so you own the entire HAL component in bugzilla. But don't worry too much
- everyone else is CC'd.

This is only temporary - we'll be changing bugzilla to send to mailing lists
instead "soon". And hopefully eventually have different owners for different HAL
architectures.

System calls are not that cheap, so I am not particularly keen on the idea of
doing a system call for every single character. This is especially true
if you have a system which does lots of output, e.g. a configuration with
tracing enabled and unbuffered. Some way of getting fflush() down to
the HAL would seem preferable.

I do not understand the exact route by which libc operations get down to
hal_diag_write_char(), but looking at some other code I see references to
DIAG_DEVICE_END_SYNC() and diag_device_end_sync(). Are these relevant somehow,
i.e. are there optional functions which the synthetic target HAL could
provide to give the desired behaviour?

I don't think they are relevant, no. The flush happens within libc stdio by it
calling cyg_stdio_flush in include/io.inl. That in turn either calls:

cyg_io_get_config(dev,
                             CYG_IO_GET_CONFIG_SERIAL_OUTPUT_DRAIN,
                             NULL, NULL);
or if we have CYGPKG_IO_FILEIO, fsync( dev ), which deeper down in fileio's
devfs calls the exact same cyg_io_get_config line above.

That cyg_io_get_config will be interpreted by the code in
io/serial/current/src/common/serial.c which _only_ drains the serial buffer at
that level. It does not contact any lower layer.

Maybe despite the naming oddity, we should just add a
CYG_IO_SET_CONFIG_HW_SERIAL_OUTPUT_DRAIN config key, which is passed into the
lower level driver (and the error code is ignored because most drivers won't
support it). So haldiag_set_config in io/serial/current/src/common/haldiag.c can
act on it and call a HAL_DIAG_FLUSH_OUTPUT macro if defined.

Note that this layer goes straight to haldiag - not via infra's diag.

I believe the DIAG_DEVICE_START/END_SYNC stuff was a mutual exclusion thing -
i.e. not related to this.

Soon to be attached is a patch which seems to work. I followed what Jifl suggested.

I have some questions

1) In serial.c: serial_get_config i do the call down into the next layer to
cause the flush without the out_cbuf lock. I don't think i need it, since im not
doing anything with the buffer. Im flushing the buffer at the next level down,
not this level which has already been drained. Is this correct?

2) In config_keys.h:  From the naming convention, it looks like the new key is
in a catagory of its own. Is this correct or do you want to make it part of the
0x100 serial catagory?

3) There is a hole in the current key numbers. 0x500 is not used so i've used it
for the HW keys. Was this hole deliberate? Are there some other keys somewhere i
don't know about which are also using 0x500.

4) Do you know of any pre-compiled win32 ser_filter.exe? I cannot get it to compile.

I've tested this on synth and ebsa285. My test case works now, but without
ser_filter i cannot run many of the serial tests.

Created attachment 75264 [details]
Proposed solution

The patch looks pretty good.

To answer your questions:

1) yes that's correct. if the lower layer needs locking, then it has to handle
that itself.

2) The name appears to be in a category of its own just because of my
imprecision when I wrote the earlier comments. It should be called
CYG_IO_SET_CONFIG_SERIAL_HW_OUTPUT_DRAIN and go in the 0x100 range.

3) This is irrelevant now, but OOI 0x500 truly is free - it's not used in the RH
codebase either.

4) If you've been delivered an eCos release for windows, there should be one in
the tools/bin directory. Otherwise I can mail one to you offline. Let me know.

Some minor comments:

o Can you make HAL_DIAG_FLUSH_OUTPUT be HAL_DIAG_FLUSH_OUTPUT() for consistency?

o Dare I mention SGML documentation? ;) For both HAL_DIAG_FLUSH_OUTPUT and the
new config key.

o Some spurious whitespace added at the end of config_keys.h (nitpick factor 9
engage ;-)).

o the ifdef in io/serial/current/src/common/haldiag.c should be around the whole
CYG_IO_SET_CONFIG_HW_SERIAL_OUTPUT_DRAIN case I think. Just that the return
value in serial.c is always ignored.

Where should be new key documentation go? 

This is really the next level down from that documented in io.sgml, section
io-serial-cyg-set-config.

io.sgml is fine - the other serial keys are in there.

Just say it's to indicate to the underlying hardware device that it should drain
its output. -EINVAL is returned if the hardware is not capable of this, e.g. if
it is unbuffered.

If you prefer, I can update it. But it's pretty easy - just cut-n-paste the
existing CYG_IO_GET_CONFIG_SERIAL_OUTPUT_DRAIN entry.

Created attachment 75284 [details]
Patch version 2

Upps. Forgot the ChangeLog Entries.....I will do that now.

This bug has moved to http://bugs.ecos.sourceware.org/show_bug.cgi?id=56318