Attempt to enable native UART on modern systems #1750

jahudka · 2021-11-13T16:52:47Z

Both current Raspberry Pi OS and Ubuntu for the Pi are missing the stropts.h header file, which causes the OLA ExtendedSerial helper to be compiled to always return false from LinuxHelper::SetDmxBaud(). This in turn makes it impossible to use the native UART DMX output plugin. This PR is an attempt to fix that by including an alternative bundled header file which defines the ioctl() function, which is the only thing needed out of stropts.h.

I'd like to say up front that I'm not a C/C++ developer - in fact this is only the second time in my life that I'm attempting to mess around with C code - so please don't be too harsh with me if I've made newbie mistakes! As it is, the code compiles on Ubuntu 21.10 for RPi without errors and from the output of olad it is apparent that the issue is fixed by this workaround - that is, where previously the UART DMX plugin logged a warning about being unable to set the correct baud rate for the UART device, now the plugin outputs the expected line about the input and output rates being the expected 250000. I've tried to provide a "minimal" fix which would only be applied when stropts.h isn't present, so it should compile on other and / or older systems exactly as before, unless I'm mistaken.

Of course, the usual UART configuration still applies - you still need to modify /boot/firmware/config.txt to disable bluetooth via a dtoverlay, enable UART and set the appropriate UART clock, and disable the hciuart service - but even with all those things in place, neither the distribution ola package nor a fresh compile of OLA produced the expected result.

Thanks everyone on the OLA team for creating and maintaining this project!

peternewman

Hi @jahudka ,

Thanks very much for this, #1749 beat you to the bug report, but not the fix, so it's certainly appreciated!

Please can you target our 0.10 branch with this fix, as it should go into those patch releases too. You'll need to recommit against those or cherry-pick or similar as your branch has all the 0.10 to master changes in it already.

More generally this doesn't feel quite right, I'd assume it still exists in a file somewhere and we're just not loading it, we might need to do some more digging. My strpopts.h (on an older system) comes from libc6-dev.

I might generate another PR for you to test if you don't mind, to start off by us logging if this fails in future, rather than silently stopping.

peternewman · 2021-11-13T17:47:25Z

common/io/ExtendedSerial.cpp

+// this provides a minimal ioctl() definition for modern systems
+// which don't have <stropts.h> anymore
+#include <ola/io/IOCtl.h>


I think from a bit of reading Linux shouldn't necessarily have stropts.h (as it's a POSIX thing).

Yes, true, as I understand it, the Streams interface probably wasn't ever even really implemented, the header files were mostly present for decades before it was finally purged, I don't recall where but I read something along those lines in the past couple of days..

peternewman · 2021-11-13T17:48:03Z

common/io/ExtendedSerial.cpp

+#else
+// this provides a minimal ioctl() definition for modern systems
+// which don't have <stropts.h> anymore
+#include <ola/io/IOCtl.h>
 #endif  // HAVE_STROPTS_H

 #ifdef HAVE_ASM_TERMIOS_H


Which would suggest it's actually this bit that's breaking

I don't think it is - ./configure reports that both termios.h and asm/termios.h are present and usable, whereas stropts.h is not; and in any case, if it were this, then my "fix" wouldn't actually fix anything.. so unless something really non-obvious is happening I don't think the issue is here.

peternewman · 2021-11-13T17:48:31Z

include/ola/io/IOCtl.h

+#define INCLUDE_OLA_IO_IOCTL_H_
+
+extern "C" {
+	extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;


Is that definition in an existing header on your machine, if so which one?

No, I found the source of stropts.h here and extrapolated from there.

jahudka · 2021-11-13T18:03:42Z

Hi, wow, fast and thorough! I'll go ahead and remake this PR against the 0.10 branch (I'm assuming I can't simply update this PR, I'll have to create a new one, right?).

Regarding stropts.h, yes, it does come from libc6-dev, but it looks like it has been removed sometime before the release of Ubuntu 20.04, since that's where most people started reporting it missing, according to my research. Which would explain why it works on older systems.

peternewman · 2021-11-13T18:09:02Z

Hi, wow, fast and thorough!

The perils of notifications! And I'm actually free today... 😆

I'll go ahead and remake this PR against the 0.10 branch (I'm assuming I can't simply update this PR, I'll have to create a new one, right?).

Yeah sadly, a cherry-pick is the best I've managed, or you might be able to rebase or something (I don't think anything has actually changed in this file).

Regarding stropts.h, yes, it does come from libc6-dev, but it looks like it has been removed sometime before the release of Ubuntu 20.04, since that's where most people started reporting it missing, according to my research. Which would explain why it works on older systems.

