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

Overhaul the termios API. #615

Merged
merged 1 commit into from
Jun 19, 2023
Merged

Overhaul the termios API. #615

merged 1 commit into from
Jun 19, 2023

Conversation

sunfishcode
Copy link
Member

Instead of defining Termios as an alias for the libc type, define our own type with a libc-compatible layout, so that we can use bitflags flags types, and have better overall ergonomics.

@sunfishcode sunfishcode changed the title Overall the termios API. Overhaul the termios API. Apr 20, 2023
@sunfishcode sunfishcode force-pushed the sunfishcode/new-termios-api branch 2 times, most recently from 5d54930 to db04499 Compare April 20, 2023 04:14
@sunfishcode sunfishcode mentioned this pull request Apr 21, 2023
17 tasks
@sunfishcode sunfishcode added the semver bump Issues that will require a semver-incompatible fix label May 4, 2023
@sunfishcode
Copy link
Member Author

I've now rebased and updated this.

And I've also added a major change: This now encapsulates the termios vs. termios2 difference on Linux, and supports arbitrary I/O speeds on both Linux and BSDs with a consistent interface.

This is a little higher-level than rustix typically aims for, but it doesn't incur extra syscalls or even extra Termios struct copies, and the things it hides are both awkward and uninteresting, such as the whole termios vs. termios2 split which only happens on Linux, where you have to use termios2 to support arbitrary speeds but you can't use termios2 with cfmakeraw etc., and also c_ispeed/c_ospeed work differently in termios2. And PowerPC lacks termios2 but has a termios that acts like termios2. And MIPS adds TCSETS to its TCSANOW etc. values.

@sunfishcode sunfishcode force-pushed the sunfishcode/new-termios-api branch 2 times, most recently from 536fc0c to 04776f6 Compare June 14, 2023 15:15
@sunfishcode sunfishcode marked this pull request as ready for review June 14, 2023 15:28
@sunfishcode sunfishcode force-pushed the sunfishcode/new-termios-api branch 11 times, most recently from 9cc626d to eabb9b6 Compare June 15, 2023 00:58
@sunfishcode
Copy link
Member Author

@SUPERCILEX This PR unifies tcgetattr and tcgetattr2 into a single framework that supports arbitrary speeds. I'd be interested to hear if this would work for your use case that motivated adding tcgetattr2 in the first place.

@SUPERCILEX
Copy link
Contributor

My use case is literally just this lol: https://github.com/SUPERCILEX/hardcaml_riscv/blob/595a0992b3afac011a540daf9db0c1c4c3cc3b4c/soft/offchip/tstp/src/main.rs#L39

So I think this PR is good—can't test it though as I don't have my FPGA with me.

One thought: maybe it makes sense to have a set_speed method that only does the bit manipulation once?

@sunfishcode sunfishcode force-pushed the sunfishcode/new-termios-api branch 2 times, most recently from a05280f to 74d07c9 Compare June 15, 2023 23:35
@sunfishcode
Copy link
Member Author

Sounds good. I've now implemented your idea of a set_speed function that sets both input and output, so your use case could look like this:

        let mut termios = tcgetattr(&device).unwrap();
        termios.set_speed(baud_rate);
        tcsetattr(&device, OptionalActions::Drain, &termios).unwrap();

This API takes care of the CBAUD and BOTHER parts for you.

So again, this is more than rustix would typically abstract away, and I'm still debating it, but there are so many portability hazards between OS's and architectures that I'm leaning towards it.

@SUPERCILEX
Copy link
Contributor

This is awesome, thanks!

@sunfishcode
Copy link
Member Author

This PR turns out to encounter what turned out to be some bugs in bitflags around the handling of unnamed bits. I've now added some workarounds to get the tests passing, but this will need some further planning.

Instead of defining `Termios` as a plain alias for the libc `termios` type,
define our own `Termios` struct with a libc-compatible layout, so that we
can use `bitflags` flags types, and have better overall ergonomics.

This encapsulates the `termios` vs. `termios2` difference on Linux, and
supports arbitrary I/O speeds on both Linux and BSDs with a consistent
interface.

This is a little higher-level than rustix typically aims for, but it doesn't
incur extra syscalls or even extra `Termios` struct copies, and the things it
hides are both awkward and uninteresting, such as the whole termios vs.
termios2 split which only happens on Linux, where you have to use termios2 to
support arbitrary speeds but you can't use termios2 with `cfmakeraw` etc.,
and also `c_ispeed`/`c_ospeed` work differently in termios2. And PowerPC
lacks termios2 but has a termios that acts like termios2. And MIPS adds
`TCSETS` to its `TCSANOW` etc. values.
@sunfishcode
Copy link
Member Author

I've now filed #695 to track the workarounds.

@sunfishcode sunfishcode merged commit 58152e9 into main Jun 19, 2023
52 checks passed
@sunfishcode sunfishcode deleted the sunfishcode/new-termios-api branch June 19, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver bump Issues that will require a semver-incompatible fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants