Skip to content

allow to set custom serial speed on Linux#80534

Merged
wfurt merged 4 commits intodotnet:mainfrom
wfurt:serial2
Jan 20, 2023
Merged

allow to set custom serial speed on Linux#80534
wfurt merged 4 commits intodotnet:mainfrom
wfurt:serial2

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Jan 12, 2023

asm/termbits.h conflicts with standard termios.h so I put the new code to separate file instead of usual #if XXX
I don't have enough HW to test actual transfer but at least I'm able to open port with custom speed.

fixes #80288

@wfurt wfurt added area-System.IO os-linux Linux OS (any supported distro) labels Jan 12, 2023
@wfurt wfurt requested review from janvorli and krwq January 12, 2023 03:57
@wfurt wfurt self-assigned this Jan 12, 2023
@ghost
Copy link

ghost commented Jan 12, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

asm/termbits.h conflicts with standard termios.h so I put the new code to separate file instead of usual #if XXX
I don't have enough HW to test actual transfer but at least I'm able to open port with custom speed.

fixes #80288

Author: wfurt
Assignees: wfurt
Labels:

area-System.IO, os-linux

Milestone: -

@janvorli
Copy link
Member

@wfurt the MUSL legs are failing due to missing TCGETS2.

@wfurt
Copy link
Member Author

wfurt commented Jan 12, 2023

I'll take a look. If it is only because of missing Ioctl name it would be easy to fix.

return 0;
}
"
HAVE_TERMIOS2)
Copy link
Member

Choose a reason for hiding this comment

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

do we know which rids will not have this defined?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we also need to update the eng/native/tryrun.cmake for cross build to get this definition. The live detection in configure.cmake is only for non-cross builds. So we need to get the result of this check on all our cross target platforms (by running the build is running natively there) and update the tryrun.cmake accordingly in this PR.
@krwq that should give an answer to your question then.

Copy link
Member Author

Choose a reason for hiding this comment

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

According to https://man7.org/linux/man-pages/man2/ioctl_tty.2.html this was added in 2.6.20 Kernel e.g. it should be available in all Linux versions we support. (and is Linux specific) I saw note that it may not work on architectures but I don't know any details. The definition lives in asm-generic/termbits.h so I expect it would compile everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then we can just add it to the eng/native/tryrun.cmake. Please note that in that file, you need to set the HAVE_XXX_EXITCODE, not HAVE_XXX and the exit code is 0 for success.
In other words, you'll need to add

set_cache_value(HAVE_TERMIOS2_EXITCODE 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we ever use it for OSes other than Linux? (Like FreeBSD ARM64)? I was looking for section with plain LINUX but aside from ALPINE I did not find good fit.

Copy link
Member

Choose a reason for hiding this comment

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

The section that is used for linux starts here:

set_cache_value(FILE_OPS_CHECK_FERROR_OF_PREVIOUS_CALL_EXITCODE 1)

It is used for all cross builds, that means including FreeBSD. Since the section for Linux is shared with FreeBSD, Illumos and Tizen, we need to know the value for those OSes. It seems that it would be as easy as cross building the repo for those three OSes with your change and the tryrun.cmake modified. If it was not supported, the build would fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I check FreeBSD headers and there is neither termios2 or TCGETS2. I suspect others would be the same.
Instead of setting negative value for all the other OSes, would it make sense to create new branch for Linux only?
And if do we already have some variable that we can use (CMAKE_SYSTEM_NAME, CLR_CMAKE_TARGET_OS, ..?) or would we need to invent something?

Copy link
Member

Choose a reason for hiding this comment

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

Since everything else is the same, I'd just make setting this single value conditional based on where it is supported.

set_cache_value(HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES_EXITCODE 0)
set_cache_value(HAVE_TERMIOS2_EXITCODE 1)
else()
set_cache_value(HAVE_TERMIOS2_EXITCODE 0)
Copy link
Member

Choose a reason for hiding this comment

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

nit: the current style is to add the "default" before the conditional block (line 103) and override the platform specific values where needed. This way we won't be needing this else() block.

@krwq krwq mentioned this pull request Jan 17, 2023
17 tasks
@wfurt wfurt merged commit 1cfa588 into dotnet:main Jan 20, 2023
@wfurt wfurt deleted the serial2 branch January 20, 2023 03:51
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
* allow to set custom serial speed on Linux

* fix compilation on alpine

* add entries for cross build
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Ports os-linux Linux OS (any supported distro)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linux: System.IO.Ports.SerialPort does not open with baudrate 111,865

6 participants