Yeah, it still feels like that ioctl should be defined somewhere even now, if we can use it.

Would you also mind testing that this logs correctly on failure before you're patch (you'll need -l 3 or -l 4) #1751. Then we can either merge it as is, or you can pull it into your branch and adapt if we end up changing the includes requirements.

peternewman · 2021-11-13T18:11:21Z

Also please heed and fix the cpplint errors on Travis:

./include/ola/io/IOCtl.h:29:  #endif line should be "#endif  // INCLUDE_OLA_IO_IOCTL_H_"  [build/header_guard] [5]
./include/ola/io/IOCtl.h:26:  Tab found; better to use spaces  [whitespace/tab] [1]
./include/ola/io/IOCtl.h:26:  Extra space before ( in function call  [whitespace/parens] [4]
./include/ola/io/IOCtl.h:26:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]

jahudka · 2021-11-13T18:23:48Z

ioctl() is actually defined in sys/ioctl.h, but including it in place of stropts.h causes conflicts with the winsize and termio structs which I've been unable to resolve with my limited skills..

Regarding the logging: I've actually tried a lot of that when I was trying to get to the bottom of this - mostly exactly what you have in your PR, except in a somewhat.. sharper... language (I was getting slightly frustrated at that point lol). And the fun part is, I never actually got the log messages to display - I would update the code, run make and make install again, wait for an hour.. and then get nothing! I pulled a lot of my hair out last night. Anyway, I'll try a recompile from your PR later tonight to check if I get the expected missing stropts.h or termios2 message which should be evidence my theory is correct, and then I'll post an updated PR based on 0.10, taking care of the cpplint errors as well (sorry, I didn't really run it before pushing this PR, I'm doing all the compilation etc on a Pi 3 and everything takes forever, so I figured I'd have the CI do some of the work for me..).

jahudka · 2021-11-13T18:28:56Z

quick question re: ./include/ola/io/IOCtl.h:26: Use int16/int64/etc, rather than the C type long [runtime/int] [4] from cpplint: which type would be the correct one to use here? since even sys/ioctl.h defines the parameter as unsigned long, according to several sources.. sorry, I know this is probably one of the noobest C++ questions ever :-D

shenghaoyang · 2021-11-14T08:46:16Z

quick question re: ./include/ola/io/IOCtl.h:26: Use int16/int64/etc, rather than the C type long [runtime/int] [4] from cpplint: which type would be the correct one to use here? since even sys/ioctl.h defines the parameter as unsigned long, according to several sources.. sorry, I know this is probably one of the noobest C++ questions ever :-D

You should stick with ulong. The syscall wrapper in libc is expecting a ulong when it's called, so using any other fixed-width type will cause issues on systems where ulong != the type used.

In any case using asm/termbits.h & asm/ioctls.h allows use of sys/ioctl.h without any conflicts (at least on my system):

#include <asm/ioctls.h>
#include <asm/termbits.h>
#include <sys/ioctl.h>

int set_baud(int fd) {
    const speed_t rate = 250000u;
    termios2 tio;

    if (ioctl(fd, TCGETS2, &tio) == -1)
        return -1;

    tio.c_cflag &= ~CBAUD;
    tio.c_cflag |= BOTHER;
    tio.c_ispeed = rate;
    tio.c_ospeed = rate;

    if (ioctl(fd, TCSETS2, &tio) == -1)
        return -1;

    return 0;
}

Other ideas are discussed at https://stackoverflow.com/questions/37710525/including-termios-h-and-asm-termios-h-in-the-same-project - if all else fails #undef hacks could be used.

jahudka · 2021-11-15T11:05:20Z

@peternewman I've finally successfully completed a build of your PR with the debug code - the pertinent section of the output looks like this:

olad/PluginManager.cpp:195: Trying to start UART native DMX
plugins/uartdmx/UartDmxPlugin.cpp:67: Trying to open UART device /dev/ttyAMA0
plugins/uartdmx/UartWidget.cpp:70: Opening serial port /dev/ttyAMA0
plugins/uartdmx/UartWidget.cpp:76: Opened serial port /dev/ttyAMA0
common/io/ExtendedSerial.cpp:84: Failed to set baud rate, due to missing stropts.h or termios2
plugins/uartdmx/UartWidget.cpp:180: Failed to set baud rate to 250k
plugins/uartdmx/UartDmxPlugin.cpp:82: Unable to setup device for output, device ignored /dev/ttyAMA0
olad/PluginManager.cpp:200: Started UART native DMX

@shenghaoyang I've seen the discussion about #undef hacks you linked and I've tried using said hacks - it looked a little like this:

