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

serialport.set fails on Linux due to Low Latency Mode (@serialport/bindings v9.0.7) #2240

Closed
GazHank opened this issue Apr 29, 2021 · 13 comments

Comments

@GazHank
Copy link
Contributor

GazHank commented Apr 29, 2021

Summary of Problem

  • What are you trying to do? set({dtr: true}) or set({dtr: false})
  • What happens? Error: Operation not supported, cannot set (thrown by serialport_unix.cpp line 342)
  • What should have happened? operation should succeed without error

Code to Reproduce the Issue

const serialport = new SerialPort(path)
const delay = time => new Promise(res=>setTimeout(res,time))

serialport.on('open', function() {
    serialport.set({dtr: true});
    await delay(1000)
    serialport.set({dtr: false});
}

Versions, Operating System and Hardware

  • SerialPort v9.0.7 (@serialport/bindings v9.0.7)
  • Node.js v14.16.0
  • Linux 20.04.2 LTS 64bit
  • Hardware and chipset? (attempting to send dtr to arduino:samd)

Additional info

This issue replaces issue #2239 to improve clarity.

It appears that this issue relates to the implementation of #2102. Specifically within the set method at serialport_unix.cpp (line 335) there is an attempt made to set low latency mode irrespective of if lowLatency was specified in the method call. Adding a check for if (data->lowLatency) per below seems to resolve this, but there may be another more preferable solution.

#if defined(__linux__)
  if (data->lowLatency) {
    int err = linuxSetLowLatencyMode(data->fd, data->lowLatency);
    if (err == -1) {
      snprintf(data->errorString, sizeof(data->errorString), "Error: %s, cannot get", strerror(errno));
      return;
    } else if(err == -2) {
      snprintf(data->errorString, sizeof(data->errorString), "Error: %s, cannot set", strerror(errno));
      return;
    }
  }
  #endif

I have tried adding a check for defined(ASYNC_LOW_LATENCY) but this doesnt seem to help...

This issue is not present in @serialport/bindings v9.0.4, however if serialport is defined in the dependencies, then when the packages are installed the underlying packages (e.g. bindings) will be upgraded to v9.0.7 resulting in this issue appearing for anyone using serialport >=9.0.0

At present the only way to workaround this issue is to revert to serialport v8, or add @serialport/bindings 9.0.4 into the package.json resolutions per below:

    "resolutions": {
        "@serialport/bindings": "9.0.4"
    }
@GazHank
Copy link
Contributor Author

GazHank commented Apr 29, 2021

Hi @reconbot & @jeremy-mf, I wanted to loop you both in as you were involved in the implementation of #2102 so am sure you will be far more knowledgable about this issue than I. I hope that's ok

@AlexDygma looping you in for visibility due to impact on Dygmalab/Bazecor#207. I think we need to revert to SerialPort v8 until this is resolved

GazHank added a commit to GazHank/node-serialport that referenced this issue Apr 29, 2021
@GazHank
Copy link
Contributor Author

GazHank commented May 1, 2021

Ok, so I have a bit of an update on this issue:

The main problem is that setting low latency mode requires admin access.

If we try to call to run the set method without admin rights then we hit a few problems:

  1. An attempt is made to set the serial configuration (TIOCSSERIAL) even if the set method call didn't specify the low latency mode (positively or negatively).
  2. Due to the sequencing of the set method, we try to set the low latency mode before TIOCCBRK or TIOCMSET; as such when low latency fails it never attempts to set the other control signal bits.

This all means that since v9.0.7 in order to run the set method we now either need to change the permissions on the ports, or run as sudo (in the case of electron apps sudo yarn start --no-sandbox).

As such I propose:

  • Change linuxSetLowLatencyMode to only try to set the TIOCSSERIAL flags if the value needs to be changed
  • Change the sequence of the EIO_Set method within serialport_unix.cpp to allow the other control bits to be changed before setting the low latency mode. Since the other settings require less privileged access levels, they are more likely to succeed, and shouldn't be broken by low latency.

Further to this, there seems to be an issue with the get method (regardless of admin rights), with the method resulting in *** stack smashing detected ***: terminated. Sorry I'd not previously identified this as my usual program logic doesn't use the get method... I think the issue with the get, is that it is trying to operate on the output of TIOCGSERIAL directly rather than looking at the .flags property of the returned data. It also seems logical to change the sequence of the get method for the same permissions (i.e. lets allow other modem status info to be returned even if we dont have access to read TIOCGSERIAL).

I think I've got a set of changes which work for this but just need to run some more checks before updating/replacing the Pull Request

Do let me know if you have any comments or suggestions

Many thanks

Gareth

GazHank added a commit to GazHank/node-serialport that referenced this issue May 1, 2021
Fix for linux low latency mode issue serialport#2240 (revised)
reconbot pushed a commit that referenced this issue May 26, 2021
…#2241)

* only set lowLatency if defined
* fix set and get methods for low_latency
* clarify error messages

Proposed fix to address #2240
@hugohil
Copy link

hugohil commented May 27, 2021

Hey all,
I'm posting here because I really had a hard time finding this one, as some characteristics of my issue are a bit different. I'm not sure it's related to low latency though... I was trying to switch dtr as well but on a raspberry pi, and my error was

Inappropriate ioctl for device, cannot get

Reverting to v8 did fix the issue. I see you pushed fixes for this but I haven't got the chance to test those, but as soon as they're published on npm I'll check that.

@GazHank
Copy link
Contributor Author

GazHank commented May 27, 2021

hi @hugohil

would you be able to share a little extra info on the environment and version you are using? Which specific version seems to work, vs which one is broken. Any extra info on Linux distro, version and info about the node version you are building would be a bonus.

The changes that are in the pipe for this should at the very least improve the clarity of the error you are seeing, since low latency issues will trigger a slightly more specific error message, but I'm not sure if this is a new issue for us to take a look at.

I have to say it's quite interesting that you are hitting this issue during the GET step rather than SET which is where I was seeing issues.

An extra check you might want to try is to run you app with admin rights (sudo) and see if you still get the error, since the main issue that low latency was actually triggering was an elevated permission requirement

Cheers

Gaz

@hugohil
Copy link

hugohil commented May 27, 2021

Hey ! @GazHank
Sure, sorry my first answer was a bit on the fast side.
I've been experiencing this both on Raspberry Pi 3 Model B+, as well as on Raspberry Pi 4. I don't have the details of the OS right now (and won't have access to the Pi until next week), but they are fairly recent version of Raspberry Pi OS (I cloned the SD card 2 or 3 weeks ago). It might also be worth noticing that I had zero problem with 9.0.7 on macOS 11.3.1 (Intel).
Regarding serialport, 9.0.7 was causing the issue, and reverting to 8.0.8 did the trick.
Finally, I mostly tried with node 14.17.0 LTS, but had a rather similar experience with v16.1.0.

To be honnest, I did take a fair amount of time around this issue and this is the closest I got, which is still not quite the same thing. I'm working with Lidar scanner, more specifically RPLidar ones, and the Pi was running the python libraries as it should, getting informations and all from the scanner. I checked and those libraries were actually switching the dtr flag, so it really something in latest version of serialport that is causing this.

I'll try with more elevated permissions level and update you.
Thanks a lot for your time!

@GazHank
Copy link
Contributor Author

GazHank commented May 27, 2021

no problem @hugohil, I really hope the latest changes fix your issues. If v8 works for you on all platforms and v9.0.x works for all platforms except linux then I think this could be the fix you need.

Let me know how you get on once you have access the the device again, I'd be happy to try to help.

@reconbot
Copy link
Member

reconbot commented May 28, 2021

[email protected] and @serialport/[email protected] have been released the bindings are building now

@GazHank
Copy link
Contributor Author

GazHank commented May 28, 2021

Thanks again @reconbot, I've checked the upgraded the packages on Win10 and Linux, and everything looks good.

@hugohil if you are still facing issues, we will probably need to raise a new issue, but feel free to tag me and I'll try to help if I can

@vicatcu
Copy link

vicatcu commented Jul 2, 2021

@reconbot @GazHank et al, I'm on a laptop with Ubuntu 20.04 (i.e. Linux / x64), with Node 14.15.4, and serialport 9.2.0 and I'm getting this exact problem. Has there been a regression or was this closed prematurely?

@GazHank
Copy link
Contributor Author

GazHank commented Jul 2, 2021

Hi @vicatcu could you confirm the error message your are seeing? and perhaps the code you are running?

I've just rerun some checks on Ubuntu 20.04.2 and I'm not seeing the original issue. I'm able to set DTR: true and see the desired effect on the usb device. I might need a little more info to try to recreate the problem

Cheers

Gaz

lvogt pushed a commit to lvogt/node-serialport that referenced this issue Aug 10, 2021
…serialport#2241)

* only set lowLatency if defined
* fix set and get methods for low_latency
* clarify error messages

Proposed fix to address serialport#2240
@tcbennun
Copy link

tcbennun commented Sep 6, 2021

I'm still facing an issue (or a very similar one) when trying to set anything (in my case, RTS).

  • Node: 16.8.0
  • serialport: 9.2.0
  • Linux kernel: 5.10.46, amd64 (Debian)

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]

Attempted to run this script both as user and root (with sudo env "PATH=$PATH" node main.js).

The serial device is a standard CH340.

@reconbot
Copy link
Member

reconbot commented Sep 6, 2021

Can you open this on a new issue?

@fromage9747
Copy link

fromage9747 commented Apr 13, 2024

2024 Debian 12 and latest version of serial port getting Operation not supported, cannot set when trying to set { dtr: true, rts: true }

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

No branches or pull requests

6 participants