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

Inter-frame sleep time calculation can generate invalid sleep time #6

Closed
merose opened this issue Mar 26, 2021 · 2 comments
Closed

Comments

@merose
Copy link

merose commented Mar 26, 2021

What is observed when running the SLE bridge and sending a CLTU:

Exception in thread "Thread-2" java.lang.IllegalArgumentException: nanosecond timeout value out of range
	at java.lang.Thread.sleep(Thread.java:332)
	at org.yamcs.sle.udpslebridge.UdpCltuUplinker.uplink(UdpCltuUplinker.java:54)
	at org.yamcs.sle.provider.CltuServiceProvider.uplinkCltu(CltuServiceProvider.java:438)
	at org.yamcs.sle.provider.CltuServiceProvider.runUplinkQueue(CltuServiceProvider.java:426)
	at org.yamcs.sle.provider.CltuServiceProvider.lambda$processStartInvocation$0(CltuServiceProvider.java:152)
	at java.lang.Thread.run(Thread.java:748)

This appears to be caused by insufficient precision in the sleep duration calculation:

        long durationNs = cltuData.length * 8 * 1000_000_000 / bitrate;
        int millis = (int) (durationNs / 1000_000);
        int nanos = (int) (durationNs % 1000_000);

The problem is that cltuData.length is of type int, as are all the operands: two constants 8 and 1000_000_000, and the bitrate. This causes the multiplication and division on the right-hand side to use int precision, which can overflow and generate a negative value for durationNs.

The easiest way to fix this is to change the constants to long type:

        long durationNs = cltuData.length * 8L * 1000_000_000L / bitrate;
        int millis = (int) (durationNs / 1000_000);
        int nanos = (int) (durationNs % 1000_000);
@merose
Copy link
Author

merose commented Mar 26, 2021

A patch file for the change.

diff --git a/src/main/java/org/yamcs/sle/udpslebridge/UdpCltuUplinker.java b/src/main/java/org/yamcs/sle/udpslebridge/UdpCltuUplinker.java
index 0e73e60f9e..d2202651f7 100644
--- a/src/main/java/org/yamcs/sle/udpslebridge/UdpCltuUplinker.java
+++ b/src/main/java/org/yamcs/sle/udpslebridge/UdpCltuUplinker.java
@@ -6,7 +6,6 @@ import java.net.DatagramSocket;
 import java.net.InetAddress;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-
 import org.yamcs.sle.CcsdsTime;
 import org.yamcs.sle.Constants.ForwardDuStatus;
 import org.yamcs.sle.provider.CltuUplinker;
@@ -46,7 +45,7 @@ public class UdpCltuUplinker implements CltuUplinker {
 
         DatagramPacket dtg = new DatagramPacket(cltuData, cltuData.length, address, port);
         ur.startTime = CcsdsTime.now();
-        long durationNs = cltuData.length * 8 * 1000_000_000 / bitrate;
+        long durationNs = cltuData.length * 8L * 1000_000_000L / bitrate;
         int millis = (int) (durationNs / 1000_000);
         int nanos = (int) (durationNs % 1000_000);

@xpromache
Copy link
Member

Implemented on master, thanks!

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

2 participants