#ifdef HAVE_STROPTS_H
// this provides ioctl() definition without conflicting with asm/termios.h
#include <stropts.h>
#else
#define winsize sys_ioctl_h_winsize
#define termio sys_ioctl_h_termio
#include <sys/ioctl.h>
#undef winsize
#undef termio
#endif

This just caused my build to hang without apparent reason (to the point that I couldn't even SSH to the machine anymore).

Whether we should or shouldn't be directly including things like asm/<anything> or <any>bits.h is another question for more experienced C/C++ devs than I am - I've read somewhere that we shouldn't, but what do I know..

jahudka · 2021-11-15T22:00:56Z

Hi, I've rebased the PR onto the 0.10 branch and fixed the CppLint errors, but now two of the CI jobs have failed on seemingly unrelated issues - in one there was a segfault, and both failed the ClientWrapperTest.py test. I'm not sure exactly what the test is supposed to do, or if it might be one of those tests which sometimes just fail (happens to some workflows in our company CI all the time) - help please?

Also, @peternewman, I'm not sure how to interpret your confused smiley reactions in the two discussions above - would you please clarify? I know that the IOCtl.h file I'm proposing with this PR is missing a Copyright notice - I'm not sure how to exactly proceed with that - on the one hand it obviously wouldn't be right to credit myself, since I didn't exactly invent that bit of code out of thin air, but on the other hand it isn't a verbatim copy of the original code, either - so I'm not sure the author(s) of the original would even want to be credited for something they didn't sign off on.. I don't want to step on anyone's toes, I'd very much like to do it right, I'm just not sure what the proper etiquette is, here.

Sorry to put so much work on you guys, I know I should do my homework and I'm trying, it's just that this is all pretty new to me - I come from a web development background, C/C++ really isn't my strong suit, and I don't contribute to OSS projects that often, either.. so please bear with me and know that I have the best of intentions :-)

peternewman · 2021-11-26T18:40:52Z

In any case using asm/termbits.h & asm/ioctls.h allows use of sys/ioctl.h without any conflicts (at least on my system):

What's your system for reference @shenghaoyang ?

@peternewman I've finally successfully completed a build of your PR with the debug code - the pertinent section of the output looks like this:

Great, thanks for doing that, merged as you've probably seen.

This just caused my build to hang without apparent reason (to the point that I couldn't even SSH to the machine anymore).

I'd be surprised if it was something around the build specifically (unless it ran out of memory or something)!

Whether we should or shouldn't be directly including things like asm/<anything> or <any>bits.h is another question for more experienced C/C++ devs than I am - I've read somewhere that we shouldn't, but what do I know..

I'd need to read up, but using a shipped header file generally feels much nicer to me than duplicating the definition in our own file (and risking it becoming out of date). It could also have issues on other OSes where the declaration differs slightly.

Hi, I've rebased the PR onto the 0.10 branch and fixed the CppLint errors, but now two of the CI jobs have failed on seemingly unrelated issues - in one there was a segfault, and both failed the ClientWrapperTest.py test. I'm not sure exactly what the test is supposed to do, or if it might be one of those tests which sometimes just fail (happens to some workflows in our company CI all the time) - help please?

It's probably just flaky tests, although maybe not the segfault, I'll re-run them or have a look.

Also, @peternewman, I'm not sure how to interpret your confused smiley reactions in the two discussions above - would you please clarify?

Hopefully the above covered it, I'd be far happier if we can just include an existing header file somehow for the declaration.

Sorry to put so much work on you guys, I know I should do my homework and I'm trying, it's just that this is all pretty new to me - I come from a web development background, C/C++ really isn't my strong suit, and I don't contribute to OSS projects that often, either.. so please bear with me and know that I have the best of intentions :-)

No worries at all, thanks for your efforts and contribution already!

shenghaoyang · 2021-11-29T06:02:22Z

What's your system for reference @shenghaoyang ?

@peternewman I ran that on Debian "Bullseye".

But it does bring in the kernel definition of struct termios (which may cause conflicts if the version that libc provides needs to be used in the same source file) - I don't think that's used in ExtendedSerial.cpp, so we should be fine.

Alternatively,

See https://github.com/npat-efault/picocom/blob/master/termbits2.h - that header explains the duplicate definition problem in more detail and picocom's approach.

Also, https://github.com/sigrokproject/libserialport/blob/master/linux_termios.c is interesting as well - libserialport isolates the linux specific declarations to a separate compilation unit.

peternewman · 2021-12-06T04:07:46Z

