[Thread] fix OperationalDatasetTimestamp#toTlvValue is not loss-less
OperationalDatasetTimestamp is saving the timestamp as an Instant
object, and this has the problem that when the dataset timestamp is
written to a TLV, it's going to loss precision as the nanoseconds
of the Instant is converted to "Ticks". This results in bug that
the dataset timestamp doesn't equal to the same value when it's
converted back from a TLV:
```
var timestamp1 = OperationalDatasetTimestamp.fromInstant(Instant.ofEpochSecond(100, 999938900));
byte[] tlv = timestamp1.toTlvValue();
timestamp2 = OperationalDatasetTimestamp.fromTlvValue(tlv);
assertThat(timestamp2).isEqualTo(timestamp1); // This fails!!!
```
Bug: 308408536
Test: atest ThreadNetworkUnitTests CtsThreadNetworkTestCases
Change-Id: I81b482a05337b6d1150edaa28b860034592a4347
diff --git a/thread/framework/java/android/net/thread/OperationalDatasetTimestamp.java b/thread/framework/java/android/net/thread/OperationalDatasetTimestamp.java
index bda9373..520acbd 100644
--- a/thread/framework/java/android/net/thread/OperationalDatasetTimestamp.java
+++ b/thread/framework/java/android/net/thread/OperationalDatasetTimestamp.java
@@ -43,9 +43,10 @@
/** @hide */
public static final int LENGTH_TIMESTAMP = Long.BYTES;
- private static final long TICKS_UPPER_BOUND = 0x8000;
+ private static final int TICKS_UPPER_BOUND = 0x8000;
- private final Instant mInstant;
+ private final long mSeconds;
+ private final int mTicks;
private final boolean mIsAuthoritativeSource;
/**
@@ -55,18 +56,34 @@
* {@link instant#getNano()} based on frequency of 32768 Hz, and {@code isAuthoritativeSource}
* is set to {@code true}.
*
+ * <p>Note that this conversion can lose precision and a value returned by {@link #toInstant}
+ * may not equal exactly the {@code instant}.
+ *
* @throws IllegalArgumentException if {@code instant.getEpochSecond()} is larger than {@code
* 0xffffffffffffL}
+ * @see toInstant
*/
@NonNull
public static OperationalDatasetTimestamp fromInstant(@NonNull Instant instant) {
- return new OperationalDatasetTimestamp(instant, /* isAuthoritativeSource= */ true);
+ int ticks = getRoundedTicks(instant.getNano());
+ long seconds = instant.getEpochSecond() + ticks / TICKS_UPPER_BOUND;
+ // the rounded ticks can be 0x8000 if instant.getNano() >= 999984742
+ ticks = ticks % TICKS_UPPER_BOUND;
+ return new OperationalDatasetTimestamp(seconds, ticks, true /* isAuthoritativeSource */);
}
- /** Converts this {@link OperationalDatasetTimestamp} object to an {@link Instant}. */
+ /**
+ * Converts this {@link OperationalDatasetTimestamp} object to an {@link Instant}.
+ *
+ * <p>Note that the return value may not equal exactly the {@code instant} if this object is
+ * created with {@link #fromInstant}.
+ *
+ * @see fromInstant
+ */
@NonNull
public Instant toInstant() {
- return mInstant;
+ long nanos = Math.round((double) mTicks * 1000000000L / TICKS_UPPER_BOUND);
+ return Instant.ofEpochSecond(mSeconds, nanos);
}
/**
@@ -100,10 +117,7 @@
public byte[] toTlvValue() {
byte[] tlv = new byte[LENGTH_TIMESTAMP];
ByteBuffer buffer = ByteBuffer.wrap(tlv);
- long encodedValue =
- (mInstant.getEpochSecond() << 16)
- | ((mInstant.getNano() * TICKS_UPPER_BOUND / 1000000000L) << 1)
- | (mIsAuthoritativeSource ? 1 : 0);
+ long encodedValue = (mSeconds << 16) | (mTicks << 1) | (mIsAuthoritativeSource ? 1 : 0);
buffer.putLong(encodedValue);
return tlv;
}
@@ -125,10 +139,6 @@
@IntRange(from = 0x0, to = 0xffffffffffffL) long seconds,
@IntRange(from = 0x0, to = 0x7fff) int ticks,
boolean isAuthoritativeSource) {
- this(makeInstant(seconds, ticks), isAuthoritativeSource);
- }
-
- private static Instant makeInstant(long seconds, int ticks) {
checkArgument(
seconds >= 0 && seconds <= 0xffffffffffffL,
"seconds exceeds allowed range (seconds = %d,"
@@ -138,25 +148,8 @@
ticks >= 0 && ticks <= 0x7fff,
"ticks exceeds allowed ranged (ticks = %d, allowedRange" + " = [0x0, 0x7fff])",
ticks);
- long nanos = Math.round((double) ticks * 1000000000L / TICKS_UPPER_BOUND);
- return Instant.ofEpochSecond(seconds, nanos);
- }
-
- /**
- * Creates new {@link OperationalDatasetTimestamp} object.
- *
- * @throws IllegalArgumentException if {@code instant.getEpochSecond()} is larger than {@code
- * 0xffffffffffffL}
- */
- private OperationalDatasetTimestamp(@NonNull Instant instant, boolean isAuthoritativeSource) {
- requireNonNull(instant, "instant cannot be null");
- long seconds = instant.getEpochSecond();
- checkArgument(
- seconds >= 0 && seconds <= 0xffffffffffffL,
- "instant seconds exceeds allowed range (seconds = %d, allowedRange = [0x0,"
- + " 0xffffffffffffL])",
- seconds);
- mInstant = instant;
+ mSeconds = seconds;
+ mTicks = ticks;
mIsAuthoritativeSource = isAuthoritativeSource;
}
@@ -171,13 +164,12 @@
/** Returns the seconds portion of the timestamp. */
public @IntRange(from = 0x0, to = 0xffffffffffffL) long getSeconds() {
- return mInstant.getEpochSecond() + getRoundedTicks(mInstant.getNano()) / TICKS_UPPER_BOUND;
+ return mSeconds;
}
/** Returns the ticks portion of the timestamp. */
public @IntRange(from = 0x0, to = 0x7fff) int getTicks() {
- // the rounded ticks can be 0x8000 if mInstant.getNano() >= 999984742
- return (int) (getRoundedTicks(mInstant.getNano()) % TICKS_UPPER_BOUND);
+ return mTicks;
}
/** Returns {@code true} if the timestamp comes from an authoritative source. */
@@ -208,13 +200,14 @@
return false;
} else {
OperationalDatasetTimestamp otherTimestamp = (OperationalDatasetTimestamp) other;
- return mInstant.equals(otherTimestamp.mInstant)
+ return mSeconds == otherTimestamp.mSeconds
+ && mTicks == otherTimestamp.mTicks
&& mIsAuthoritativeSource == otherTimestamp.mIsAuthoritativeSource;
}
}
@Override
public int hashCode() {
- return Objects.hash(mInstant, mIsAuthoritativeSource);
+ return Objects.hash(mSeconds, mTicks, mIsAuthoritativeSource);
}
}
diff --git a/thread/tests/unit/src/android/net/thread/OperationalDatasetTimestampTest.java b/thread/tests/unit/src/android/net/thread/OperationalDatasetTimestampTest.java
index 32063fc..2244a89 100644
--- a/thread/tests/unit/src/android/net/thread/OperationalDatasetTimestampTest.java
+++ b/thread/tests/unit/src/android/net/thread/OperationalDatasetTimestampTest.java
@@ -27,6 +27,8 @@
import org.junit.Test;
import org.junit.runner.RunWith;
+import java.time.Instant;
+
/** Unit tests for {@link OperationalDatasetTimestamp}. */
@SmallTest
@RunWith(AndroidJUnit4.class)
@@ -58,4 +60,22 @@
assertThat(timestamp2).isEqualTo(timestamp1);
}
+
+ @Test
+ public void toTlvValue_timestampFromInstant_conversionIsLossLess() {
+ // This results in ticks = 999938900 / 1000000000 * 32768 = 32765.9978752 ~= 32766.
+ // The ticks 32766 is then converted back to 999938964.84375 ~= 999938965 nanoseconds.
+ // A wrong implementation may save Instant.getNano() and compare against the nanoseconds
+ // and results in precision loss when converted between OperationalDatasetTimestamp and the
+ // TLV values.
+ OperationalDatasetTimestamp timestamp1 =
+ OperationalDatasetTimestamp.fromInstant(Instant.ofEpochSecond(100, 999938900));
+
+ OperationalDatasetTimestamp timestamp2 =
+ OperationalDatasetTimestamp.fromTlvValue(timestamp1.toTlvValue());
+
+ assertThat(timestamp2.getSeconds()).isEqualTo(100);
+ assertThat(timestamp2.getTicks()).isEqualTo(32766);
+ assertThat(timestamp2).isEqualTo(timestamp1);
+ }
}