Linux low latency fix (allow set without changing low latency mode) #2241

GazHank · 2021-04-29T14:07:24Z

Only set low Latency mode if lowLatency is defined

Proposed fix to address #2240

Proposed fix to address serialport#2240

reconbot · 2021-04-30T14:23:08Z

packages/bindings/src/serialport_unix.cpp

-  } else if(err == -2) {
-    snprintf(data->errorString, sizeof(data->errorString), "Error: %s, cannot set", strerror(errno));
-    return;
+  if (data->lowLatency) {


so if it's undefined or false its not set, and if it's true it is set? does this mean you can't turn it off? Would you ever want to turn it off?

Hi @reconbot, thanks for the quick response :-)

I have to admit, toggling low latency on and off isn't really something I considered. Given how the user sets low latency within linux, and the use cases I've seen I don't know if toggling is common.

I'm not certain this is the best fix, and bigger change may be needed to fully address this feature, but my priorities when proposing this were:

to get the set method to work again for people who don't need to invoke low latency mode

continue to allow anyone who does want to invoke the low latency mode to enable it

keep the change as small and low impact as possible (so we can fix point 1 asap)

As I understand it, the current code and the default behaviour of Linux are not aligned. In particular, I believe that in order to be able to invoke lowLatency mode you need to first change the default behaviour of the port, using something like: sudo setserial /dev/ttyUSB0 low_latency. As such, by default Linux will not allow you to invoke low latency mode in Serialport, however the code within the set method tries to configure it irrespective of the options the user supplies to the set method. Even specifying port.set({dtr: true, LowLatency: false }) or port.set({dtr: true}) will cause an attempt to set the low latency bit to zero and error out.

Having thought about this a little more, I wonder if the set method is really the right place to be activating low latency mode, since all of the other options within the set method are really control signals. I can't help thinking about low latency mode being more like setting the baud rate etc. As such it would seem to me to be more appropriate to allow it to be defined within the port open method, and within the update method. Of course we probably need to find a way to check if we have permission to enable low latency mode first otherwise we will continue to throw errors. The potential downside of treating low latency mode like baud rate is that baud rate has it's own get method, it would be a shame to have to add a get method just for this since it is platform specific.

Let me know if you would like me to try a different way to fix this

Cheers

Gareth

Upon further investigation I think we need a better fix for this. I'll add more info to the issue based on my improved understanding of what's going on...

I will try to make a new PR to address this more completely ASAP, but until a better fix is available I still think this is an acceptable workaround to get the v9 code working on Linux for all users.

Per my comment in the issue, anyone placing a dependency on "serialport": "9.0.0" will get the broken version of bindings since the interdependence is specified as "^9.0.0".

Fix for linux low latency mode issue serialport#2240 (revised)

GazHank · 2021-05-01T17:28:23Z

Improved fix for low latency mode:

Changes

serialport_linux.cpp:

Revise the linux set method to only attempt an update if required (if the bit doesn't need to be flipped then don't attempt an unnecessary call for which we might not have permission)
Addition of a linux specific linuxGetLowLatencyMode method - for consistency with linuxGetSystemBaudRate, allowing removal of the logic from the more general unix code

serialport_linux.h:

Addition of a linux specific linuxGetLowLatencyMode method - for consistency with linuxGetSystemBaudRate

serialport_unix.cpp:

Revise sequence of set method - to allow control bits (e.g. dtr) to be set even if low latency fails
Revise sequence of get method - to allow control bits (e.g. dtr) to be returned even if get of low latency info fails
Revise get method to invoke linux specific method (point 2 above, and removing broken logic in the process)
Revise the error messages for the set and get messages for the low latency calls to make it clearer when these fail - since they are likely to fail when we don't have suitable access rights

Testing

Tested on Linux Ubuntu LTS and Win10:

Linux without admin rights

Set method call {dtr: true} - succeeds without error
Set method call {dtr: true, lowLatency: true} - dtr correctly set, error reported for lowLatency, but handled without issue
Get method call - succeeds without error

Linux with admin rights

Set method call {dtr: true} - succeeds without error
Set method call {dtr: true, lowLatency: true} - succeeds without error
Get method call - succeeds without error

Windows 10

Set method call {dtr: true} - succeeds without error
Set method call {dtr: true, lowLatency: true} - succeeds without error (lowLatency ignored)
Get method call - succeeds without error

Additional considerations

The the focus of this change was entirely on addressing errors per #2240 as such I have not performed any actual benchmarking of the lowLatency mode to check for changes to latency (or increases to CPU load - setserial has info on expected benefits/impacts).

GazHank · 2021-05-01T17:30:46Z

@reconbot do let me know if you would like to discuss or revise any of these changes, or if you would prefer if I setup a new PR for this

reconbot

Sorry for the delay in review. I think you uncovered a bug, and also some feedback on simplifying the call to set.

reconbot · 2021-05-24T14:30:23Z

packages/bindings/src/serialport_linux.cpp

-    return -2;
+  if (oldFlags != ss.flags)
+  {
+    if (ioctl(fd, TIOCSSERIAL, &ss)) {


I think this is wrong - https://man7.org/linux/man-pages/man2/ioctl.2.html#RETURN_VALUE

It should be a positive number on success not failure.

agreed, looks like we should check for negative values to indicate errors instead:
if (ioctl(fd, TIOCSSERIAL, &ss) < 0) {

although the same also seems to be true for the setting the baud rate:
if (ioctl(fd, TCSETS2, &t)) { return -2; }
does that need a fix too?

reconbot · 2021-05-24T14:32:04Z

packages/bindings/src/serialport_linux.cpp

@@ -43,17 +43,34 @@ int linuxSetLowLatencyMode(const int fd, const bool enable) {
  if (ioctl(fd, TIOCGSERIAL, &ss)) {
    return -1;
  }
+
+  int oldFlags = ss.flags;


lets see if oldFlags & ASYNC_LOW_LATENCY == enable and just return early if we don't have to do anything?

good shout that is lots nicer

reconbot · 2021-05-24T14:32:25Z

packages/bindings/src/serialport_unix.cpp

@@ -332,17 +332,6 @@ void EIO_Set(uv_work_t* req) {
    bits |= TIOCM_DSR;
  }

-  #if defined(__linux__)


reconbot · 2021-05-26T12:45:09Z

packages/bindings/src/serialport_linux.cpp

-    ss.flags &= ~ASYNC_LOW_LATENCY;
-  }
+  if (ss.flags & ASYNC_LOW_LATENCY != enable) {
+


just a nitpick it's better to return early than it is to nest

reconbot · 2021-05-26T12:45:37Z

packages/bindings/src/serialport_linux.cpp

-  if (oldFlags != ss.flags)
-  {
-    if (ioctl(fd, TIOCSSERIAL, &ss)) {
+    if (ioctl(fd, TIOCSSERIAL, &ss) < 0) {


another nitpick ioctl will always return -1 on error never anything else

…serialport#2241) * only set lowLatency if defined * fix set and get methods for low_latency * clarify error messages Proposed fix to address serialport#2240

only set lowLatency if defined

b8e1a48

Proposed fix to address serialport#2240

reconbot reviewed Apr 30, 2021

View reviewed changes

GazHank and others added 3 commits May 1, 2021 17:05

fix set and get methods for low_latency

ca2c3d8

clarify error messages

7310751

Merge pull request #1 from GazHank/pr/GazHank/2241-1

60a167b

Fix for linux low latency mode issue serialport#2240 (revised)

reconbot reviewed May 24, 2021

View reviewed changes

Changes based on feedback

1e0d14f

reconbot approved these changes May 26, 2021

View reviewed changes

reconbot merged commit fb53b99 into serialport:master May 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux low latency fix (allow set without changing low latency mode) #2241

Linux low latency fix (allow set without changing low latency mode) #2241

GazHank commented Apr 29, 2021

reconbot Apr 30, 2021

GazHank Apr 30, 2021 •

edited

Loading

GazHank May 1, 2021

GazHank commented May 1, 2021

GazHank commented May 1, 2021

reconbot left a comment

reconbot May 24, 2021

GazHank May 24, 2021

reconbot May 25, 2021

reconbot May 24, 2021

GazHank May 24, 2021

reconbot May 24, 2021

reconbot May 26, 2021

reconbot May 26, 2021

Linux low latency fix (allow set without changing low latency mode) #2241

Linux low latency fix (allow set without changing low latency mode) #2241

Conversation

GazHank commented Apr 29, 2021

Choose a reason for hiding this comment

GazHank Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GazHank commented May 1, 2021

Changes

Testing

Additional considerations

GazHank commented May 1, 2021

reconbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GazHank Apr 30, 2021 •

edited

Loading