Sorry for taking a while to get back. I think I'd prefer @shenghaoyang 's solution, or just a rather hacky #undef like we've used elsewhere for Windows and like https://stackoverflow.com/a/48521433 rather than redefining the call ourselves.

jahudka · 2021-12-06T22:21:56Z

@peternewman well, in that case I'm afraid I'm not the right man for the job - it's all a little over my head - so we can probably close this and you or somebody else who knows what they're doing can open a new PR?

peternewman · 2021-12-30T02:38:08Z

@peternewman well, in that case I'm afraid I'm not the right man for the job - it's all a little over my head - so we can probably close this and you or somebody else who knows what they're doing can open a new PR?

No worries at all and thanks for all your attempts on this. I left it open because it did indeed work if people needed a quick fix. Now I've managed to have a look and come up with #1760 , I'll close this (as hopefully that works in the physical domain too).

I come from a web development background

We're always looking for people to work on the web front-end (especially the shiny new version) for OLA (and indeed some of our related projects) if you ever want to do some bits of that... 😃

peternewman linked an issue Nov 13, 2021 that may be closed by this pull request

Uarts on RPi4 stopped working (Bullseye & ola 0.10.8) #1749

Closed

peternewman mentioned this pull request Nov 13, 2021

Uarts on RPi4 stopped working (Bullseye & ola 0.10.8) #1749

Closed

peternewman added Maintainability OpSys-Linux Component-Plugin labels Nov 13, 2021

peternewman added this to the 0.10.9 milestone Nov 13, 2021

peternewman requested changes Nov 13, 2021

View reviewed changes

peternewman reviewed Nov 13, 2021

View reviewed changes

peternewman mentioned this pull request Nov 15, 2021

Add more debugging so we can tell why setting the baud rate failed #1751

Merged

jahudka changed the base branch from master to 0.10 November 15, 2021 11:53

jahudka added 2 commits November 15, 2021 12:57

attempt to fix issue with missing stropts.h

113119f

fix lint error

553957a

fix log message when dependencies weren't present at compile time

2649a6a

peternewman assigned jahudka Dec 6, 2021

peternewman mentioned this pull request Dec 30, 2021

Fix UART DMX plugin on Bullseye and more recent Linux distributions #1760

Merged

peternewman closed this Dec 30, 2021

jahudka deleted the jahudka-fix-missing-stropts-h branch December 31, 2021 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to enable native UART on modern systems #1750

Attempt to enable native UART on modern systems #1750

jahudka commented Nov 13, 2021

peternewman left a comment

peternewman Nov 13, 2021

jahudka Nov 13, 2021

peternewman Nov 13, 2021

jahudka Nov 13, 2021 •

edited

Loading

peternewman Nov 13, 2021

jahudka Nov 13, 2021

jahudka commented Nov 13, 2021

peternewman commented Nov 13, 2021

peternewman commented Nov 13, 2021

jahudka commented Nov 13, 2021

jahudka commented Nov 13, 2021 •

edited

Loading

shenghaoyang commented Nov 14, 2021 •

edited

Loading

jahudka commented Nov 15, 2021

jahudka commented Nov 15, 2021

peternewman commented Nov 26, 2021

shenghaoyang commented Nov 29, 2021

peternewman commented Dec 6, 2021

jahudka commented Dec 6, 2021

peternewman commented Dec 30, 2021

Attempt to enable native UART on modern systems #1750

Attempt to enable native UART on modern systems #1750

Conversation

jahudka commented Nov 13, 2021

peternewman left a comment

Choose a reason for hiding this comment

peternewman Nov 13, 2021

Choose a reason for hiding this comment

jahudka Nov 13, 2021

Choose a reason for hiding this comment

peternewman Nov 13, 2021

Choose a reason for hiding this comment

jahudka Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

peternewman Nov 13, 2021

Choose a reason for hiding this comment

jahudka Nov 13, 2021

Choose a reason for hiding this comment

jahudka commented Nov 13, 2021

peternewman commented Nov 13, 2021

peternewman commented Nov 13, 2021

jahudka commented Nov 13, 2021

jahudka commented Nov 13, 2021 • edited Loading

shenghaoyang commented Nov 14, 2021 • edited Loading

jahudka commented Nov 15, 2021

jahudka commented Nov 15, 2021

peternewman commented Nov 26, 2021

shenghaoyang commented Nov 29, 2021

peternewman commented Dec 6, 2021

jahudka commented Dec 6, 2021

peternewman commented Dec 30, 2021

jahudka Nov 13, 2021 •

edited

Loading

jahudka commented Nov 13, 2021 •

edited

Loading

shenghaoyang commented Nov 14, 2021 •

edited

Loading