Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys.stdio: add support for ioctl and make sys.stdin.buffer.read non-blocking #4859

Closed
wants to merge 3 commits into from

Conversation

dpgeorge
Copy link
Member

This PR allows to poll sys.stdin for readability, and also makes sys.stdin.buffer.read non-blocking (but keeps sys.stdin.read blocking as it was before). A port must provide mp_hal_stdin_rx_avail() for this to work (this function is only implemented on esp8266 for now).

Testing of polling can be done with:

import sys, select
while True:
    res = select.select([sys.stdin.buffer], [], [])
    data = sys.stdin.buffer.read(1)
    print('got', data)
    if data == b'q':
        break

Testing of non-blocking read can be done with (enter chars while it's sleeping):

import time, sys
time.sleep(3); print(sys.stdin.buffer.read(4)

See issue #4849 for background.

@pfalcon
Copy link
Contributor

pfalcon commented Jun 19, 2019

and also makes sys.stdin.buffer.read non-blocking

On what this is based? It's hard to believe that CPython has non-blocking sys.stdin.buffer. Not only it doesn't, in my quick testing (n 3.6.7), it doesn't even issue short reads, i.e. running sys.stdin.buffer.read(4) and typing just 1\n won't return, waiting for more chars (pressing Enter at the end of each line is of course required due to default cooked/line-based mode of terminal).

@dpgeorge
Copy link
Member Author

On what this is based? It's hard to believe that CPython has non-blocking sys.stdin.buffer

See my comment #4849 (comment)

sys.stdin reading on bare-metal is already different to CPython. Non-blocking behaviour is arguably more useful than blocking, and since sys.stdin.buffer is most likely going to be used for raw reads I changed this here to non-blocking, but not sys.stdin.

@robert-hh
Copy link
Contributor

If a method exists to check, whether sys.stdin.read() would be successful, the non-blocking read may not be needed. That method is given now with select. That's good.
I imagine that you want to make sys.stdin.buffer.read() similar to uart.read(), which was made non-blocking recently. But uart.read() always could return None in case of a timeout, so the difference is minor: non-blocking read == read with timeout 0.

@pfalcon
Copy link
Contributor

pfalcon commented Jun 21, 2019

Non-blocking behaviour is arguably more useful than blocking, and since sys.stdin.buffer is most likely going to be used for raw reads I changed this here to non-blocking, but not sys.stdin.

As someone who likes to do adjustments, simplifications, and extensions to CPython I/O model, as well as the original designer of MicroPython's streams subsystem, I don't see how "non-blocking behaviour is arguably more useful than blocking". Blocking behavior is definitely more useful than non-blocking, because non-blocking mode for any non-toy usage requires I/O scheduling (aka answering question "what to do if there's EAGAIN condition"). That's why CPython has it blocking, following the same default for POSIX.

It however should be possible to enable non-blocking mode for any stream which can support it. There's stream.setblocking() method for this. MicroPython streams also cater for "no short reads/writes" behavior, but if for some type of stream (e.g. UART streams) short reads would make sense, there're provisions for such (there're just no current usages in the codebase).

Beyond that, there're a number of issues and unfinished things in MicroPython stream subsystem:

  • sys.stdin/sys.stdout are (should be) just variables and should support assignment. In this regard some adhoc stream object can be assigned to it. The defaults should still be compatible with CPython (which are sane overall).
  • In CPython, sys.stdin object is of type _io.TextIOWrapper, which is adapter which takes byte stream and reads out unicode characters out of it. In MicroPython, _io.TextIOWrapper is a completely adhoc object, that needs to be fixed.
  • In CPython, sys.stdin.buffer is nothing but .buffer attribute of _io.TextIOWrapper object, value of which is _io.BufferedReader object, which is adapter which takes raw stream and adds read buffering to it. This class is missing in MicroPython and needs to be implemented.
  • In CPython, sys.stdin.buffer.raw us just .raw attribute of _io.BufferedReader object, whose value is underlying raw stream.

Again, perhaps some levels above might be superfluous for small baremetal ports, but doing things like "sys.stdin is blocking, while "sys.stdin.buffer is non-blocking", ignoring theie meaning and interaction of the things as described above - that's complete adhoc mess, just for the purpose of doing something #4849. And noticeable further deterioration of design quality, already started by #4020, where, instead of starting with generic improvements to stream interface (with a whole bunch having been proposed), just adhoc I2C-specific changes were made.

As for #4849, it's rather separate issue. It's a bit too optimistic to assume in the first place that one case use the same stream for REPL, and change its settings arbitrarily at the same time. I see that issue very similar to Unix port one, where adding something to dupterm breaks REPL. It would be very similar in Unix to setting terminal to e.g. raw mode behind bash's back - there should be no surprise that it stops to work as expected.

Fixing that issue would require to implement the I/O stream hierarchy above, and more than that, and of course per good tradition would start with Unix port.

@dpgeorge
Copy link
Member Author

I agree that making sys.stdin.buffer non-blocking is inconsistent. The commit here to do that was mainly added as a point for discussion in the context of #4849. I'm happy not to include this change.

But making sys.stdin pollable is I think a useful addition. I updated the PR to make it work with dupterm and a few of the other ports (not just esp8266).

A port must provide the following function for this to work:

    uintptr_t mp_hal_stdio_poll(uintptr_t poll_flags);
@dpgeorge
Copy link
Member Author

dpgeorge commented Jul 1, 2019

Merged in b7da67c, 964ae32 and c80614d
(dropping non-blocking change to sys.stdin.buffer).

@dpgeorge dpgeorge closed this Jul 1, 2019
@dpgeorge dpgeorge deleted the sys-stdio-ioctl branch July 1, 2019 07:12
@jimmo jimmo mentioned this pull request Aug 28, 2019
tannewt added a commit to tannewt/circuitpython that referenced this pull request Jun 23, 2021
Fix pulsein pause and resume functions to handle HCSR04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants