glasgow icon indicating copy to clipboard operation
glasgow copied to clipboard

Documentation: REPL: suggestions for UART example

Open ewenmcneill opened this issue 1 year ago • 1 comments

(I started writing this as a comment on https://github.com/GlasgowEmbedded/glasgow/issues/312 but it got long, so I've made it its own issue)

Re https://glasgow-embedded.org/latest/use/repl-script.html#uart which i had a look at because Attie mentioned it in today's meeting, experimenting with it, it appears that the default iface.read() will read "whatever is available, possibly nothing": see https://github.com/GlasgowEmbedded/glasgow/blob/05c44bd0ff7fc051c08a942ed6c18cf0b0f8de20/software/glasgow/access/direct/demultiplexer.py#L227-L241 with the default length=None. Which means as presented the example is a bit racey (I started looking into this as on my second write/read cycle in a run I got a partial return, even with "types by hand" delays, and while experimenting I got an empty byte string even after writing bytes).

It might be worth adding a note to the UART REPL example in the documentation to the effect that iface.read() will return "whatever is immediately available, which might not be the whole message as shown in the example". Or perhaps explicitly calling iface.read(length=6) to force reading all bytes expected to be returned for more user-reproducibility. (FTR, I wasn't that surprised by the short read, given it's a byte oriented serial line; but I was a bit surprised by having an iface.read() finish returning a zero length byte string when I had written something.)

Relatedly, help(iface) doesn't include any description of how the length parameter is interpreted on read(), which might be worth adding. Eg, "if length is omitted or None returns data immediately available; otherwise waits until required number of bytes have been received". Which I think would be a doc string on glasglow.access.direct.demultiplexer.DirectDemultiplexerInterface.read() since AFAICT that's what the REPL user is encouraged to use for interactive help:

ewen@parthenon:~$ glasgow repl uart -V 3.3
I: g.device.hardware: device already has bitstream ID d45fa78180875415078a05fcec3dd124
I: g.cli: running handler for applet 'uart'
I: g.applet.interface.uart: port(s) A, B voltage set to 3.3 V
I: g.applet.interface.uart: dropping to REPL; use 'help(iface)' to see available APIs
>>> type(iface)
<class 'glasgow.access.direct.demultiplexer.DirectDemultiplexerInterface'>
>>> 

ETA: Example of the "zero bytes read" read (in this case on the same command line, to make it faster to reproduce; but I did also get this with "up arrow, run again" one command per command line earlier):

UART REPL example showing some racey behaviour
>>> await iface.write(b'hello!')
>>> await iface.read()
<memory at 0x7fc93eac31c0>
>>> bytes(_)
b'hello!'
>>> await iface.write(b'hello!')
>>> await iface.read()
<memory at 0x7fc93eac3400>
>>> bytes(_)
b'hello!'
>>> await iface.write(b'hello!'); await iface.read(); bytes(_)
<memory at 0x7fc93eb5cd00>
b''
>>> await iface.write(b'hello!'); await iface.read(); bytes(_)
<memory at 0x7fc93eac3c40>
b'hello!'
>>> await iface.write(b'hello!'); await iface.read(); bytes(_)
<memory at 0x7fc93ea9f100>
b'hello!'
>>> await iface.read()
<memory at 0x7fc93ea9f4c0>
>>> bytes(_)
b'hello!'
>>> 

(Note also the final iface.read() which caught up on the outstanding message.)

Example of partial message reads, including parts of two messages
>>> await iface.write(b'Hello World!')
>>> await iface.read()
<memory at 0x7fc93eac7400>
>>> bytes(_)
b'Hello World!'
>>> await iface.write(b'Hello World!')
>>> await iface.read()
<memory at 0x7fc93ea9fc40>
>>> bytes(_)
b'He'
>>> await iface.write(b'Hello World!')
>>> await iface.read()
<memory at 0x7fc93eacba00>
>>> bytes(_)
b'llo World!Hello Wo'
>>> await iface.read()
<memory at 0x7fc93eac71c0>
>>> bytes(_)
b'rld!'
>>> 

(FTR, all of this is expected, but IMHO should be documented as expected to avoid user surprise.)

ewenmcneill avatar Oct 22 '23 00:10 ewenmcneill

The UART should simply not return a DirectDemultiplexerInterface directly. In our chat I have called that out as incorrect and we should fix this instead of messing with documentation.

whitequark avatar Oct 22 '23 09:10 whitequark