SerialPort.set() fails on Linux due to failed ioctl TIOCGSERIAL call (error code ENOTTY), with USB serial devices

[tcbennun]

This occurs when set() is used at all. In the example below, the only field used in the call is rts.

• Node: 16.8.0
• serialport: 9.2.0
• Linux kernel: 5.10.46
• Arch: amd64
• Distribution: Debian 10

The serial device in question, in my case, is a CH340C IC, driven by the Linux ch341 driver. The CH34x series is incredibly common, so it is important to maintain support.

Minimal example:

const SerialPort = require("serialport");

const port = new SerialPort("/dev/ttyUSB0", {
  autoOpen: false,
  baudRate: 460800,
  rtscts: true,
});

port.on("error", (error) => {
  console.log(error);
})

port.on("open", async () => {
  console.log("opened");
  port.set({rts: true, });
});

port.open();

And the output:

opened
[Error: Error: Inappropriate ioctl for device, cannot get low latency]

This occurs because TIOCGSERIAL is not supported for certain USB serial drivers, including ch341. See v5.10.46:usb-serial.c:400.

In kernel v5.13, this patch series was introduced to add generic support for TIOCGSERIAL and TIOCSSERIAL, to fix these pointless incompatibilities.

In order to properly support kernels prior to v5.13 (so, most distributions as of 2021), we should not fail due to TIOCGSERIAL returning an error. Instead, we can assume that lowLatency is permanently false if TIOCGSERIAL fails.

[tcbennun]

(okay, so it doesn't "fail", because linuxSetLowLatencyMode is attempted right at the end of EIO_Set, meaning other sets actually do work; but it generates an unnecessary error for sure.)

[GazHank]

Thanks for raising this @tcbennun it looks like the lowlatency fix needs some more work. I'm glad the the resequencing of the logic worked, so that only the low latency aspect fails, but the rest of the set process succeeds. As you suggest the low latency failure error isn't very useful in situations where you never tried to trigger low latency.

Could you check if the get functionality is working in your scenario, as I fear the Get will generate an error and not return the basic info because of the low latency problem?

[tcbennun]

Could you check if the get functionality is working in your scenario, as I fear the Get will generate an error and not return the basic info because of the low latency problem?

That's correct @GazHank. When get is attempted, the callback receives the Inappropriate ioctl for device error and the status object is undefined.

The get operation should attempt to get lowLatency, but just leave the field undefined if ENOTTY occurs.

The set operation should not attempt to set lowLatency if the field in the set object is undefined, I suppose. If the field is defined, however, it should rightly attempt and fail if ENOTTY occurs.

[GazHank]

Thanks @tcbennun, it's disappointing to be correct :-(

We previously tried to use #if defined(__linux__) && defined(ASYNC_LOW_LATENCY) to protect environments which didn't understand (/were incompatible with) the low latency mode, but that doesn't seem to do the trick. Since that is invoked at compile time, we would need to maintain 2 binaries if we continue with this type of approach.

As I see it there are 3 potential solutions:

• Find an alternative to ASYNC_LOW_LATENCY which can be used (per above this may have issues for the prebuilds)
• Maintain LowLatency as a bool, but disable all of the error messages (let it fail silently)
• Switch LowLatency to have 3 possible values (true, false and unknown/undefined), we can then retain the error handling, but only invoke this if someone actually tries to set LL, for the get function a return value of undefined would indicate an error or lack of success.

I'm considering the 3rd option to be preferable, but keen to get any other opinions or ideas.

[reconbot]

Maybe enable the feature during open and don’t mess with it if it’s disabled.

On Wed, Sep 15, 2021 at 1:09 PM Gareth Hancock @.***> wrote:

Thanks @tcbennun https://github.com/tcbennun, it's disappointing to be correct :-(

We previously tried to use #if defined(linux) && defined(ASYNC_LOW_LATENCY) to protect environments which didn't understand (/were incompatible with) the low latency mode, but that doesn't seem to do the trick. Since that is invoked at compile time, we would need to maintain 2 binaries if we continue with this type of approach.

As I see it there are 3 potential solutions:

• Find an alternative to ASYNC_LOW_LATENCY which can be used (per above this may have issues for the prebuilds)
• Maintain LowLatency as a bool, but disable all of the error messages (let it fail silently)
• Switch LowLatency to have 3 possible values (true, false and unknown/undefined), we can then retain the error handling, but only invoke this if someone actually tries to set LL, for the get function a return value of undefined would indicate an error or lack of success.

I'm considering the 3rd option to be preferable, but keen to get any other opinions or ideas.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/serialport/node-serialport/issues/2312#issuecomment-920208464, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGK3UFTO3R7RTGTEADPVLUCDHNBANCNFSM5DQ3RX4Q .

--

Francis Gulotta @.***

[GazHank]

Maybe enable the feature during open and don’t mess with it if it’s disabled.

Makes sense, although at the moment I think LL is only an option during set, and cannot be defined during open. I'm once again inclined to think about low latency being similar to baud rate and therefore should be configured (optionally) during open and update rather than during the set method. I just don't like the idea of implementing breaking changes unless I can avoid it

[reconbot]

It's a breaking change but not commonly used one. I'd rather get us to a better api than an iffy one.

[GazHank]

Sounds good.

I'll see if I can draft something over the next few days. I'd consider including this in with the N-API changes (to avoid duplication) but am not sure when they will be ready for prime time yet

[reconbot]

We'd probably want to beta that for a while too, it's going to have a lot of people jumping on it, and should be a major change too. So release wise maybe we do both in rc releases?

[GazHank]

I've proposed a quick patch for the errors being thrown, hopefully this addresses this issue while a API breaking change is worked on.

@tcbennun it would be great if you could check if that patch works for you. I'm conscious that we still try to invoke the same operations in Linux, we just ignore the unnecessary errors. So in your case where you aren't trying to use the low latency mode on unsupported kit I'm hoping it all looks peachy and low latency just reports as false

[GazHank]

Hi @tcbennun have you managed to test the recent releases? Hopefully they should patch the issues you have; giving a little time to work on the longer term API changes

[tcbennun]

Sorry for the delay @GazHank , I just got a chance to test. This has indeed fixed the issue for me! I can set and get without error - I only receive an error when I try to set lowLatency to true, as expected. And lowLatency reports false.

[GazHank]

No probs @tcbennun thanks for the confirmation. I've incorporate the same change into the branch that I hope will be version 10; so you shouldn't see any regressions when we deploy future versions🤞🏼

I still would like to work out a better API for the LowLatency functionality, but it's firmly on the TODO list for now