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

ports/unix: Add full uos.dupterm support. #6080

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andrewleech
Copy link
Sponsor Contributor

The unix port currently supports a very limited uos.dupterm implementation.
It can only replace stdin and/or duplicate stdout.
When using uos.dupterm to replace slot 0 it does not return a handle to the existing (posix stdio) stream.

This PR resolves these issues, preferring to use dupterm for all stdio on unix when MICROPY_PY_OS_DUPTERM=1 is configured in the build.

With this change simple modification of the output stream is possible, for example to format logging messaging with a basic timestamp

stdio = None

class logger:
    def write(self, buf):
        global stdio        
        stdio.write(str(utime.time()))
        stdio.write(" | ")
        stdio.write(buf)

stdio = uos.dupterm(logger(), 0)

@@ -103,3 +103,7 @@ enum {

void mp_hal_get_mac(int idx, uint8_t buf[6]);
#endif

#if MICROPY_PY_OS_DUPTERM
void init_dupterm_stdio();
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have mp_ prefix since it is in a header file. Or even better, mp_unix_ since it is specific to the unix port.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was loath to change the name of an existing file, but I too thought it was oddly inconsistent in name.

@@ -64,8 +66,11 @@ STATIC void sighandler(int signum) {
}
#endif

int mp_interrupt_char = -1;
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nice to have a comment that explains this is the mp_interrupt_char from lib/utils/interupt_char.h

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Yes I had to add this just to allow compilation with MICROPY_PY_OS_DUPTERM enabled.

MP_STATE_VM(dupterm_objs[idx]) = save_term;
STATIC mp_uint_t unix_stdio_read(mp_obj_t self_in, void *buf, mp_uint_t size, int *errcode) {
ssize_t ret;
MP_HAL_RETRY_SYSCALL(ret, read(STDIN_FILENO, (byte *)buf, size), {});
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be handling the error? Otherwise we end up returning -1 at the end of the function without setting *errorcode

STATIC mp_uint_t unix_stdio_read(mp_obj_t self_in, void *buf, mp_uint_t size, int *errcode) {
ssize_t ret;
MP_HAL_RETRY_SYSCALL(ret, read(STDIN_FILENO, (byte *)buf, size), {});
if (ret == 0) {
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought a return value of 0 meant that we have reached EOF rather than non-blocking. Is this a special case for if stdin is closed but other slots are still open?


STATIC mp_uint_t unix_stdio_write(mp_obj_t self_in, const void *buf, mp_uint_t size, int *errcode) {
int ret;
MP_HAL_RETRY_SYSCALL(ret, write(STDOUT_FILENO, (const byte *)buf, size), {});
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here with not handling the error

@dpgeorge
Copy link
Member

On CPython I guess you can hook into stdout by redefining sys.stdout... and maybe that's how MicroPython should have done it from the start, instead of adding uos.dupterm() (although it'd still need an efficient multiplexer, eg sys.stdout = micropython.stream_multiplex(sys.stdout, machine.UART(...)), not to mention handling sys.stdin).

Anyway, the way it is now sys.stdout is before the dupterm multiplexer, so writing to sys.stdout goes to all dupterm streams (similar for reading). And what this PR does is adds raw-stdout/stdin objects, ie they do not go via dupterm. On bare-metal the equivalent would be a UART or USB_VCP object.

It just seems like there's a lot of complexity here with two different kinds of stdout/stdin objects (a dupterm one and a raw one). But I don't really see a way around that.

Would it be possible to reuse the extmod/vfs_posix_file.c code to implement the raw stdin/stdout? That code has compile-time dupterm support but maybe that dupterm support can be made optional at runtime (via a flag in the stdio object) so that the same code can be used for both sets of stdio objects?

@andrewleech
Copy link
Sponsor Contributor Author

Thanks for the pointer towards extmod/vfs_posix_file.c - it already has the stdio object instantiated

const mp_obj_vfs_posix_file_t mp_sys_stdin_obj = {{&mp_type_textio}, STDIN_FILENO};
const mp_obj_vfs_posix_file_t mp_sys_stdout_obj = {{&mp_type_textio}, STDOUT_FILENO};
const mp_obj_vfs_posix_file_t mp_sys_stderr_obj = {{&mp_type_textio}, STDERR_FILENO};

I should be able to clean up this patch reusing them.

@andrewleech
Copy link
Sponsor Contributor Author

andrewleech commented May 28, 2020

@dpgeorge there's a catch with using the mp_sys_stdin_obj etc objects above as i/o for dupterm, other things read/write to these objects - and writes to stdout/stderr are redirected to dupterm (hello recursion).

I've resolved this by adding a relatively small amount of extra logic to make a new const mp_obj_vfs_posix_file_t mp_sys_raw_stdio_obj which will automatically read/write to the correct fd and not go via dupterm - this new raw_stdio can be used for unix_mphal.c both with and without dupterm.

It feels like a bit of a tacked-on-the-side addition, but it does make for less duplication than I had previously.

Now I'm also getting a lot of travis fails, for one the minimal port doesn't have the new file handle created, presumably due to a lack of one/both of MICROPY_VFS_POSIX / MICROPY_VFS_POSIX_FILE
Some of the others are failing a number of unit tests, don't know why yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants